[iOS] Upstream some stability code

[iOS] Upstream some stability code

Committed: https://crrev.com/b548bee8a122307ac0e48bee2425356c7a9b59b7
Cr-Commit-Position: refs/heads/master@{#337822}

[droger, 2015-07-08 11:41:45]

droger@chromium.org changed reviewers:
+ lpromero@chromium.org

[droger, 2015-07-08 11:42:15]

Patchset #1 (id:1) has been deleted

[lpromero, 2015-07-08 13:37:16]

LGTM

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_report/breakpad_helper.h (right):

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/breakpad_helper.h:18: // Enable/Disable crash
handling.
Enables/Disables. Here and in several methods below.

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_report/crash_report_multi_parameter.mm (right):

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_report_multi_parameter.mm:1: // Copyright
2014 The Chromium Authors. All rights reserved.
I don't know the rules when upstreaming. Should we update the year?

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider.mm (right):

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider.mm:90: sharedInstance]
didSeeMemoryWarningShortlyBeforeTerminating];
git cl format messed this one up.

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider_unittest.mm
(right):

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider_unittest.mm:106:
SHUTDOWN_IN_FOREGROUND_NO_CRASH_LOG_NO_MEMORY_WARNING,
Shouldn't this be indented 2 instead of 4?

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/ios_chrome.gyp
File ios/chrome/ios_chrome.gyp (right):

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/ios_chrome.g...
ios/chrome/ios_chrome.gyp:241:
'browser/metrics/ios_stability_metrics_provider.h',
Sort.

[droger, 2015-07-08 14:04:37]

Thanks for the review.

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_report/crash_report_multi_parameter.mm (right):

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_report_multi_parameter.mm:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/07/08 13:37:16, lpromero wrote:
> I don't know the rules when upstreaming. Should we update the year?

The policy is to keep the original year when upstreaming files.

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider_unittest.mm
(right):

https://codereview.chromium.org/1207353005/diff/20001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider_unittest.mm:106:
SHUTDOWN_IN_FOREGROUND_NO_CRASH_LOG_NO_MEMORY_WARNING,
On 2015/07/08 13:37:16, lpromero wrote:
> Shouldn't this be indented 2 instead of 4?

I don't know, I don't see anything in the style guide. I would assume that the
tool is correct here.

[droger, 2015-07-08 14:04:41]

The CQ bit was checked by droger@chromium.org

[droger, 2015-07-08 14:04:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/1207353005/#ps40001
(title: "Review comments")

[commit-bot: I haz the power, 2015-07-08 14:04:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207353005/40001

[commit-bot: I haz the power, 2015-07-08 14:04:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-08 14:04:57]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[droger, 2015-07-08 14:05:35]

droger@chromium.org changed reviewers:
+ sdefresne@chromium.org

[droger, 2015-07-08 14:05:35]

sdefresne: can you give the superstar committer approval?

[sdefresne, 2015-07-08 14:18:34]

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_report/breakpad_helper.mm (right):

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/breakpad_helper.mm:23: // TODO(ios): Move this
up where it belongs once
nit: can you find a better attribution than "ios"? maybe stuartmorgan since he
reported the breakpad issue.

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_report/crash_report_multi_parameter.h (right):

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_report_multi_parameter.h:9: 
#include "base/logging.h"

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_report_multi_parameter.h:15: -
(id)initWithKey:(NSString*)key;
id -> instancetype and maybe NS_DESIGNATED_INITIALIZER

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_report_multi_parameter.h:16: 
- (instancetype)init NS_UNAVAILABLE;

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_report/crash_report_multi_parameter.mm (right):

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_report_multi_parameter.mm:22: @private
@private is not required since it is in the implementation file.

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_report_multi_parameter.mm:27: - (id)init {
id -> instancetype

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_report_multi_parameter.mm:32: -
(id)initWithKey:(NSString*)key {
id -> instancetype

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_report_multi_parameter.mm:42:
dictionary_->Remove([key UTF8String], NULL);
Shouldn't all those [... UTF8String] be SysNSStringToUTF8(...).c_str()?

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
File ios/chrome/browser/crash_report/crash_report_user_application_state.mm
(right):

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/cras...
ios/chrome/browser/crash_report/crash_report_user_application_state.mm:19: -
(id)init {
id -> instancetype

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider_unittest.mm
(right):

https://codereview.chromium.org/1207353005/diff/40001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider_unittest.mm:23: }
}  // namespace

Moreover in the same CL you mix namespace without blank lines (here) and ones
with blank lines (ios/chrome/browser/metrics/ios_stability_metrics_provider.mm).
Can you harmonize?

[droger, 2015-07-08 15:03:46]

done, ptal.

[sdefresne, 2015-07-08 15:20:06]

lgtm

[droger, 2015-07-08 15:24:24]

The CQ bit was checked by droger@chromium.org

[droger, 2015-07-08 15:24:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/1207353005/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-07-08 15:24:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207353005/60001

[commit-bot: I haz the power, 2015-07-08 15:29:37]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-07-08 15:30:26]

Patchset 3 (id:??) landed as
https://crrev.com/b548bee8a122307ac0e48bee2425356c7a9b59b7
Cr-Commit-Position: refs/heads/master@{#337822}