Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695923008:
Add a temporary crash key to debug nib loading issues. (Closed)
|
Created:
3 years, 10 months ago by Sidney San Martín Modified:
3 years, 10 months ago Reviewers:
Robert Sesek CC:
chromium-reviews, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a temporary crash key to debug nib loading issues.
BUG=685985
Review-Url: https://codereview.chromium.org/2695923008
Cr-Commit-Position: refs/heads/master@{#450969}
Committed: https://chromium.googlesource.com/chromium/src/+/1ee86ba6f52bb583d9d11b4e39b49fe4f4af32d5
Patch Set 1 #
Total comments: 6
Patch Set 2 : Un-remove this line #Patch Set 3 : Use stat() to test for the nib's existence and include error in the crash key #
Total comments: 8
Patch Set 4 : Add a missing include, de-auto, de-lambda. #Patch Set 5 : More accurate comment. #
Messages
Total messages: 35 (26 generated)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdy@chromium.org changed reviewers: + rsesek@chromium.org
Do you think this is the right info to capture?
The CQ bit was unchecked by sdy@chromium.org
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2695923008/diff/1/chrome/browser/ui/cocoa/too... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2695923008/diff/1/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:258: crash_keys::mac::kToolbarNibInfo, [&] { I don't see any advantage of a lambda here. Can you just write this straight-line? https://codereview.chromium.org/2695923008/diff/1/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:263: while (closestPath && ![fileManager fileExistsAtPath:closestPath]) { I would use actual stat on the nib file so that we can record errno. It would be useful to know the difference between ENOENT and EACCESS, for example. And then also record the closest path like you're doing here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2695923008/diff/1/chrome/browser/ui/cocoa/too... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2695923008/diff/1/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:258: crash_keys::mac::kToolbarNibInfo, [&] { On 2017/02/15 20:20:18, Robert Sesek wrote: > I don't see any advantage of a lambda here. Can you just write this > straight-line? Trying not to leak locals. What would you do instead?
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2695923008/diff/1/chrome/browser/ui/cocoa/too... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2695923008/diff/1/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:258: crash_keys::mac::kToolbarNibInfo, [&] { On 2017/02/15 22:45:19, sdy wrote: > On 2017/02/15 20:20:18, Robert Sesek wrote: > > I don't see any advantage of a lambda here. Can you just write this > > straight-line? > > Trying not to leak locals. What would you do instead? I didn't change this yet (because I'm trying to keep locals contained, but LMK if you'd still prefer straight line. https://codereview.chromium.org/2695923008/diff/1/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:263: while (closestPath && ![fileManager fileExistsAtPath:closestPath]) { On 2017/02/15 20:20:18, Robert Sesek wrote: > I would use actual stat on the nib file so that we can record errno. It would be > useful to know the difference between ENOENT and EACCESS, for example. And then > also record the closest path like you're doing here. Done, let me know if it makes sense.
https://codereview.chromium.org/2695923008/diff/1/chrome/browser/ui/cocoa/too... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2695923008/diff/1/chrome/browser/ui/cocoa/too... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:258: crash_keys::mac::kToolbarNibInfo, [&] { On 2017/02/15 23:15:01, sdy wrote: > On 2017/02/15 22:45:19, sdy wrote: > > On 2017/02/15 20:20:18, Robert Sesek wrote: > > > I don't see any advantage of a lambda here. Can you just write this > > > straight-line? > > > > Trying not to leak locals. What would you do instead? > > I didn't change this yet (because I'm trying to keep locals contained, but LMK > if you'd still prefer straight line. It's temporary code, so I don't feel strongly, but it's also temporary code so why do the locals matter? (also not sure about all this use of auto, but again, temporary code). https://codereview.chromium.org/2695923008/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2695923008/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:7: #include <algorithm> #include <sys/stat.h> https://codereview.chromium.org/2695923008/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:257: base::debug::ScopedCrashKey nib_crash_key{ naming: nibCrashKey https://codereview.chromium.org/2695923008/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:262: stat(nibPath.UTF8String, &sb); fileSystemRepresentation https://codereview.chromium.org/2695923008/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:263: auto nibErrno = errno; errno here may be something unrelated if stat returned 0 (success). nibErrno should default to 0 and you should only set if it stat fails. Also, I don't think this is an appropriate use of auto on this line.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2695923008/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/2695923008/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:7: #include <algorithm> On 2017/02/16 00:25:16, Robert Sesek wrote: > > #include <sys/stat.h> Done. https://codereview.chromium.org/2695923008/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:257: base::debug::ScopedCrashKey nib_crash_key{ On 2017/02/16 00:25:16, Robert Sesek wrote: > naming: nibCrashKey Done. https://codereview.chromium.org/2695923008/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:262: stat(nibPath.UTF8String, &sb); On 2017/02/16 00:25:16, Robert Sesek wrote: > fileSystemRepresentation Done. https://codereview.chromium.org/2695923008/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:263: auto nibErrno = errno; On 2017/02/16 00:25:16, Robert Sesek wrote: > errno here may be something unrelated if stat returned 0 (success). nibErrno > should default to 0 and you should only set if it stat fails. > > Also, I don't think this is an appropriate use of auto on this line. 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 sdy@chromium.org
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM
The CQ bit was checked by sdy@chromium.org
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": 80001, "attempt_start_ts": 1487257698499870,
"parent_rev": "dcfe7f207b2b188b50b3e25eef5765b662c36455", "commit_rev":
"1ee86ba6f52bb583d9d11b4e39b49fe4f4af32d5"}
Message was sent while issue was closed.
Description was changed from ========== Add a temporary crash key to debug nib loading issues. BUG=685985 ========== to ========== Add a temporary crash key to debug nib loading issues. BUG=685985 Review-Url: https://codereview.chromium.org/2695923008 Cr-Commit-Position: refs/heads/master@{#450969} Committed: https://chromium.googlesource.com/chromium/src/+/1ee86ba6f52bb583d9d11b4e39b4... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1ee86ba6f52bb583d9d11b4e39b4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
