GoogleCrashdumpUploader: adds Upload(string*) API to get the HTTP response.

GoogleCrashdumpUploader: adds Upload(string*) API to get the HTTP response.

[gunsch]

FYI: this is my first CL sent over to Breakpad, apologies if I sent anything out
incorrectly.

[Mark Mentovai]

Please use mark@chromium.org (fixed).

What’s “response?” It’s just a string. Is it just the response body? What about
the header? What about the HTTP status code?

What’s this for? How will the information be used?

[gunsch]

On 2014/09/16 18:39:54, Mark Mentovai wrote:
> Please use mailto:mark@chromium.org (fixed).
> 
> What’s “response?” It’s just a string. Is it just the response body? What
about
> the header? What about the HTTP status code?
> 
> What’s this for? How will the information be used?

On 2014/09/16 18:39:54, Mark Mentovai wrote:
> Please use mailto:mark@chromium.org (fixed).
> 
> What’s “response?” It’s just a string. Is it just the response body? What
about
> the header? What about the HTTP status code?
> 
> What’s this for? How will the information be used?

Hi,
 Thanks for taking a look. I realize this is coming out of the blue, but we're
hoping to upstream a few small additions to GoogleCrashdumpUploader. Chromecast
uses the class fairly heavily but currently has a completely forked copy. We're
in the process of unforking from Chromium and upstreaming our code to Chromium,
largely in src/chromecast. (see crbug/336640)

We could upload our own forked copy to src/chromecast in Chromium and use
LibcurlWrapper as-is, but it seems to us that it'd be preferable to add the
functionality here rather than duplicating the class. Let me know if you'd
prefer that to us sending patches to breakpad.

For your CL comments:

"response" is just the response body. However, it's similarly named to
LibcurlWrapper::SendRequest which it calls, which just names it server_response.
I've changed the name to match exactly.

As to how it's used: 

Chromecast uses the Crash frontend not just for crash reporting, but to grab and
upload minidumps at a given point in time, similar to an "adb bugreport". The
Crash frontend prints the crash report ID in the response body, hence this API
change. Chromecast platform then communicates the crash report ID back to the
requesting device.

[Mark Mentovai]

OK, then the parameter should at least be called http_response_body.

But I don’t know how useful http_response_body is without http_status_code and
http_response_header. Can we get out-parameters for those too?

[lcwu]

https://breakpad.appspot.com/7714002/diff/30002/google_crashdump_uploader.cc
File google_crashdump_uploader.cc (right):

https://breakpad.appspot.com/7714002/diff/30002/google_crashdump_uploader.cc#...
google_crashdump_uploader.cc:166: return Upload(&server_response);
Since the original behavior of Upload() is to not get the server_response back,
why not just pass in NULL here? (That is, what's the purpose of passing in a
pointer to a local variable whose value is never used?)

[gunsch]

GoogleCrashdumpUploader: adds Upload(string*) API to get the HTTP response.

[gunsch]

On 2014/09/17 22:19:28, gunsch wrote:
> GoogleCrashdumpUploader: adds Upload(string*) API to get the HTTP response.

I guess that should be "PTAL", still finding my way around the gcl tool :) 

Added the extra return information, required some plumbing through
LibcurlWrapper.

[Mark Mentovai]

https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/google_crash...
File src/common/linux/google_crashdump_uploader.h (right):

https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/google_crash...
src/common/linux/google_crashdump_uploader.h:82: bool Upload();
Is this even needed anymore? Don’t we control all of the callers? It’s easy
enough for callers that don’t care about this stuff to pass NULL, so I think we
should dump the old obsolete parameter-less form.

https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/libcurl_wrap...
File src/common/linux/libcurl_wrapper.cc (right):

