Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1138703004:
[iOS] Upstream CrashReportBackgroundUploader (Closed)
Description[iOS] Upstream CrashReportBackgroundUploader
CrashReportBackgroundUploader handle uploading crashes in background using
iOS 7 capability of periodically waking up the application in background.
BUG=429756
Committed: https://crrev.com/ae76d4258f4dbb158cd85997ce6ccfa823a6fa42
Cr-Commit-Position: refs/heads/master@{#329643}
Patch Set 1 #Patch Set 2 : Add (nonatomic, assign) to hasPendingCrashReportsToUploadAtStartup property as discussed offline #Patch Set 3 : Update tools/metrics/actions/actions.xml #
Total comments: 4
Patch Set 4 : Update tools/metrics/histograms/histograms.xml #
Total comments: 2
Patch Set 5 : Update tools/metrics/histograms/histograms.xml #
Total comments: 2
Messages
Total messages: 37 (16 generated)
sdefresne@chromium.org changed reviewers: + jif@chromium.org, mark@chromium.org
sdefresne@chromium.org changed required reviewers: + jif@chromium.org, mark@chromium.org
jif: please review mark: for added DEPS on //breakpad olivierrobin: FYI as original implementor
lgtm
On 2015/05/11 14:49:11, jif wrote: > lgtm LGTM. In description: CrashReportBackgroundUploader handle uploading crashes in background using iOS 7 capability of periodically waking up the application in background.
On 2015/05/11 15:00:57, Olivier Robin wrote: > On 2015/05/11 14:49:11, jif wrote: > > lgtm > > LGTM. > > In description: > CrashReportBackgroundUploader handle uploading crashes in background using > iOS 7 capability of periodically waking up the application in background. Done.
sdefresne@chromium.org changed reviewers: + thestig@chromium.org - mark@chromium.org
sdefresne@chromium.org changed required reviewers: + thestig@chromium.org - mark@chromium.org
On 2015/05/11 15:47:31, sdefresne wrote: > On 2015/05/11 15:00:57, Olivier Robin wrote: > > On 2015/05/11 14:49:11, jif wrote: > > > lgtm > > > > LGTM. > > > > In description: > > CrashReportBackgroundUploader handle uploading crashes in background using > > iOS 7 capability of periodically waking up the application in background. > > Done. thestig: for added DEPS on //breakpad
lgtm
Dependent CL https://codereview.chromium.org/1131303003/ landed, sending to CQ. Thank you all for the review.
The CQ bit was checked by sdefresne@chromium.org
The CQ bit was unchecked by sdefresne@chromium.org
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, jif@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/1138703004/#ps20001 (title: "Add (nonatomic, assign) to hasPendingCrashReportsToUploadAtStartup property as discussed offline")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138703004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sdefresne@chromium.org changed reviewers: + isherman@chromium.org
sdefresne@chromium.org changed required reviewers: + isherman@chromium.org
+isherman for tools/metrics/actions/actions.xml
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1138703004/diff/40001/ios/chrome/browser/cras... File ios/chrome/browser/crash_report/crash_report_background_uploader.mm (right): https://codereview.chromium.org/1138703004/diff/40001/ios/chrome/browser/cras... ios/chrome/browser/crash_report/crash_report_background_uploader.mm:297: UMA_HISTOGRAM_COUNTS_100(kUMAMobilePendingReportsOnBackgroundWakeUp, If this code is being upstreamed, can you move the corresponding histograms.xml entry to the open source file as well? https://codereview.chromium.org/1138703004/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1138703004/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:1066: <action name="BackgroundUploadReportAborted"> Is this a new action being added or was it in the code for a while now, but never in an actions.xml file?
On 2015/05/13 14:33:53, Alexei Svitkine wrote: > https://codereview.chromium.org/1138703004/diff/40001/ios/chrome/browser/cras... > File ios/chrome/browser/crash_report/crash_report_background_uploader.mm > (right): > > https://codereview.chromium.org/1138703004/diff/40001/ios/chrome/browser/cras... > ios/chrome/browser/crash_report/crash_report_background_uploader.mm:297: > UMA_HISTOGRAM_COUNTS_100(kUMAMobilePendingReportsOnBackgroundWakeUp, > If this code is being upstreamed, can you move the corresponding histograms.xml > entry to the open source file as well? Good catch, will do. > https://codereview.chromium.org/1138703004/diff/40001/tools/metrics/actions/a... > File tools/metrics/actions/actions.xml (right): > > https://codereview.chromium.org/1138703004/diff/40001/tools/metrics/actions/a... > tools/metrics/actions/actions.xml:1066: <action > name="BackgroundUploadReportAborted"> > Is this a new action being added or was it in the code for a while now, but > never in an actions.xml file? It is not a new action, it's been for some time in the downstream fork of iOS and the action was in the private version of actions.xml.
asvitkine: please take another look https://codereview.chromium.org/1138703004/diff/40001/ios/chrome/browser/cras... File ios/chrome/browser/crash_report/crash_report_background_uploader.mm (right): https://codereview.chromium.org/1138703004/diff/40001/ios/chrome/browser/cras... ios/chrome/browser/crash_report/crash_report_background_uploader.mm:297: UMA_HISTOGRAM_COUNTS_100(kUMAMobilePendingReportsOnBackgroundWakeUp, On 2015/05/13 14:33:52, Alexei Svitkine wrote: > If this code is being upstreamed, can you move the corresponding histograms.xml > entry to the open source file as well? Sure, done in next patchset. https://codereview.chromium.org/1138703004/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1138703004/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:1066: <action name="BackgroundUploadReportAborted"> On 2015/05/13 14:33:53, Alexei Svitkine wrote: > Is this a new action being added or was it in the code for a while now, but > never in an actions.xml file? The action was in the downstream code and the action declared in the private version of actions.xml. This CL move the code reporting the action to chromium upstream repository and move the action declaration from the private actions.xml to the public one.
https://codereview.chromium.org/1138703004/diff/20002/ios/chrome/browser/cras... File ios/chrome/browser/crash_report/crash_report_background_uploader.mm (right): https://codereview.chromium.org/1138703004/diff/20002/ios/chrome/browser/cras... ios/chrome/browser/crash_report/crash_report_background_uploader.mm:306: UMA_HISTOGRAM_LONG_TIMES_100(kUMAMobileCrashBackgroundUploadDelay, This one too please.
D'oh. This should be the last, sorry. https://codereview.chromium.org/1138703004/diff/20002/ios/chrome/browser/cras... File ios/chrome/browser/crash_report/crash_report_background_uploader.mm (right): https://codereview.chromium.org/1138703004/diff/20002/ios/chrome/browser/cras... ios/chrome/browser/crash_report/crash_report_background_uploader.mm:306: UMA_HISTOGRAM_LONG_TIMES_100(kUMAMobileCrashBackgroundUploadDelay, On 2015/05/13 14:56:19, Alexei Svitkine wrote: > This one too please. Done.
PTAL
sdefresne@chromium.org changed required reviewers: - isherman@chromium.org
lgtm https://codereview.chromium.org/1138703004/diff/70001/ios/chrome/browser/cras... File ios/chrome/browser/crash_report/crash_report_background_uploader.h (right): https://codereview.chromium.org/1138703004/diff/70001/ios/chrome/browser/cras... ios/chrome/browser/crash_report/crash_report_background_uploader.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Nit: 2015 - fix throughout
Thank you for the review. https://codereview.chromium.org/1138703004/diff/70001/ios/chrome/browser/cras... File ios/chrome/browser/crash_report/crash_report_background_uploader.h (right): https://codereview.chromium.org/1138703004/diff/70001/ios/chrome/browser/cras... ios/chrome/browser/crash_report/crash_report_background_uploader.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/05/13 15:41:08, Alexei Svitkine wrote: > Nit: 2015 - fix throughout This file is upstreamed from Chrome on iOS private repository. As a general rule we do not update the copyright date when upstreaming.
sdefresne@chromium.org changed reviewers: - isherman@chromium.org
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, jif@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/1138703004/#ps70001 (title: "Update tools/metrics/histograms/histograms.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138703004/70001
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ae76d4258f4dbb158cd85997ce6ccfa823a6fa42 Cr-Commit-Position: refs/heads/master@{#329643} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
