Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817263002:
Remove usage of Tab's |url| property from CrashReportHelper. (Closed)
|
Created:
3 years, 8 months ago by kkhorimoto Modified:
3 years, 6 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove usage of Tab's |url| property from CrashReportHelper.
BUG=546208
Review-Url: https://codereview.chromium.org/2817263002
Cr-Commit-Position: refs/heads/master@{#476070}
Committed: https://chromium.googlesource.com/chromium/src/+/440490dec42a4f251c498e9af7f9f442945a0720
Patch Set 1 #
Total comments: 4
Patch Set 2 : use pending URL if necessary #
Total comments: 4
Patch Set 3 : fix compile, avoid BOOL pitfalls #Messages
Total messages: 17 (9 generated)
[email protected] changed reviewers: + [email protected], [email protected], [email protected]
https://codereview.chromium.org/2817263002/diff/1/ios/chrome/browser/crash_re... File ios/chrome/browser/crash_report/crash_report_helper.mm (right): https://codereview.chromium.org/2817263002/diff/1/ios/chrome/browser/crash_re... ios/chrome/browser/crash_report/crash_report_helper.mm:218: [self recordURL:base::SysUTF8ToNSString(newTab.lastCommittedURL.spec()) Not sure whether the crash reporter should be sending the last committed or the visible URL.
https://codereview.chromium.org/2817263002/diff/1/ios/chrome/browser/crash_re... File ios/chrome/browser/crash_report/crash_report_helper.mm (right): https://codereview.chromium.org/2817263002/diff/1/ios/chrome/browser/crash_re... ios/chrome/browser/crash_report/crash_report_helper.mm:218: [self recordURL:base::SysUTF8ToNSString(newTab.lastCommittedURL.spec()) On 2017/04/14 20:26:11, kkhorimoto_ wrote: > Not sure whether the crash reporter should be sending the last committed or the > visible URL. I believe pending URL (or last committed of pending does not exist) could be more useful here. This will preserve the original behavior. We should not use visibleURL however.
https://codereview.chromium.org/2817263002/diff/1/ios/chrome/browser/crash_re... File ios/chrome/browser/crash_report/crash_report_helper.mm (right): https://codereview.chromium.org/2817263002/diff/1/ios/chrome/browser/crash_re... ios/chrome/browser/crash_report/crash_report_helper.mm:218: [self recordURL:base::SysUTF8ToNSString(newTab.lastCommittedURL.spec()) On 2017/04/14 21:34:11, Eugene But wrote: > On 2017/04/14 20:26:11, kkhorimoto_ wrote: > > Not sure whether the crash reporter should be sending the last committed or > the > > visible URL. > I believe pending URL (or last committed of pending does not exist) could be > more useful here. This will preserve the original behavior. We should not use > visibleURL however. I'll second the use of pending URL. If we're trying to load a new page, that's more likely to contribute to a crash than the old page.
The CQ bit was checked by [email protected] to run a CQ dry run
https://codereview.chromium.org/2817263002/diff/1/ios/chrome/browser/crash_re... File ios/chrome/browser/crash_report/crash_report_helper.mm (right): https://codereview.chromium.org/2817263002/diff/1/ios/chrome/browser/crash_re... ios/chrome/browser/crash_report/crash_report_helper.mm:218: [self recordURL:base::SysUTF8ToNSString(newTab.lastCommittedURL.spec()) On 2017/05/09 15:12:40, rohitrao (ping after 24h) wrote: > On 2017/04/14 21:34:11, Eugene But wrote: > > On 2017/04/14 20:26:11, kkhorimoto_ wrote: > > > Not sure whether the crash reporter should be sending the last committed or > > the > > > visible URL. > > I believe pending URL (or last committed of pending does not exist) could be > > more useful here. This will preserve the original behavior. We should not use > > visibleURL however. > > I'll second the use of pending URL. If we're trying to load a new page, that's > more likely to contribute to a crash than the old page. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
lgtm! https://codereview.chromium.org/2817263002/diff/20001/ios/chrome/browser/cras... File ios/chrome/browser/crash_report/crash_report_helper.mm (right): https://codereview.chromium.org/2817263002/diff/20001/ios/chrome/browser/cras... ios/chrome/browser/crash_report/crash_report_helper.mm:221: pendingItem ? pendingItem->GetURL() : newTab.lastcommittedURL; Not related to your code, but |lastCommittedURL| :( Not sure if we care about fixing this however. https://codereview.chromium.org/2817263002/diff/20001/ios/chrome/browser/cras... ios/chrome/browser/crash_report/crash_report_helper.mm:224: pending:pendingItem]; pendingItem ? YES : NO https://google.github.io/styleguide/objcguide.xml#BOOL_Pitfalls
https://codereview.chromium.org/2817263002/diff/20001/ios/chrome/browser/cras... File ios/chrome/browser/crash_report/crash_report_helper.mm (right): https://codereview.chromium.org/2817263002/diff/20001/ios/chrome/browser/cras... ios/chrome/browser/crash_report/crash_report_helper.mm:221: pendingItem ? pendingItem->GetURL() : newTab.lastcommittedURL; On 2017/05/31 22:14:34, Eugene But wrote: > Not related to your code, but |lastCommittedURL| :( Not sure if we care about > fixing this however. Done (it was a typo; the property is correct) https://codereview.chromium.org/2817263002/diff/20001/ios/chrome/browser/cras... ios/chrome/browser/crash_report/crash_report_helper.mm:224: pending:pendingItem]; On 2017/05/31 22:14:34, Eugene But wrote: > pendingItem ? YES : NO > > https://google.github.io/styleguide/objcguide.xml#BOOL_Pitfalls Done.
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected] Link to the patchset: https://codereview.chromium.org/2817263002/#ps40001 (title: "fix compile, avoid BOOL pitfalls")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496269341551090,
"parent_rev": "7d2dd6afb69f065963451f88b0166d02279070f8", "commit_rev":
"440490dec42a4f251c498e9af7f9f442945a0720"}
Message was sent while issue was closed.
Description was changed from ========== Remove usage of Tab's |url| property from CrashReportHelper. BUG=546208 ========== to ========== Remove usage of Tab's |url| property from CrashReportHelper. BUG=546208 Review-Url: https://codereview.chromium.org/2817263002 Cr-Commit-Position: refs/heads/master@{#476070} Committed: https://chromium.googlesource.com/chromium/src/+/440490dec42a4f251c498e9af7f9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/440490dec42a4f251c498e9af7f9... |
