Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2863983002:
DCHECK when loading images that do not exist. (Closed)
|
Created:
3 years, 7 months ago by jif Modified:
3 years, 6 months ago CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDCHECK when loading images that do not exist.
Enabled only in debug mode.
This works by swizzling [UIImage imageNamed:] and making sure that it returns
a non-nil UIImage*.
BUG=718850
Review-Url: https://codereview.chromium.org/2863983002
Cr-Commit-Position: refs/heads/master@{#470887}
Committed: https://chromium.googlesource.com/chromium/src/+/b75611794c4ee0538c0f62aaf75abaa409d8963d
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments. #Patch Set 3 : Ran downstream bots. Added more exceptions. Better logging. #
Total comments: 3
Messages
Total messages: 30 (17 generated)
Description was changed from ========== DCHECK when loading images that do not exist. BUG=718850 ========== to ========== DCHECK when loading images that do not exist. Enabled only in debug mode. This works by swizzling [UIImage imageNamed:] and making sure that it returns a non-nil UIImage*. BUG=718850 ==========
jif@chromium.org changed reviewers: + marq@chromium.org
Description was changed from ========== DCHECK when loading images that do not exist. Enabled only in debug mode. This works by swizzling [UIImage imageNamed:] and making sure that it returns a non-nil UIImage*. BUG=718850 ========== to ========== [WIP] DCHECK when loading images that do not exist. Enabled only in debug mode. This works by swizzling [UIImage imageNamed:] and making sure that it returns a non-nil UIImage*. BUG=718850 ==========
This is a WIP. I want to know if this idea sounds good before landing it.
lpromero@chromium.org changed reviewers: + lpromero@chromium.org
Looks good for the runtime approach. Could you run the trybots, EG bot, and maybe internal bots with this? https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... File ios/chrome/app/startup/setup_debugging.mm (right): https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... ios/chrome/app/startup/setup_debugging.mm:28: // The original implementation of [UIImage imageNamed:]. Super tiny nit: Add a dash: "of -[".
The CQ bit was checked by jif@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: This issue passed the CQ dry run.
Overall approach looks fine to me, I've added some style/clarity comments. https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... File ios/chrome/app/startup/setup_debugging.mm (right): https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... ios/chrome/app/startup/setup_debugging.mm:21: NSMutableSet* whiteList = [NSMutableSet set]; Add a comment that the whitelist will be retained by the block. https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... ios/chrome/app/startup/setup_debugging.mm:28: // The original implementation of [UIImage imageNamed:]. On 2017/05/05 15:24:37, lpromero wrote: > Super tiny nit: Add a dash: "of -[". I don't think that's needed, since imageNamed: is a class method. https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... ios/chrome/app/startup/setup_debugging.mm:36: Class klass = objc_getClass("UIImage"); Don't use the Java 'klass' naming convention. 'class' isn't an Obj-C reserved word, so just use that (or inline the getClass() call).
drive-by https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... File ios/chrome/app/startup/setup_debugging.mm (right): https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... ios/chrome/app/startup/setup_debugging.mm:28: // The original implementation of [UIImage imageNamed:]. On 2017/05/09 09:35:31, marq (OOO to May 9) wrote: > On 2017/05/05 15:24:37, lpromero wrote: > > Super tiny nit: Add a dash: "of -[". > > I don't think that's needed, since imageNamed: is a class method. Shouldn't it be "of +[UIImage imageNamed:." then? https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... ios/chrome/app/startup/setup_debugging.mm:36: Class klass = objc_getClass("UIImage"); On 2017/05/09 09:35:30, marq (OOO to May 9) wrote: > Don't use the Java 'klass' naming convention. 'class' isn't an Obj-C reserved > word, so just use that (or inline the getClass() call). But "class" is a C++ reserved name, and cannot be used in a Obj-C++ file.
Description was changed from ========== [WIP] DCHECK when loading images that do not exist. Enabled only in debug mode. This works by swizzling [UIImage imageNamed:] and making sure that it returns a non-nil UIImage*. BUG=718850 ========== to ========== DCHECK when loading images that do not exist. Enabled only in debug mode. This works by swizzling [UIImage imageNamed:] and making sure that it returns a non-nil UIImage*. BUG=718850 ==========
https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... File ios/chrome/app/startup/setup_debugging.mm (right): https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... ios/chrome/app/startup/setup_debugging.mm:28: // The original implementation of [UIImage imageNamed:]. On 2017/05/09 10:07:27, sdefresne wrote: > On 2017/05/09 09:35:31, marq (OOO to May 9) wrote: > > On 2017/05/05 15:24:37, lpromero wrote: > > > Super tiny nit: Add a dash: "of -[". > > > > I don't think that's needed, since imageNamed: is a class method. > > Shouldn't it be "of +[UIImage imageNamed:." then? Either: [UIImage imageNamed:] or UIImage's +imageNamed: method. https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... ios/chrome/app/startup/setup_debugging.mm:36: Class klass = objc_getClass("UIImage"); On 2017/05/09 10:07:27, sdefresne wrote: > On 2017/05/09 09:35:30, marq (OOO to May 9) wrote: > > Don't use the Java 'klass' naming convention. 'class' isn't an Obj-C reserved > > word, so just use that (or inline the getClass() call). > > But "class" is a C++ reserved name, and cannot be used in a Obj-C++ file. Then, 'aClass'.
thank you. ptal https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... File ios/chrome/app/startup/setup_debugging.mm (right): https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... ios/chrome/app/startup/setup_debugging.mm:28: // The original implementation of [UIImage imageNamed:]. On 2017/05/10 12:59:11, marq (ping after 24h) wrote: > On 2017/05/09 10:07:27, sdefresne wrote: > > On 2017/05/09 09:35:31, marq (OOO to May 9) wrote: > > > On 2017/05/05 15:24:37, lpromero wrote: > > > > Super tiny nit: Add a dash: "of -[". > > > > > > I don't think that's needed, since imageNamed: is a class method. > > > > Shouldn't it be "of +[UIImage imageNamed:." then? > > Either: > > [UIImage imageNamed:] > > or > > UIImage's +imageNamed: method. Done. https://codereview.chromium.org/2863983002/diff/1/ios/chrome/app/startup/setu... ios/chrome/app/startup/setup_debugging.mm:36: Class klass = objc_getClass("UIImage"); On 2017/05/10 12:59:11, marq (ping after 24h) wrote: > On 2017/05/09 10:07:27, sdefresne wrote: > > On 2017/05/09 09:35:30, marq (OOO to May 9) wrote: > > > Don't use the Java 'klass' naming convention. 'class' isn't an Obj-C > reserved > > > word, so just use that (or inline the getClass() call). > > > > But "class" is a C++ reserved name, and cannot be used in a Obj-C++ file. > > Then, 'aClass'. Done.
The CQ bit was checked by jif@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: This issue passed the CQ dry run.
LGTM, nice tool to have. Do we load any assets using other mechanisms?
On 2017/05/11 08:53:51, marq (ping after 24h) wrote: > LGTM, nice tool to have. > > Do we load any assets using other mechanisms? Yes! [UIIMage imageWithContentsOfFile:] [NSData dataWithContentsOfFile:] Filed bug about it: crbug.com/721258 Now I just need to find a swizzling/NSInvocation wizard to implement this without duplicating the code 3 times.
The CQ bit was checked by jif@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": 40001, "attempt_start_ts": 1494494280293960,
"parent_rev": "c85a433aeda481ee57352a35d0da781490acc7d2", "commit_rev":
"b75611794c4ee0538c0f62aaf75abaa409d8963d"}
Message was sent while issue was closed.
Description was changed from ========== DCHECK when loading images that do not exist. Enabled only in debug mode. This works by swizzling [UIImage imageNamed:] and making sure that it returns a non-nil UIImage*. BUG=718850 ========== to ========== DCHECK when loading images that do not exist. Enabled only in debug mode. This works by swizzling [UIImage imageNamed:] and making sure that it returns a non-nil UIImage*. BUG=718850 Review-Url: https://codereview.chromium.org/2863983002 Cr-Commit-Position: refs/heads/master@{#470887} Committed: https://chromium.googlesource.com/chromium/src/+/b75611794c4ee0538c0f62aaf75a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b75611794c4ee0538c0f62aaf75a...
Message was sent while issue was closed.
pkl@chromium.org changed reviewers: + pkl@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2863983002/diff/40001/ios/chrome/app/startup/... File ios/chrome/app/startup/setup_debugging.mm (right): https://codereview.chromium.org/2863983002/diff/40001/ios/chrome/app/startup/... ios/chrome/app/startup/setup_debugging.mm:34: When I run locally in debugger, I'm getting many DCHECKs for images such as these: ptr_new_tab omnibox_background keyboard_button custom_row_voice custom_row_voice_pressed ... Are we supposed to keep adding to this whiteList?
Message was sent while issue was closed.
https://codereview.chromium.org/2863983002/diff/40001/ios/chrome/app/startup/... File ios/chrome/app/startup/setup_debugging.mm (right): https://codereview.chromium.org/2863983002/diff/40001/ios/chrome/app/startup/... ios/chrome/app/startup/setup_debugging.mm:21: // Swizzles [UIImage imageNamed:] to trigger a DCHECK if an invalid image is There are other methods for loading images. For example: https://cs.corp.google.com/piper///depot/google3/googlemac/iPhone/Shared/SSOA...
Message was sent while issue was closed.
https://codereview.chromium.org/2863983002/diff/40001/ios/chrome/app/startup/... File ios/chrome/app/startup/setup_debugging.mm (right): https://codereview.chromium.org/2863983002/diff/40001/ios/chrome/app/startup/... ios/chrome/app/startup/setup_debugging.mm:21: // Swizzles [UIImage imageNamed:] to trigger a DCHECK if an invalid image is On 2017/05/29 13:31:12, lpromero wrote: > There are other methods for loading images. For example: > https://cs.corp.google.com/piper///depot/google3/googlemac/iPhone/Shared/SSOA... Correct! See https://bugs.chromium.org/p/chromium/issues/detail?id=721258 |
