Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1580983002:
Fix the dynamic browser action setIcon path to work with any size icon. (Closed)
|
Created:
4 years, 11 months ago by Evan Stade Modified:
4 years, 11 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the dynamic browser action setIcon path to work with any size icon.
BUG=564926, 575256
TBR=benwells@chromium.org
Committed: https://crrev.com/168694381abc2a710a04fe765548849ebbde0e0e
Cr-Commit-Position: refs/heads/master@{#371133}
Patch Set 1 #
Total comments: 21
Patch Set 2 : devlin review #Patch Set 3 : fix rounding error #
Total comments: 2
Patch Set 4 : mac compatible image equality check #
Total comments: 12
Patch Set 5 : attempt image comparison #Patch Set 6 : slice n dice #Patch Set 7 : update more json #
Total comments: 4
Patch Set 8 : fix pre-existing typos and add positive value check #Patch Set 9 : move size validation #
Total comments: 2
Patch Set 10 : twiddle constant #Messages
Total messages: 73 (25 generated)
estade@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/1580983002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc (right): https://codereview.chromium.org/1580983002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:202: // Make sure the browser action bar updated. I couldn't come up with a good way to keep comparing actual pixels but I think we can maintain the spirit of the test without doing so. https://codereview.chromium.org/1580983002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (left): https://codereview.chromium.org/1580983002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:162: gfx::ImageSkia bg = *ResourceBundle::GetSharedInstance().GetImageSkiaNamed( this wasn't doing anything (except enlarging the image, which shouldn't be necessary as the image gets centered) because the asset is just a fully transparent png. https://codereview.chromium.org/1580983002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/browser_action.json (right): https://codereview.chromium.org/1580983002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/browser_action.json:98: "description": "Either an ImageData object or a dictionary {size -> ImageData} representing icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * 19 will be selected. Initially only scales 1 and 2 will be supported. At least one image must be specified. Note that 'details.imageData = foo' is equivalent to 'details.imageData = {'19': foo}'" self review: should update these comments https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... File extensions/renderer/resources/set_icon.js (left): https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... extensions/renderer/resources/set_icon.js:46: 'The imageData property must contain an ImageData object that ' + I guess we could keep this check, I'm not sure what good it's doing though.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580983002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580983002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1580983002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc (right): https://codereview.chromium.org/1580983002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:202: // Make sure the browser action bar updated. On 2016/01/13 18:35:41, Evan Stade wrote: > I couldn't come up with a good way to keep comparing actual pixels but I think > we can maintain the spirit of the test without doing so. What's stopping us from comparing pixels/bitmaps/etc still? https://codereview.chromium.org/1580983002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (left): https://codereview.chromium.org/1580983002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:162: gfx::ImageSkia bg = *ResourceBundle::GetSharedInstance().GetImageSkiaNamed( On 2016/01/13 18:35:41, Evan Stade wrote: > this wasn't doing anything (except enlarging the image, which shouldn't be > necessary as the image gets centered) because the asset is just a fully > transparent png. Hmm... my guess is that maybe we *needed* to enlarge the icon at some point (because we do utilize the area around the icon for, e.g., the badge). But I think all that code has migrated, so I think you're right that this is okay. We'll see if anything breaks. https://codereview.chromium.org/1580983002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/browser_action/no_icon/background.js (right): https://codereview.chromium.org/1580983002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/browser_action/no_icon/background.js:6: getImageData(0, 0, 21, 21); Were these chosen as sizes that are unused? https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... File extensions/renderer/resources/set_icon.js (left): https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... extensions/renderer/resources/set_icon.js:46: 'The imageData property must contain an ImageData object that ' + On 2016/01/13 18:35:41, Evan Stade wrote: > I guess we could keep this check, I'm not sure what good it's doing though. Well, if we can scale, it's totally irrelevant, right? https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... extensions/renderer/resources/set_icon.js:88: var detailKeyCount = Object.getOwnPropertyNames(details.path).length; nit: getOwnPropertyNames wouldn't allow for inherited properties, whereas the for-in loops do. This probably isn't a big deal (I don't know how many devs, if any, would use inheritance for setIcon arguments), but it might be nice to be consistent (and lean towards just allowing all enumerable properties). https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... extensions/renderer/resources/set_icon.js:94: if (--detailKeyCount == 0) { This particular part would be more elegant with Promise.all(), but means converting some stuff to promises. I'll leave it up to you if you want to do it. https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... extensions/renderer/resources/set_icon.js:98: }.bind(null, iconSize)); do we need this?
The CQ bit was checked by estade@chromium.org to run a CQ dry run
https://codereview.chromium.org/1580983002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc (right): https://codereview.chromium.org/1580983002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:202: // Make sure the browser action bar updated. On 2016/01/13 19:58:12, Devlin wrote: > On 2016/01/13 18:35:41, Evan Stade wrote: > > I couldn't come up with a good way to keep comparing actual pixels but I think > > we can maintain the spirit of the test without doing so. > > What's stopping us from comparing pixels/bitmaps/etc still? We have to reconstruct the scaling that I recently added to icon_with_badge_image_source.cc (in addition to the AddBackground mumbo jumbo), but I couldn't get it to work and it's impossible to debug so after some time I gave up. I don't think it's a good test when debugging is impossible. https://codereview.chromium.org/1580983002/diff/1/chrome/browser/ui/views/too... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (left): https://codereview.chromium.org/1580983002/diff/1/chrome/browser/ui/views/too... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:162: gfx::ImageSkia bg = *ResourceBundle::GetSharedInstance().GetImageSkiaNamed( On 2016/01/13 19:58:12, Devlin wrote: > On 2016/01/13 18:35:41, Evan Stade wrote: > > this wasn't doing anything (except enlarging the image, which shouldn't be > > necessary as the image gets centered) because the asset is just a fully > > transparent png. > > Hmm... my guess is that maybe we *needed* to enlarge the icon at some point > (because we do utilize the area around the icon for, e.g., the badge). But I > think all that code has migrated, so I think you're right that this is okay. > We'll see if anything breaks. my personal guess is that the background asset used to be non-transparent (I can't find any old screenshots to confirm this, but it's possible it just never made it into a stable release) https://codereview.chromium.org/1580983002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/browser_action/no_icon/background.js (right): https://codereview.chromium.org/1580983002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/browser_action/no_icon/background.js:6: getImageData(0, 0, 21, 21); On 2016/01/13 19:58:12, Devlin wrote: > Were these chosen as sizes that are unused? yea, pretty much arbitrary. As the comment in the test states, I didn't want x / 16 or x / 19 to come out to a scale factor that's likely to appear on any device that might run these tests, because then the tests are not as easy to write (as representations will get automatically added for the active scale factor, so testing their non-existence doesn't work). https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... File extensions/renderer/resources/set_icon.js (left): https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... extensions/renderer/resources/set_icon.js:46: 'The imageData property must contain an ImageData object that ' + On 2016/01/13 19:58:12, Devlin wrote: > On 2016/01/13 18:35:41, Evan Stade wrote: > > I guess we could keep this check, I'm not sure what good it's doing though. > > Well, if we can scale, it's totally irrelevant, right? I'm assuming we'd change this to width/height == iconSize. All we would be enforcing is that { "21": someImageData } is consistent (someImageData is actually 21x21). Now we don't have to enforce this, but it's not totally irrelevant: most likely if it trips it's catching a developer's mistake. There's no reason I can think of to bother specifying a size that's not correct, as opposed to omitting that size or correcting the key. But if we tighten the verification to exact equality, that could break some existing extensions I guess. :[ https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... extensions/renderer/resources/set_icon.js:88: var detailKeyCount = Object.getOwnPropertyNames(details.path).length; On 2016/01/13 19:58:12, Devlin wrote: > nit: getOwnPropertyNames wouldn't allow for inherited properties, whereas the > for-in loops do. This probably isn't a big deal (I don't know how many devs, if > any, would use inheritance for setIcon arguments), but it might be nice to be > consistent (and lean towards just allowing all enumerable properties). Done. https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... extensions/renderer/resources/set_icon.js:94: if (--detailKeyCount == 0) { On 2016/01/13 19:58:12, Devlin wrote: > This particular part would be more elegant with Promise.all(), but means > converting some stuff to promises. I'll leave it up to you if you want to do > it. Tempting, but I'll pass for now. :) https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... extensions/renderer/resources/set_icon.js:98: }.bind(null, iconSize)); On 2016/01/13 19:58:12, Devlin wrote: > do we need this? Believe me, it didn't occur to me to add this and it caused much anguish until I realized the problem. Without it, we depend on iconSize inside the inlined function, but by the time the first load comes back, iconSize has already been changed to a later key. js is fun.
estade@chromium.org changed reviewers: + oshima@chromium.org
https://codereview.chromium.org/1580983002/diff/40001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/icon_with_badge_image_source.cc (left): https://codereview.chromium.org/1580983002/diff/40001/chrome/browser/ui/exten... chrome/browser/ui/extensions/icon_with_badge_image_source.cc:95: gfx::ScaleToCeiledSize(rep.pixel_size(), target_scale / rep.scale()); found a rounding error here (+oshima) sometimes scaled_size would come out to 1px more than we wanted, so the scaled image becomes 20px instead of 19px.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580983002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580983002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/1580983002/diff/40001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/icon_with_badge_image_source.cc (left): https://codereview.chromium.org/1580983002/diff/40001/chrome/browser/ui/exten... chrome/browser/ui/extensions/icon_with_badge_image_source.cc:95: gfx::ScaleToCeiledSize(rep.pixel_size(), target_scale / rep.scale()); On 2016/01/14 02:04:06, Evan Stade (ooo till Tue 1.19) wrote: > found a rounding error here (+oshima) > > sometimes scaled_size would come out to 1px more than we wanted, so the scaled > image becomes 20px instead of 19px. Acknowledged.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580983002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580983002/60001
https://codereview.chromium.org/1580983002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc (right): https://codereview.chromium.org/1580983002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:202: // Make sure the browser action bar updated. On 2016/01/14 01:59:19, Evan Stade wrote: > On 2016/01/13 19:58:12, Devlin wrote: > > On 2016/01/13 18:35:41, Evan Stade wrote: > > > I couldn't come up with a good way to keep comparing actual pixels but I > think > > > we can maintain the spirit of the test without doing so. > > > > What's stopping us from comparing pixels/bitmaps/etc still? > > We have to reconstruct the scaling that I recently added to > icon_with_badge_image_source.cc (in addition to the AddBackground mumbo jumbo), > but I couldn't get it to work and it's impossible to debug so after some time I > gave up. I don't think it's a good test when debugging is impossible. It seems that what we should be checking is that the relationship between icon_factory.GetIcon(0) and BrowserActionsBar()->GetIcon(0) exists, and, since the scaling is just adding skia reps, I would have thought there'd be a good way to somehow compare the shared Image object - but it seems it's rather buried. But this makes me nervous, because this is probably our only test for dynamic browser actions. If there's no way to suitably compare these at scaling, can you add a second simple test that just tests the dynamic icon is used (with no scaling, so we can do comparison)? https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... File extensions/renderer/resources/set_icon.js (right): https://codereview.chromium.org/1580983002/diff/1/extensions/renderer/resourc... extensions/renderer/resources/set_icon.js:98: }.bind(null, iconSize)); On 2016/01/14 01:59:19, Evan Stade wrote: > On 2016/01/13 19:58:12, Devlin wrote: > > do we need this? > > Believe me, it didn't occur to me to add this and it caused much anguish until I > realized the problem. Without it, we depend on iconSize inside the inlined > function, but by the time the first load comes back, iconSize has already been > changed to a later key. js is fun. Wow. https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc (right): https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:207: extra newline https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:213: EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale)); nit: please comment above these why we expect/don't expect a certain representation. https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:267: // setting only size 19. These comments (also on line 250) need to be updated, right? https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/icon_with_badge_image_source.cc (right): https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/icon_with_badge_image_source.cc:135: } if (grayscale_) { this if should go on a newline.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
varkha@chromium.org changed reviewers: + varkha@chromium.org
Do the following 2 files need to be updated as well: chrome/common/extensions/api/declarative_content.json chrome/common/extensions/api/page_action.json ? https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/icon_with_badge_image_source.cc (right): https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/icon_with_badge_image_source.cc:98: target_width, target_width), Is it OK to use |target_width| for both dest_width and dest_height with what could possibly be non-square images? https://codereview.chromium.org/1580983002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/browser_action.json (right): https://codereview.chromium.org/1580983002/diff/60001/chrome/common/extension... chrome/common/extensions/api/browser_action.json:98: "description": "Either an ImageData object or a dictionary {size -> ImageData} representing icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * 19 will be selected. Initially only scales 1 and 2 will be supported. At least one image must be specified. Note that 'details.imageData = foo' is equivalent to 'details.imageData = {'19': foo}'" nit: Those descriptions will need an update.
https://codereview.chromium.org/1580983002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc (right): https://codereview.chromium.org/1580983002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:202: // Make sure the browser action bar updated. On 2016/01/19 22:44:40, Devlin wrote: > On 2016/01/14 01:59:19, Evan Stade wrote: > > On 2016/01/13 19:58:12, Devlin wrote: > > > On 2016/01/13 18:35:41, Evan Stade wrote: > > > > I couldn't come up with a good way to keep comparing actual pixels but I > > think > > > > we can maintain the spirit of the test without doing so. > > > > > > What's stopping us from comparing pixels/bitmaps/etc still? > > > > We have to reconstruct the scaling that I recently added to > > icon_with_badge_image_source.cc (in addition to the AddBackground mumbo > jumbo), > > but I couldn't get it to work and it's impossible to debug so after some time > I > > gave up. I don't think it's a good test when debugging is impossible. > > It seems that what we should be checking is that the relationship between > icon_factory.GetIcon(0) and BrowserActionsBar()->GetIcon(0) exists, and, since > the scaling is just adding skia reps, I would have thought there'd be a good way > to somehow compare the shared Image object - but it seems it's rather buried. > But this makes me nervous, because this is probably our only test for dynamic > browser actions. > > If there's no way to suitably compare these at scaling, can you add a second > simple test that just tests the dynamic icon is used (with no scaling, so we can > do comparison)? I think I found a way to salvage image comparisons. We'll see if mac likes it. I can't even really explain why this works and other attempts failed because again, debugging image comparisons is hard. https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc (right): https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:207: On 2016/01/19 22:44:40, Devlin wrote: > extra newline Done. https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:213: EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale)); On 2016/01/19 22:44:40, Devlin wrote: > nit: please comment above these why we expect/don't expect a certain > representation. done, also see comment above the kSmallIconScale definition https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:267: // setting only size 19. On 2016/01/19 22:44:40, Devlin wrote: > These comments (also on line 250) need to be updated, right? Done. https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/icon_with_badge_image_source.cc (right): https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/icon_with_badge_image_source.cc:135: } if (grayscale_) { On 2016/01/19 22:44:40, Devlin wrote: > this if should go on a newline. Done.
lgtm, thanks for finding a way to do the comparisons. I I think varkha@ is right that page_action and declarative_content apis also need to be updated (I'm pretty sure the code all goes to the same place [set_icon], so hopefully this is less painful).
Description was changed from ========== Fix the dynamic browser action setIcon path to work with any size icon. BUG=564926 ========== to ========== Fix the dynamic browser action setIcon path to work with any size icon. BUG=564926,575256 ==========
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580983002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580983002/100001
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...)
https://codereview.chromium.org/1580983002/diff/120001/chrome/common/extensio... File chrome/common/extensions/api/browser_action.json (right): https://codereview.chromium.org/1580983002/diff/120001/chrome/common/extensio... chrome/common/extensions/api/browser_action.json:109: "description": "Either a relative image path or a dictionary {size -> relative image path} pointing to icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * n will be selected, where n is the size of the icon in the UI. At least one image must be specified. Note that 'details.path = foo' is equivalent to 'details.imageData = {'16': foo}'" Is this a copy and paste typo? s/details.imageData/details.path https://codereview.chromium.org/1580983002/diff/120001/chrome/common/extensio... File chrome/common/extensions/api/declarative_content.json (right): https://codereview.chromium.org/1580983002/diff/120001/chrome/common/extensio... chrome/common/extensions/api/declarative_content.json:94: // "description": "Either a relative image path or a dictionary {size -> relative image path} pointing to icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * n will be selected, where n is the size of the icon in the UI. At least one image must be specified. Note that 'details.path = foo' is equivalent to 'details.imageData = {'16': foo}'" s/details.imageData/details.path ? https://codereview.chromium.org/1580983002/diff/120001/chrome/common/extensio... File chrome/common/extensions/api/page_action.json (right): https://codereview.chromium.org/1580983002/diff/120001/chrome/common/extensio... chrome/common/extensions/api/page_action.json:107: "description": "Either a relative image path or a dictionary {size -> relative image path} pointing to icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * n will be selected, where n is the size of the icon in the UI. At least one image must be specified. Note that 'details.path = foo' is equivalent to 'details.imageData = {'16': foo}'" s/details.imageData/details.path ?
Another question. With this API now accepting any icon size is there any validation for the size of binary or scale factors in the code? What happens if the icon_size key is negative / zero / very large? Might there be any way to exploit that?
On 2016/01/21 17:18:27, varkha wrote: > Another question. With this API now accepting any icon size is there any > validation for the size of binary or scale factors in the code? What happens if > the icon_size key is negative / zero / very large? Might there be any way to > exploit that? That's a good point (I don't think it could be exploited because we do some sanity-checking validation, but it might not be desirable). Some quick validation that the size is between > 0 (or even, say, 4) and around a thousand would probably be appropriate.
I have traced the call stack and I cannot see any red flags currently since we only divide the icon_size by the native size and never other way around and then use the scale as a key in a map of representations so if the scale doesn't match we are not using it. Still sanitizing parameters in JS could be a good idea.
As far as the "exploit" angle is concerned, if you have a malicious extension installed, you're already hosed. Should we be worried about super large icons? I dunno. I did add a positive value check in set_icon_natives which just drops entries with negative keys. https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/icon_with_badge_image_source.cc (right): https://codereview.chromium.org/1580983002/diff/60001/chrome/browser/ui/exten... chrome/browser/ui/extensions/icon_with_badge_image_source.cc:98: target_width, target_width), On 2016/01/20 21:22:23, varkha wrote: > Is it OK to use |target_width| for both dest_width and dest_height with what > could possibly be non-square images? The images *should* always be square. The API allows you to specify something like {"19": "icon.png"}, so it assumes a square. https://codereview.chromium.org/1580983002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/browser_action.json (right): https://codereview.chromium.org/1580983002/diff/60001/chrome/common/extension... chrome/common/extensions/api/browser_action.json:98: "description": "Either an ImageData object or a dictionary {size -> ImageData} representing icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * 19 will be selected. Initially only scales 1 and 2 will be supported. At least one image must be specified. Note that 'details.imageData = foo' is equivalent to 'details.imageData = {'19': foo}'" On 2016/01/20 21:22:23, varkha wrote: > nit: Those descriptions will need an update. Done. https://codereview.chromium.org/1580983002/diff/120001/chrome/common/extensio... File chrome/common/extensions/api/browser_action.json (right): https://codereview.chromium.org/1580983002/diff/120001/chrome/common/extensio... chrome/common/extensions/api/browser_action.json:109: "description": "Either a relative image path or a dictionary {size -> relative image path} pointing to icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * n will be selected, where n is the size of the icon in the UI. At least one image must be specified. Note that 'details.path = foo' is equivalent to 'details.imageData = {'16': foo}'" On 2016/01/21 03:05:49, varkha wrote: > Is this a copy and paste typo? s/details.imageData/details.path Done.
Actually I moved the size validation to extension_action.cc so it would work for manifest icons as well as dynamic ones.
On 2016/01/21 19:41:29, Evan Stade wrote: > Actually I moved the size validation to extension_action.cc so it would work for > manifest icons as well as dynamic ones. I think we should also add a similar check in LoadIconsFromDictionary(). And, of course, extra tests for this would be nice. I'm fine with postponing that to a later CL.
On 2016/01/21 19:53:25, Devlin (Slow until 1-27) wrote: > On 2016/01/21 19:41:29, Evan Stade wrote: > > Actually I moved the size validation to extension_action.cc so it would work > for > > manifest icons as well as dynamic ones. > > I think we should also add a similar check in LoadIconsFromDictionary(). And, > of course, extra tests for this would be nice. I'm fine with postponing that to > a later CL. extension_action.cc is already used for the manifest case. Why do we want to add a redundant check?
On 2016/01/21 20:10:37, Evan Stade wrote: > On 2016/01/21 19:53:25, Devlin (Slow until 1-27) wrote: > > On 2016/01/21 19:41:29, Evan Stade wrote: > > > Actually I moved the size validation to extension_action.cc so it would work > > for > > > manifest icons as well as dynamic ones. > > > > I think we should also add a similar check in LoadIconsFromDictionary(). And, > > of course, extra tests for this would be nice. I'm fine with postponing that > to > > a later CL. > > extension_action.cc is already used for the manifest case. Why do we want to add > a redundant check? ParseIconFromCanvasDictionary looks like it's only called from the ExtensionActionAPI and DeclarativeContentAPI. What am I missing?
On 2016/01/21 21:01:21, Devlin (Slow until 1-27) wrote: > On 2016/01/21 20:10:37, Evan Stade wrote: > > On 2016/01/21 19:53:25, Devlin (Slow until 1-27) wrote: > > > On 2016/01/21 19:41:29, Evan Stade wrote: > > > > Actually I moved the size validation to extension_action.cc so it would > work > > > for > > > > manifest icons as well as dynamic ones. > > > > > > I think we should also add a similar check in LoadIconsFromDictionary(). > And, > > > of course, extra tests for this would be nice. I'm fine with postponing > that > > to > > > a later CL. > > > > extension_action.cc is already used for the manifest case. Why do we want to > add > > a redundant check? > > ParseIconFromCanvasDictionary looks like it's only called from the > ExtensionActionAPI and DeclarativeContentAPI. What am I missing? K, I take it back. I assumed it was so because I updated that function in my original round of changes. But it turns out it's not used. LoadIconsFromDictionary isn't used either. That's only for the product icons. The manifest action icons are loaded here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... As you can see only the first one matters and the key (size) is just ignored. The image itself can be any size and will be scaled.
On 2016/01/21 23:47:19, Evan Stade wrote: > On 2016/01/21 21:01:21, Devlin (Slow until 1-27) wrote: > > On 2016/01/21 20:10:37, Evan Stade wrote: > > > On 2016/01/21 19:53:25, Devlin (Slow until 1-27) wrote: > > > > On 2016/01/21 19:41:29, Evan Stade wrote: > > > > > Actually I moved the size validation to extension_action.cc so it would > > work > > > > for > > > > > manifest icons as well as dynamic ones. > > > > > > > > I think we should also add a similar check in LoadIconsFromDictionary(). > > And, > > > > of course, extra tests for this would be nice. I'm fine with postponing > > that > > > to > > > > a later CL. > > > > > > extension_action.cc is already used for the manifest case. Why do we want to > > add > > > a redundant check? > > > > ParseIconFromCanvasDictionary looks like it's only called from the > > ExtensionActionAPI and DeclarativeContentAPI. What am I missing? > > K, I take it back. I assumed it was so because I updated that function in my > original round of changes. But it turns out it's not used. > LoadIconsFromDictionary isn't used either. That's only for the product icons. > The manifest action icons are loaded here: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > As you can see only the first one matters and the key (size) is just ignored. > The image itself can be any size and will be scaled. TODO: Clean up icon initialization code at some point. LoadIconsFromDictionary is used, but only for page action default icons (grr...). So it looks like there's no good single place to validate icon size. Ideally, we'd make one. That's work for another CL. For here, what about we just leave some validation code in the js (that's a good spot anyway, since it'll save process hops if it's invalid), and tackle the other places (product icons, manifest-specified icons) in another CL.
On 2016/01/22 00:18:35, Devlin (Slow until 1-27) wrote: > On 2016/01/21 23:47:19, Evan Stade wrote: > > On 2016/01/21 21:01:21, Devlin (Slow until 1-27) wrote: > > > On 2016/01/21 20:10:37, Evan Stade wrote: > > > > On 2016/01/21 19:53:25, Devlin (Slow until 1-27) wrote: > > > > > On 2016/01/21 19:41:29, Evan Stade wrote: > > > > > > Actually I moved the size validation to extension_action.cc so it > would > > > work > > > > > for > > > > > > manifest icons as well as dynamic ones. > > > > > > > > > > I think we should also add a similar check in LoadIconsFromDictionary(). > > > > And, > > > > > of course, extra tests for this would be nice. I'm fine with postponing > > > that > > > > to > > > > > a later CL. > > > > > > > > extension_action.cc is already used for the manifest case. Why do we want > to > > > add > > > > a redundant check? > > > > > > ParseIconFromCanvasDictionary looks like it's only called from the > > > ExtensionActionAPI and DeclarativeContentAPI. What am I missing? > > > > K, I take it back. I assumed it was so because I updated that function in my > > original round of changes. But it turns out it's not used. > > LoadIconsFromDictionary isn't used either. That's only for the product icons. > > The manifest action icons are loaded here: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > > > As you can see only the first one matters and the key (size) is just ignored. > > The image itself can be any size and will be scaled. > > TODO: Clean up icon initialization code at some point. > > LoadIconsFromDictionary is used, but only for page action default icons > (grr...). > > So it looks like there's no good single place to validate icon size. Ideally, > we'd make one. That's work for another CL. For here, what about we just leave > some validation code in the js (that's a good spot anyway, since it'll save > process hops if it's invalid), and tackle the other places (product icons, > manifest-specified icons) in another CL. I have a separate CL for LoadIconsFromDictionary up. Between that one and this one, are we missing any cases? I don't think avoiding process hops is a great win given that this is an error case that should only be hit during the extension development cycle (not by real users).
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580983002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580983002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(s lgtm) > I have a separate CL for LoadIconsFromDictionary up. Between that one and this > one, are we missing any cases? I don't think so. I'd feel a little bit better with an api test that tries to use a too-large icon. > I don't think avoiding process hops is a great win given that this is an error > case that should only be hit during the extension development cycle (not by real > users). If the extension author cares, this is true. :) In either case, I think this way is fine. https://codereview.chromium.org/1580983002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_action.cc (right): https://codereview.chromium.org/1580983002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_action.cc:134: icon_size > 1024) { nit: in your other CL, you use GIGANTOR * 4. We should use the same limit here (and probably constify it as something like MaxAllowableIconSize)
https://codereview.chromium.org/1580983002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_action.cc (right): https://codereview.chromium.org/1580983002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_action.cc:134: icon_size > 1024) { On 2016/01/22 23:41:40, Devlin (Slow until 1-27) wrote: > nit: in your other CL, you use GIGANTOR * 4. We should use the same limit here > (and probably constify it as something like MaxAllowableIconSize) Admittedly, 1024 is somewhat arbitrary. But why match GIGANGTOR? Extension icons are not currently related in size to the value for GIGANTOR. They are EXTENSION_ICON_ACTION. I picked GIGANGTOR*4 as what you'd need for GIGANTOR at 4x scale (which makes sense for product icons). If anything, we should be limiting this to EXTENSION_ICON_ACTION * 4.
On 2016/01/23 00:03:25, Evan Stade wrote: > https://codereview.chromium.org/1580983002/diff/160001/chrome/browser/extensi... > File chrome/browser/extensions/extension_action.cc (right): > > https://codereview.chromium.org/1580983002/diff/160001/chrome/browser/extensi... > chrome/browser/extensions/extension_action.cc:134: icon_size > 1024) { > On 2016/01/22 23:41:40, Devlin (Slow until 1-27) wrote: > > nit: in your other CL, you use GIGANTOR * 4. We should use the same limit > here > > (and probably constify it as something like MaxAllowableIconSize) > > Admittedly, 1024 is somewhat arbitrary. But why match GIGANGTOR? Extension icons > are not currently related in size to the value for GIGANTOR. They are > EXTENSION_ICON_ACTION. I picked GIGANGTOR*4 as what you'd need for GIGANTOR at > 4x scale (which makes sense for product icons). If anything, we should be > limiting this to EXTENSION_ICON_ACTION * 4. My goal is just to make sure no one tries to use an awesome gigapixel picture as an icon and have Chrome scale it. ;) Beyond that, use your best judgment. Regardless of what you decide, please to at least constify it (even local to the file), e.g. // Though we allow for scaled action icons, keep it reasonable. A good maximum is <FOO>. const int kMaxExtensionIconSize = <FOO>;
> please to at least constify it s/to/do
On 2016/01/23 00:22:06, Devlin (Slow until 1-27) wrote: > On 2016/01/23 00:03:25, Evan Stade wrote: > > > https://codereview.chromium.org/1580983002/diff/160001/chrome/browser/extensi... > > File chrome/browser/extensions/extension_action.cc (right): > > > > > https://codereview.chromium.org/1580983002/diff/160001/chrome/browser/extensi... > > chrome/browser/extensions/extension_action.cc:134: icon_size > 1024) { > > On 2016/01/22 23:41:40, Devlin (Slow until 1-27) wrote: > > > nit: in your other CL, you use GIGANTOR * 4. We should use the same limit > > here > > > (and probably constify it as something like MaxAllowableIconSize) > > > > Admittedly, 1024 is somewhat arbitrary. But why match GIGANGTOR? Extension > icons > > are not currently related in size to the value for GIGANTOR. They are > > EXTENSION_ICON_ACTION. I picked GIGANGTOR*4 as what you'd need for GIGANTOR at > > 4x scale (which makes sense for product icons). If anything, we should be > > limiting this to EXTENSION_ICON_ACTION * 4. > > My goal is just to make sure no one tries to use an awesome gigapixel picture as > an icon and have Chrome scale it. ;) Beyond that, use your best judgment. > Regardless of what you decide, please to at least constify it (even local to the > file), e.g. > // Though we allow for scaled action icons, keep it reasonable. A good maximum > is <FOO>. > const int kMaxExtensionIconSize = <FOO>; done
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1580983002/#ps180001 (title: "twiddle constant")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580983002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580983002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, no build URL) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build URL) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, no build URL) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build URL)
The CQ bit was checked by tandrii@chromium.org
On 2016/01/23 01:08:06, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, no build URL) > chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux > (JOB_FAILED, no build URL) > chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux > (JOB_FAILED, no build URL) > chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux > (JOB_FAILED, no build URL) > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL) > linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux > (JOB_FAILED, no build URL) > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, > no build URL) > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, no > build URL) > linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, no > build URL) > linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, no > build URL) > linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, no > build URL) > linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, no > build URL) > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build URL) sorry, my bad. Re-triggering CQ for you.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580983002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580983002/180001
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...)
On 2016/01/23 01:29:25, commit-bot: I haz the power wrote: > 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...) ok, this failure isn't due to CQ. It's genuine: ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: chrome/browser/ui/extensions/icon_with_badge_image_source.cc
Description was changed from ========== Fix the dynamic browser action setIcon path to work with any size icon. BUG=564926,575256 ========== to ========== Fix the dynamic browser action setIcon path to work with any size icon. BUG=564926,575256 TBR=benwells@chromium.org ==========
TBR=benwells for chrome/browser/ui/extensions/icon_with_badge_image_source.cc
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1580983002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1580983002/180001
estade@chromium.org changed reviewers: + benwells@chromium.org
actually +benwells this time
Message was sent while issue was closed.
Description was changed from ========== Fix the dynamic browser action setIcon path to work with any size icon. BUG=564926,575256 TBR=benwells@chromium.org ========== to ========== Fix the dynamic browser action setIcon path to work with any size icon. BUG=564926,575256 TBR=benwells@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Fix the dynamic browser action setIcon path to work with any size icon. BUG=564926,575256 TBR=benwells@chromium.org ========== to ========== Fix the dynamic browser action setIcon path to work with any size icon. BUG=564926,575256 TBR=benwells@chromium.org Committed: https://crrev.com/168694381abc2a710a04fe765548849ebbde0e0e Cr-Commit-Position: refs/heads/master@{#371133} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/168694381abc2a710a04fe765548849ebbde0e0e Cr-Commit-Position: refs/heads/master@{#371133}
Message was sent while issue was closed.
lgtm |
