Add high level API for breakpad on iOS.

Add high level API for breakpad on iOS.

 The new API allows to automatically upload repports to the crash server when
the application restarts.

 This change also:
 - Correct a bug on the test for correct alignment of the abrt signal handler
 - Add user friendly information on crashes for SIGABRT and NSException
Committed: https://code.google.com/p/google-breakpad/source/detail?r=935

[qsr]

Ping?

[Mark Mentovai]

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
File src/client/ios/BreakpadController.h (right):

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:29: 
This file needs #include guards.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:30: #include "client/ios/Breakpad.h"
The general rule (in Chromium) is to #import Objective-C headers instead of
using #include. This one is an Objective-C header.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:33: // configure it, ensure thread-safety,
and send crash reports back to the
does configure -> configures

ensure -> ensures

send -> sends

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:36: @interface BreakpadController : NSObject
{
You should #import <Foundation/Foundation.h> instead of relying on getting it
from Breakpad.h.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:45: // The dictionnary that contains
configuration for breakpad. Modifying it
Spelling: dictionary

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:61: // Update the controller configuration.
It merge its old configuration with the
Merges, not merge.

This should explain how the merge is done. Do new values simply replace old
values in the configuration? Or if the values are dictionaries or arrays, is the
replacement a dictionary merge or array append?

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:65: // Configure the URL to upload the
report to. This must be called at least one
one -> once

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:69: // Set the minimal interval between 2
uploads. This must be called at least one
one -> once

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:69: // Set the minimal interval between 2
uploads. This must be called at least one
You don’t say what unit the interval is expressed in.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:69: // Set the minimal interval between 2
uploads. This must be called at least one
2 -> two

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:73: // Specify parameter that will be
uploaded to the crash server. See
-> Specify a parameter

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:86: // Starts the BreakpadController by
registering the Signal Handlers.
signal handlers is not a proper name, so it does not need Capital Letters.

This is more than just a signal handler, though. It’s also a Mach exception
handler.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:91: // Unregisters the Signal Handlers.
Same.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
File src/client/ios/BreakpadController.mm (right):

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:45: // Returns when a report can be sent.
|-1| means never, |0| means that a report
Blank line before this. Also a blank line after sendDelay. Otherwise, I can’t
find anything in here.

I’d add blanks around the @interface and the @end too.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:63: NSString* platform() {
GetPlatform?

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:67: sysctlbyname("hw.machine", NULL, &size,
NULL, 0);
Early return (nil) if the sysctl fails or returns size 0.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:68: char* machine = new char[size];
scoped_array?

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:69: if (sysctlbyname("hw.machine", machine,
&size, NULL, 0) == 0)
You should have a const char[] for "hw.machine".

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:85: if (!sharedInstance_) {
Why don’t you just write

static BreakpadController* sharedInstance_ = [[BreakpadController alloc] init];
return sharedInstance_;

right here?

You can even get away without the @synchronized as long as everyone that
compiles this code builds it with thread-safe statics, which are the default.
(Overridden with -fno-threadsafe-statics.)

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:92: - (id)init {
Why don’t you enforce singleton-ness by making init return nil, and having an
initSingleton method, private to this file?

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:100: uploadInterval_ = [uploadInterval
intValue];
I see this code repeated three times throughout this file.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:107: // Since this class is a Singleton we
don't expect this method to be
singleton -> lowercase s. It’s not a proper name.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:154: object:nil];
Line up the colons as you’ve done elsewhere?

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:205: if (!breakpadRef_ || uploadInterval_
<= 0 || !enableUploads_)
Is uploadInterval_ not supposed to be allowed to be 0? I didn’t see that
documented anywhere.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:208: // We do not want to DOS the crash
server. We do not send more than one
Avoid using “we” (and “us”) in comments. Kinda throughout this file.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:214: NSTimeInterval spanSeconds =
(CFAbsoluteTimeGetCurrent() - lastTime);
No need for parentheses.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:244: // If more report must be sent, make
sure this method is called again.
report -> reports

[qsr]

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
File src/client/ios/BreakpadController.h (right):

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:29: 
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> This file needs #include guards.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:30: #include "client/ios/Breakpad.h"
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> The general rule (in Chromium) is to #import Objective-C headers instead of
> using #include. This one is an Objective-C header.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:33: // configure it, ensure thread-safety,
and send crash reports back to the
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> does configure -> configures
> 
> ensure -> ensures
> 
> send -> sends

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:36: @interface BreakpadController : NSObject
{
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> You should #import <Foundation/Foundation.h> instead of relying on getting it
> from Breakpad.h.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:45: // The dictionnary that contains
configuration for breakpad. Modifying it
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> Spelling: dictionary

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:61: // Update the controller configuration.
It merge its old configuration with the
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> Merges, not merge.
> 
> This should explain how the merge is done. Do new values simply replace old
> values in the configuration? Or if the values are dictionaries or arrays, is
the
> replacement a dictionary merge or array append?

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:65: // Configure the URL to upload the
report to. This must be called at least one
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> one -> once

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:69: // Set the minimal interval between 2
uploads. This must be called at least one
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> one -> once

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:69: // Set the minimal interval between 2
uploads. This must be called at least one
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> You don’t say what unit the interval is expressed in.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:69: // Set the minimal interval between 2
uploads. This must be called at least one
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> 2 -> two

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:73: // Specify parameter that will be
uploaded to the crash server. See
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> -> Specify a parameter

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:86: // Starts the BreakpadController by
registering the Signal Handlers.
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> signal handlers is not a proper name, so it does not need Capital Letters.
> 
> This is more than just a signal handler, though. It’s also a Mach exception
> handler.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:91: // Unregisters the Signal Handlers.
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> Same.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
File src/client/ios/BreakpadController.mm (right):

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:45: // Returns when a report can be sent.
|-1| means never, |0| means that a report
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> Blank line before this. Also a blank line after sendDelay. Otherwise, I can’t
> find anything in here.
> 
> I’d add blanks around the @interface and the @end too.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:63: NSString* platform() {
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> GetPlatform?

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:67: sysctlbyname("hw.machine", NULL, &size,
NULL, 0);
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> Early return (nil) if the sysctl fails or returns size 0.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:68: char* machine = new char[size];
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> scoped_array?

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:69: if (sysctlbyname("hw.machine", machine,
&size, NULL, 0) == 0)
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> You should have a const char[] for "hw.machine".

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:85: if (!sharedInstance_) {
 Done, but letting the synchronization, just to be on the safe side. I don't
expect this code to be in any critical section anytime soon.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:92: - (id)init {
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> Why don’t you enforce singleton-ness by making init return nil, and having an
> initSingleton method, private to this file?

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:100: uploadInterval_ = [uploadInterval
intValue];
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> I see this code repeated three times throughout this file.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:107: // Since this class is a Singleton we
don't expect this method to be
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> singleton -> lowercase s. It’s not a proper name.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:154: object:nil];
 Not sure to understand. Trying to lineup with 
cancelPreviousPerformRequestsWithTarget:self will make the second line more than
80 characters. Do you want me to align just the second and third line?

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:205: if (!breakpadRef_ || uploadInterval_
<= 0 || !enableUploads_)
 Added documentation.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:208: // We do not want to DOS the crash
server. We do not send more than one
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> Avoid using “we” (and “us”) in comments. Kinda throughout this file.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:214: NSTimeInterval spanSeconds =
(CFAbsoluteTimeGetCurrent() - lastTime);
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> No need for parentheses.

Done.

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:244: // If more report must be sent, make
sure this method is called again.
On 2012/03/13 17:55:22, Mark Mentovai wrote:
> report -> reports

Done.

[Mark Mentovai]

LGTM otherwise

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
File src/client/ios/BreakpadController.mm (right):

https://breakpad.appspot.com/361001/diff/2001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:154: object:nil];
qsr wrote:
>  Not sure to understand. Trying to lineup with 
> cancelPreviousPerformRequestsWithTarget:self will make the second line more
than
> 80 characters. Do you want me to align just the second and third line?

Nah, leave it as-is.

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
File src/client/ios/BreakpadController.h (right):

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:1: // Copyright (c) 2011, Google Inc.
2012

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:58: // The interval to wait between 2
uploads. Value is 0 if no upload must be
2 -> two

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
File src/client/ios/BreakpadController.mm (right):

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:1: // Copyright (c) 2011, Google Inc.
2012

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:66: namespace {
Blank line after this…

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:72: const char kHwMachineSysctlName[] =
"hw.machine";
Since this is only used inside GetPlatform, only declare it inside GetPlatform.

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:79: if (sysctlbyname("hw.machine", NULL,
&size, NULL, 0) || size == 0)
You didn’t use the const char[] here!

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:86: }  // namespace
…and before this.

[qsr]

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
File src/client/ios/BreakpadController.h (right):

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:1: // Copyright (c) 2011, Google Inc.
On 2012/03/13 19:42:12, Mark Mentovai wrote:
> 2012

Done.

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.h:58: // The interval to wait between 2
uploads. Value is 0 if no upload must be
On 2012/03/13 19:42:12, Mark Mentovai wrote:
> 2 -> two

Done.

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
File src/client/ios/BreakpadController.mm (right):

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:1: // Copyright (c) 2011, Google Inc.
On 2012/03/13 19:42:12, Mark Mentovai wrote:
> 2012

Done.

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:66: namespace {
On 2012/03/13 19:42:12, Mark Mentovai wrote:
> Blank line after this…

Done.

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:72: const char kHwMachineSysctlName[] =
"hw.machine";
On 2012/03/13 19:42:12, Mark Mentovai wrote:
> Since this is only used inside GetPlatform, only declare it inside
GetPlatform.

Done.

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:79: if (sysctlbyname("hw.machine", NULL,
&size, NULL, 0) || size == 0)
On 2012/03/13 19:42:12, Mark Mentovai wrote:
> You didn’t use the const char[] here!

Done.

https://breakpad.appspot.com/361001/diff/4001/src/client/ios/BreakpadControll...
src/client/ios/BreakpadController.mm:86: }  // namespace
On 2012/03/13 19:42:12, Mark Mentovai wrote:
> …and before this.

Done.