Add an API to Breakpad to upload custom file to the crash server.

Add an API to Breakpad to upload custom file to the crash server.

 On iOS, sending logs using the usual breakpad behavior is not possible,
because
tar is not available. This allow to use Breakpad to send any file to the crash
server.

R=mark@chromium.org

[Mark Mentovai]

http://breakpad.appspot.com/327001/diff/2001/1006
File src/client/ios/Breakpad.mm (right):

http://breakpad.appspot.com/327001/diff/2001/1006#newcode430
Line 430: void Breakpad::UploadData(NSData *data, NSDictionary
*server_parameter) {
|server_parameter| should probably be named |server_parameters|.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode431
Line 431: NSMutableDictionary *parameters =
Maybe rename this so that it’s clear that it’s the NSDictionary form of
config_params_. I’m finding it confusing to read this function with three
different variables with some variant of “parameters” in their names.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode432
Line 432: [NSMutableDictionary dictionaryWithCapacity:0];
You might as well just say [NSMutableDictionary dictionary]. Then it’d all fit
on one line.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode435
Line 435: while (const KeyValueEntry *next = it.Next())
The multi-line loop body needs {braces}.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode439
Line 439: Uploader* uploader = [[[Uploader alloc] initWithConfig:parameters]
This is more readable if you put the line break after the =, as in

  Uploader* uploader =
      [[[Uploader alloc] initWithConfig:parameters] autorelease];

http://breakpad.appspot.com/327001/diff/2001/1006#newcode439
Line 439: Uploader* uploader = [[[Uploader alloc] initWithConfig:parameters]
Do you want the * on the type name (as here) or the variable name (as on lines
430, 431, 435, and 442)?

You should be consistent with the code around you. I know that this file uses a
mix of styles, so in the presence of conflicting guidance, you should at least
be consistent with yourself.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode441
Line 441: if (uploader) {
This conditional doesn’t appear to be necessary.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode442
Line 442: for(NSString *key in server_parameter) {
Style nit: put a space between |for| and |(|.

Reference:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...
(“Space after the keyword in conditions and loops.”)

http://breakpad.appspot.com/327001/diff/2001/1006#newcode715
Line 715: breakpad->UploadData(data, server_parameter);
Weird indent on this line (copied from 700 and 685.)

http://breakpad.appspot.com/327001/diff/2001/1006#newcode718
Line 718: fprintf(stderr, "BreakpadUploadNextReport() : error\n");
Not the right string to use for this error message.

http://breakpad.appspot.com/327001/diff/2001/1008
File src/client/mac/sender/uploader.m (right):

http://breakpad.appspot.com/327001/diff/2001/1008#newcode60
Line 60: - (BOOL)translateConfigurationData:(NSDictionary *)config;
The existing code is bad because these private functions don’t have comments
describing what they do. They’re grandfathered in, but your new function doesn’t
have the same leeway. Please provide a short blurb describing what this does,
because it’s unclear just from the declaration alone.

http://breakpad.appspot.com/327001/diff/2001/1008#newcode147
Line 147: NSUserDefaults *ud = [NSUserDefaults standardUserDefaults];
Should this line through line 158 be shared with 119 through 134 above?

http://breakpad.appspot.com/327001/diff/2001/1008#newcode155
Line 155: GTMLoggerDebug(@"uploader readConfigurationData failed");
Wrong string.

http://breakpad.appspot.com/327001/diff/2001/1008#newcode194
Line 194: - (BOOL)translateConfigurationData:(NSDictionary *)config {
This is obviously mostly a copy-paste job from -readConfigurationData. Why isn’t
there more sharing?

http://breakpad.appspot.com/327001/diff/2001/1008#newcode593
Line 593: - (void)uploadData:(NSData *)data {
Similarly, can this share with -report somehow?

[qsr]

Ok, this is finally quite a large refactoring...

http://breakpad.appspot.com/327001/diff/2001/1006
File src/client/ios/Breakpad.mm (right):

http://breakpad.appspot.com/327001/diff/2001/1006#newcode430
Line 430: void Breakpad::UploadData(NSData *data, NSDictionary
*server_parameter) {
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> |server_parameter| should probably be named |server_parameters|.

Done.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode431
Line 431: NSMutableDictionary *parameters =
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> Maybe rename this so that it’s clear that it’s the NSDictionary form of
> config_params_. I’m finding it confusing to read this function with three
> different variables with some variant of “parameters” in their names.

Done.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode432
Line 432: [NSMutableDictionary dictionaryWithCapacity:0];
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> You might as well just say [NSMutableDictionary dictionary]. Then it’d all fit
> on one line.

Done.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode435
Line 435: while (const KeyValueEntry *next = it.Next())
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> The multi-line loop body needs {braces}.

Done.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode439
Line 439: Uploader* uploader = [[[Uploader alloc] initWithConfig:parameters]
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> Do you want the * on the type name (as here) or the variable name (as on lines
> 430, 431, 435, and 442)?
> 
> You should be consistent with the code around you. I know that this file uses
a
> mix of styles, so in the presence of conflicting guidance, you should at least
> be consistent with yourself.

 Totally agree, this one slipped as the * on the variable that seems to be used
widely in breakpad is not the style I'm used to... :(

http://breakpad.appspot.com/327001/diff/2001/1006#newcode441
Line 441: if (uploader) {
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> This conditional doesn’t appear to be necessary.

Done.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode442
Line 442: for(NSString *key in server_parameter) {
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> Style nit: put a space between |for| and |(|.
> 
> Reference:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...
> (“Space after the keyword in conditions and loops.”)

Done.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode715
Line 715: breakpad->UploadData(data, server_parameter);
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> Weird indent on this line (copied from 700 and 685.)

Done.

http://breakpad.appspot.com/327001/diff/2001/1006#newcode718
Line 718: fprintf(stderr, "BreakpadUploadNextReport() : error\n");
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> Not the right string to use for this error message.

Done.

http://breakpad.appspot.com/327001/diff/2001/1008
File src/client/mac/sender/uploader.m (right):

http://breakpad.appspot.com/327001/diff/2001/1008#newcode60
Line 60: - (BOOL)translateConfigurationData:(NSDictionary *)config;
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> The existing code is bad because these private functions don’t have comments
> describing what they do. They’re grandfathered in, but your new function
doesn’t
> have the same leeway. Please provide a short blurb describing what this does,
> because it’s unclear just from the declaration alone.

Done.

http://breakpad.appspot.com/327001/diff/2001/1008#newcode147
Line 147: NSUserDefaults *ud = [NSUserDefaults standardUserDefaults];
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> Should this line through line 158 be shared with 119 through 134 above?

Done.

http://breakpad.appspot.com/327001/diff/2001/1008#newcode155
Line 155: GTMLoggerDebug(@"uploader readConfigurationData failed");
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> Wrong string.

Done.

http://breakpad.appspot.com/327001/diff/2001/1008#newcode194
Line 194: - (BOOL)translateConfigurationData:(NSDictionary *)config {
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> This is obviously mostly a copy-paste job from -readConfigurationData. Why
isn’t
> there more sharing?

Done.

http://breakpad.appspot.com/327001/diff/2001/1008#newcode593
Line 593: - (void)uploadData:(NSData *)data {
On 2011/11/16 14:20:28, Mark Mentovai wrote:
> Similarly, can this share with -report somehow?

Done.

[Mark Mentovai]

LGTM.

Nice refactoring. I don’t love the old code but I’m not going to hold you to
fixing it up.

http://breakpad.appspot.com/327001/diff/2003/6007
File src/client/mac/sender/uploader.mm (right):

http://breakpad.appspot.com/327001/diff/2003/6007#newcode141
Line 141: // Read the minidump referenced in |parameters_| and update
|minidumpContents_|
Blank line before.

http://breakpad.appspot.com/327001/diff/2003/6007#newcode144
Line 144: // Read the log files referenced in |parameters_| and update
|logFileData_|
Blank line before.

[thakis]

http://breakpad.appspot.com/327001/diff/9/4010
File src/client/mac/sender/uploader.mm (right):

http://breakpad.appspot.com/327001/diff/9/4010#newcode549
Line 549: [self uploadData:logFileData_ name:@"log" url:url];
You're passing a url: parameter here, but uploadData:name: doesn't take a url:
parameter. I'm fixing this in https://chromiumcodereview.appspot.com/9234054/ ;
shout if that's wrong somehow.