https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/libcurl_wrap...
src/common/linux/libcurl_wrapper.cc:125: if (http_response_data != NULL) {
Should probably do http_header_data->clear() here.

https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/libcurl_wrap...
src/common/linux/libcurl_wrapper.cc:131: (*easy_setopt_)(curl_,
CURLOPT_HEADERFUNCTION, WriteCallback);
Here too. The alternative is to have a WriteCallback() variant that does
assign() instead of append(). It seems that CURLOPT_WRITEFUNCTION will be called
potentially multiple times, but CURLOPT_HEADERFUNCTION will only be called once.

[gunsch]

https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/google_crash...
File src/common/linux/google_crashdump_uploader.h (right):

https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/google_crash...
src/common/linux/google_crashdump_uploader.h:82: bool Upload();
On 2014/09/17 22:47:29, Mark Mentovai wrote:
> Is this even needed anymore? Don’t we control all of the callers? It’s easy
> enough for callers that don’t care about this stuff to pass NULL, so I think
we
> should dump the old obsolete parameter-less form.

Done. Was concerned about bridging with callers in Chromium but it looks like
there aren't any.

I'm starting to wonder about the purpose of this class, actually---does anyone
use it? As far as I can see, client/linux/sender/google_crash_report_sender.cc
is the only caller (including in Chromium), nothing else references
GoogleCrashdumpUploader, and google_crash_report_sender.cc isn't even referenced
in any build files (last reference deleted in
http://breakpad.appspot.com/225001). Am I missing something?

https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/libcurl_wrap...
File src/common/linux/libcurl_wrapper.cc (right):

https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/libcurl_wrap...
src/common/linux/libcurl_wrapper.cc:125: if (http_response_data != NULL) {
On 2014/09/17 22:47:29, Mark Mentovai wrote:
> Should probably do http_header_data->clear() here.

Done.

https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/libcurl_wrap...
src/common/linux/libcurl_wrapper.cc:131: (*easy_setopt_)(curl_,
CURLOPT_HEADERFUNCTION, WriteCallback);
On 2014/09/17 22:47:29, Mark Mentovai wrote:
> Here too. The alternative is to have a WriteCallback() variant that does
> assign() instead of append(). It seems that CURLOPT_WRITEFUNCTION will be
called
> potentially multiple times, but CURLOPT_HEADERFUNCTION will only be called
once.

Done.

[lcwu]

On 2014/09/17 23:38:10, gunsch wrote:
>
https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/google_crash...
> File src/common/linux/google_crashdump_uploader.h (right):
> 
>
https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/google_crash...
> src/common/linux/google_crashdump_uploader.h:82: bool Upload();
> On 2014/09/17 22:47:29, Mark Mentovai wrote:
> > Is this even needed anymore? Don’t we control all of the callers? It’s easy
> > enough for callers that don’t care about this stuff to pass NULL, so I think
> we
> > should dump the old obsolete parameter-less form.
> 
> Done. Was concerned about bridging with callers in Chromium but it looks like
> there aren't any.
> 
> I'm starting to wonder about the purpose of this class, actually---does anyone
> use it? As far as I can see, client/linux/sender/google_crash_report_sender.cc
> is the only caller (including in Chromium), nothing else references
> GoogleCrashdumpUploader, and google_crash_report_sender.cc isn't even
referenced
> in any build files (last reference deleted in
> http://breakpad.appspot.com/225001). Am I missing something?

I don't think you are missing anything. According to Gaofeng, Chromium stops to
use google_crashdump_uploader. Gaofeng can correct me if I was wrong.

> 
>
https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/libcurl_wrap...
> File src/common/linux/libcurl_wrapper.cc (right):
> 
>
https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/libcurl_wrap...
> src/common/linux/libcurl_wrapper.cc:125: if (http_response_data != NULL) {
> On 2014/09/17 22:47:29, Mark Mentovai wrote:
> > Should probably do http_header_data->clear() here.
> 
> Done.
> 
>
https://breakpad.appspot.com/7714002/diff/80001/src/common/linux/libcurl_wrap...
> src/common/linux/libcurl_wrapper.cc:131: (*easy_setopt_)(curl_,
> CURLOPT_HEADERFUNCTION, WriteCallback);
> On 2014/09/17 22:47:29, Mark Mentovai wrote:
> > Here too. The alternative is to have a WriteCallback() variant that does
> > assign() instead of append(). It seems that CURLOPT_WRITEFUNCTION will be
> called
> > potentially multiple times, but CURLOPT_HEADERFUNCTION will only be called
> once.
> 
> Done.

[Mark Mentovai]

LGTM