Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1637763002:
Fix bug in browser action icon scaling for non-1x scale factors. (Closed)
|
Created:
4 years, 11 months ago by Evan Stade Modified:
4 years, 10 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, 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 bug in browser action icon scaling for non-1x scale factors.
skia::ImageOperations::Resize needs pixels, and we were passing dp.
Also, ignore the icon size key and just use the icon size itself. This fixes a bug (evidenced again by Shoptagr) where an icon size such as
"19": "icons/19.png" // 19.png is actually 50x50
would be scaled incorrectly. At this point, the API should take a list rather than a dict, but it's probably not possible to change that and maintain backcompat.
BUG=581232
TBR=benwells@chromium.org
Committed: https://crrev.com/52969d7388642a56447da090bf577974a6bf562f
Cr-Commit-Position: refs/heads/master@{#371904}
Patch Set 1 #Patch Set 2 : also deal with wrong icon sizes #Patch Set 3 : remove dbg code #
Total comments: 7
Patch Set 4 : size limits #Patch Set 5 : . #
Messages
Total messages: 31 (15 generated)
Description was changed from ========== Fix bug in browser action icon scaling for non-1x scale factors. skia::ImageOperations::Resize needs pixels, and we were passing dp. BUG=581232 ========== to ========== Fix bug in browser action icon scaling for non-1x scale factors. skia::ImageOperations::Resize needs pixels, and we were passing dp. Also, ignore the icon size key and just use the icon size itself. This fixes a bug (evidenced again by Shoptagr) where an icon size such as "19": "icons/19.png" // 19.png is actually 50x50 would be scaled incorrectly. At this point, the API should take a list rather than a dict, but it's probably not possible to change that and maintain backcompat. BUG=581232 ==========
estade@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
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/1637763002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637763002/40001
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
Tried it locally and all the icons looked correctly. https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/icon_with_badge_image_source.cc (right): https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/ui/exten... chrome/browser/ui/extensions/icon_with_badge_image_source.cc:136: rep, ExtensionAction::ActionIconSize(), canvas->image_scale())); nit: could cache ExtensionAction::ActionIconSize() return value.
lgtm https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_action.cc (left): https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_action.cc:136: icon_size > kActionIconMaxSize) { Do we not want any checks for ridiculous scaling? (Agreed that the index into the dictionary might not be useful, but we could check the bitmap size itself.) https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_action.cc (right): https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_action.cc:150: if (bitmap.width() != bitmap.height()) I wonder if we really *need* this to be a square. But we've always kind of assumed it was, so this is fine for now. Could you add a // TODO(devlin): Can we support rectangular icons?
https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_action.cc (left): https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_action.cc:136: icon_size > kActionIconMaxSize) { On 2016/01/26 19:08:48, Devlin (Slow until 1-27) wrote: > Do we not want any checks for ridiculous scaling? (Agreed that the index into > the dictionary might not be useful, but we could check the bitmap size itself.) righto, baby rescued from bathwater https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_action.cc (right): https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_action.cc:150: if (bitmap.width() != bitmap.height()) On 2016/01/26 19:08:48, Devlin (Slow until 1-27) wrote: > I wonder if we really *need* this to be a square. But we've always kind of > assumed it was, so this is fine for now. > > Could you add a > // TODO(devlin): Can we support rectangular icons? Why would we want to let developers do that? Is there any advantage? If we determine we need to scale the icon, we'll scale it to square, which looks bad if it's changing dimensions. If we don't think we need to scale (based on width), we'll use it as is, which will look bad if height > width (the icon would extend below the 16x16 box it's supposed to be inside).
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/1637763002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637763002/80001
https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_action.cc (right): https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_action.cc:150: if (bitmap.width() != bitmap.height()) On 2016/01/26 19:50:07, Evan Stade wrote: > On 2016/01/26 19:08:48, Devlin (Slow until 1-27) wrote: > > I wonder if we really *need* this to be a square. But we've always kind of > > assumed it was, so this is fine for now. > > > > Could you add a > > // TODO(devlin): Can we support rectangular icons? > > Why would we want to let developers do that? Is there any advantage? > > If we determine we need to scale the icon, we'll scale it to square, which looks > bad if it's changing dimensions. If we don't think we need to scale (based on > width), we'll use it as is, which will look bad if height > width (the icon > would extend below the 16x16 box it's supposed to be inside). Well, we can scale it to be proportionate and maintain the aspect ratio, and then center the icon. This is already what any developer without a perfectly square icon would do and then provide a square icon with a transparent background, but we could save them that step. But, low priority.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_action.cc (right): https://codereview.chromium.org/1637763002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_action.cc:150: if (bitmap.width() != bitmap.height()) On 2016/01/26 19:58:25, Devlin (Slow until 1-27) wrote: > On 2016/01/26 19:50:07, Evan Stade wrote: > > On 2016/01/26 19:08:48, Devlin (Slow until 1-27) wrote: > > > I wonder if we really *need* this to be a square. But we've always kind of > > > assumed it was, so this is fine for now. > > > > > > Could you add a > > > // TODO(devlin): Can we support rectangular icons? > > > > Why would we want to let developers do that? Is there any advantage? > > > > If we determine we need to scale the icon, we'll scale it to square, which > looks > > bad if it's changing dimensions. If we don't think we need to scale (based on > > width), we'll use it as is, which will look bad if height > width (the icon > > would extend below the 16x16 box it's supposed to be inside). > > Well, we can scale it to be proportionate and maintain the aspect ratio, and > then center the icon. This is already what any developer without a perfectly > square icon would do and then provide a square icon with a transparent > background, but we could save them that step. > > But, low priority. IMO, better not to save them that step. It adds code complexity and hurts performance.
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/1637763002/#ps80001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637763002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637763002/80001
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...)
Description was changed from ========== Fix bug in browser action icon scaling for non-1x scale factors. skia::ImageOperations::Resize needs pixels, and we were passing dp. Also, ignore the icon size key and just use the icon size itself. This fixes a bug (evidenced again by Shoptagr) where an icon size such as "19": "icons/19.png" // 19.png is actually 50x50 would be scaled incorrectly. At this point, the API should take a list rather than a dict, but it's probably not possible to change that and maintain backcompat. BUG=581232 ========== to ========== Fix bug in browser action icon scaling for non-1x scale factors. skia::ImageOperations::Resize needs pixels, and we were passing dp. Also, ignore the icon size key and just use the icon size itself. This fixes a bug (evidenced again by Shoptagr) where an icon size such as "19": "icons/19.png" // 19.png is actually 50x50 would be scaled incorrectly. At this point, the API should take a list rather than a dict, but it's probably not possible to change that and maintain backcompat. BUG=581232 TBR=benwells@chromium.org ==========
estade@chromium.org changed reviewers: + benwells@chromium.org
once again TBR benwells
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/1637763002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637763002/80001
Message was sent while issue was closed.
Description was changed from ========== Fix bug in browser action icon scaling for non-1x scale factors. skia::ImageOperations::Resize needs pixels, and we were passing dp. Also, ignore the icon size key and just use the icon size itself. This fixes a bug (evidenced again by Shoptagr) where an icon size such as "19": "icons/19.png" // 19.png is actually 50x50 would be scaled incorrectly. At this point, the API should take a list rather than a dict, but it's probably not possible to change that and maintain backcompat. BUG=581232 TBR=benwells@chromium.org ========== to ========== Fix bug in browser action icon scaling for non-1x scale factors. skia::ImageOperations::Resize needs pixels, and we were passing dp. Also, ignore the icon size key and just use the icon size itself. This fixes a bug (evidenced again by Shoptagr) where an icon size such as "19": "icons/19.png" // 19.png is actually 50x50 would be scaled incorrectly. At this point, the API should take a list rather than a dict, but it's probably not possible to change that and maintain backcompat. BUG=581232 TBR=benwells@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix bug in browser action icon scaling for non-1x scale factors. skia::ImageOperations::Resize needs pixels, and we were passing dp. Also, ignore the icon size key and just use the icon size itself. This fixes a bug (evidenced again by Shoptagr) where an icon size such as "19": "icons/19.png" // 19.png is actually 50x50 would be scaled incorrectly. At this point, the API should take a list rather than a dict, but it's probably not possible to change that and maintain backcompat. BUG=581232 TBR=benwells@chromium.org ========== to ========== Fix bug in browser action icon scaling for non-1x scale factors. skia::ImageOperations::Resize needs pixels, and we were passing dp. Also, ignore the icon size key and just use the icon size itself. This fixes a bug (evidenced again by Shoptagr) where an icon size such as "19": "icons/19.png" // 19.png is actually 50x50 would be scaled incorrectly. At this point, the API should take a list rather than a dict, but it's probably not possible to change that and maintain backcompat. BUG=581232 TBR=benwells@chromium.org Committed: https://crrev.com/52969d7388642a56447da090bf577974a6bf562f Cr-Commit-Position: refs/heads/master@{#371904} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/52969d7388642a56447da090bf577974a6bf562f Cr-Commit-Position: refs/heads/master@{#371904} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
