Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1025513004:
Ensure that the extension icons show up in the omnibox at fractional scales. (Closed)
|
Created:
5 years, 9 months ago by ananta Modified:
5 years, 9 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. |
DescriptionEnsure that the extension icons show up in the omnibox at fractional scales.
Proposed fix is to discard all unsupported representations (Read fractional scales) in the Extension icon image when
the icon is loaded. This ensures that stale caches are invalidated and the correct icons are loaded when the cache is regenerated.
BUG=461958
Committed: https://crrev.com/764a6e3d09a5e9043f72db15cca606d4e5467941
Cr-Commit-Position: refs/heads/master@{#322225}
Patch Set 1 #Patch Set 2 : Fix compile error #Patch Set 3 : Remove stale representations and add comment #
Total comments: 6
Patch Set 4 : Address oshima review comments #
Total comments: 2
Patch Set 5 : Remove unsupported image scales from the ImageSkia cache when we receive the OnImageLoaded notifica… #
Total comments: 2
Patch Set 6 : Move IsSupportedScale to ui\base\layout.cc/.h #
Total comments: 2
Patch Set 7 : Use g_supported_scale_factors to verify if the scale passed in is supported. #
Total comments: 2
Messages
Total messages: 37 (10 generated)
ananta@chromium.org changed reviewers: + oshima@chromium.org, pkotwicz@chromium.org
https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... File extensions/browser/extension_icon_image.cc (right): https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... extensions/browser/extension_icon_image.cc:228: // A better approach might be to set the |image_| member using the ImageSkia can you add TODO(oshima): here? https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... extensions/browser/extension_icon_image.cc:234: for (size_t rep_index = 0; rep_index < reps.size(); ++rep_index) { range based for-loop, and nuke {}
https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... File extensions/browser/extension_icon_image.cc (right): https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... extensions/browser/extension_icon_image.cc:236: } actually, you should be able to do: image_skia_ = gfx::ImageSkia(); to clear reps.
https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... File extensions/browser/extension_icon_image.cc (right): https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... extensions/browser/extension_icon_image.cc:228: // A better approach might be to set the |image_| member using the ImageSkia On 2015/03/24 01:19:05, oshima wrote: > can you add TODO(oshima): here? Done. https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... extensions/browser/extension_icon_image.cc:234: for (size_t rep_index = 0; rep_index < reps.size(); ++rep_index) { On 2015/03/24 01:19:05, oshima wrote: > range based for-loop, and nuke {} Removed the loop and replaced with image_skia_ = gfx::ImageSkia() https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... extensions/browser/extension_icon_image.cc:236: } On 2015/03/24 01:22:22, oshima wrote: > actually, you should be able to do: > > image_skia_ = gfx::ImageSkia(); > > to clear reps. Done.
On 2015/03/24 02:12:23, ananta wrote: > https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... > File extensions/browser/extension_icon_image.cc (right): > > https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... > extensions/browser/extension_icon_image.cc:228: // A better approach might be to > set the |image_| member using the ImageSkia > On 2015/03/24 01:19:05, oshima wrote: > > can you add TODO(oshima): here? > > Done. > > https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... > extensions/browser/extension_icon_image.cc:234: for (size_t rep_index = 0; > rep_index < reps.size(); ++rep_index) { > On 2015/03/24 01:19:05, oshima wrote: > > range based for-loop, and nuke {} > > Removed the loop and replaced with image_skia_ = gfx::ImageSkia() > > https://codereview.chromium.org/1025513004/diff/40001/extensions/browser/exte... > extensions/browser/extension_icon_image.cc:236: } > On 2015/03/24 01:22:22, oshima wrote: > > actually, you should be able to do: > > > > image_skia_ = gfx::ImageSkia(); > > > > to clear reps. > > Done. As per our discussion we now remove all unsupported scales from the ImageSkia cache when we receive the OnImageLoaded notification in the extension.
https://codereview.chromium.org/1025513004/diff/60001/extensions/browser/exte... File extensions/browser/extension_icon_image.cc (right): https://codereview.chromium.org/1025513004/diff/60001/extensions/browser/exte... extensions/browser/extension_icon_image.cc:234: image_skia_ = gfx::ImageSkia(); Drive by: Why don't you use the ImageSkia constructor which takes in a gfx::ImageSkiaRep? You'll also need to fix the unit tests
https://codereview.chromium.org/1025513004/diff/60001/extensions/browser/exte... File extensions/browser/extension_icon_image.cc (right): https://codereview.chromium.org/1025513004/diff/60001/extensions/browser/exte... extensions/browser/extension_icon_image.cc:234: image_skia_ = gfx::ImageSkia(); On 2015/03/24 21:43:08, pkotwicz wrote: > Drive by: Why don't you use the ImageSkia constructor which takes in a > gfx::ImageSkiaRep? > > You'll also need to fix the unit tests Please take a peek at the updated patch. We cannot nuke the image_skia_ instance using either ctor. This sets the source_ to NULL which causes subsequent scaled image requests to break leading to images getting displayed distorted.
https://codereview.chromium.org/1025513004/diff/80001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/1025513004/diff/80001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:321: bool ImageSkia::IsSupportedScale(float scale) { can you move this to ui/base/layout.h ?
https://codereview.chromium.org/1025513004/diff/80001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/1025513004/diff/80001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:321: bool ImageSkia::IsSupportedScale(float scale) { On 2015/03/24 21:57:01, oshima wrote: > can you move this to ui/base/layout.h ? Done.
lgtm
I will take Oshima's word. It would be nice if users of ImageSkia::AddRepresentation() did not need to know about the inner workings of ImageSkia
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1025513004/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...)
ananta@chromium.org changed reviewers: + yoz@chromium.org
+yoz for extensions owners
LGTM for extensions
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1025513004/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...)
ananta@chromium.org changed reviewers: + sky@chromium.org
+sky for ui/base owners
https://codereview.chromium.org/1025513004/diff/100001/ui/base/layout.cc File ui/base/layout.cc (right): https://codereview.chromium.org/1025513004/diff/100001/ui/base/layout.cc#newc... ui/base/layout.cc:97: gfx::ImageSkia::GetSupportedScales(); Why are you using ImageSkia and not g_supported_scale_factors?
https://codereview.chromium.org/1025513004/diff/100001/ui/base/layout.cc File ui/base/layout.cc (right): https://codereview.chromium.org/1025513004/diff/100001/ui/base/layout.cc#newc... ui/base/layout.cc:97: gfx::ImageSkia::GetSupportedScales(); On 2015/03/25 14:49:10, sky wrote: > Why are you using ImageSkia and not g_supported_scale_factors? Done.
LGTM
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, yoz@chromium.org Link to the patchset: https://codereview.chromium.org/1025513004/#ps120001 (title: "Use g_supported_scale_factors to verify if the scale passed in is supported.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1025513004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/764a6e3d09a5e9043f72db15cca606d4e5467941 Cr-Commit-Position: refs/heads/master@{#322225}
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
Minor variable declaration suggestion from /analyze warning. https://codereview.chromium.org/1025513004/diff/120001/extensions/browser/ext... File extensions/browser/extension_icon_image.cc (right): https://codereview.chromium.org/1025513004/diff/120001/extensions/browser/ext... extensions/browser/extension_icon_image.cc:237: for (const auto rep : reps) { The declaration of 'rep' should, I believe, be "const auto& rep" in order to avoid a gratuitous copy of the ImageSkiaRep object. The copy isn't super expensive, but still probably worth avoiding. Also, 'rep' shadows the same-named variable in the outer scope. This isn't a bug but it can lead to confusion about which variable was intended. Consider renaming the inner 'rep'? I noticed this because of a new variable shadowing warning from /analyze.
Message was sent while issue was closed.
https://codereview.chromium.org/1025513004/diff/120001/extensions/browser/ext... File extensions/browser/extension_icon_image.cc (right): https://codereview.chromium.org/1025513004/diff/120001/extensions/browser/ext... extensions/browser/extension_icon_image.cc:237: for (const auto rep : reps) { On 2015/03/26 18:44:37, brucedawson wrote: > The declaration of 'rep' should, I believe, be "const auto& rep" in order to > avoid a gratuitous copy of the ImageSkiaRep object. The copy isn't super > expensive, but still probably worth avoiding. > > Also, 'rep' shadows the same-named variable in the outer scope. This isn't a bug > but it can lead to confusion about which variable was intended. Consider > renaming the inner 'rep'? I noticed this because of a new variable shadowing > warning from /analyze. Will address these in a followup Thanks Ananta |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
