Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2665623002:
Fix Jumplist favicons to have high resolution in HDPI Windows displays (Closed)
DescriptionFix Jumplist favicons to have high-resolution in HDPI Windows displays
On a HDPI windows machine, if right-click Chrome in the taskbar, the
jumplist favicons are not displayed in high-resolution. They look
blurry, different from other favicons in the rest of Chrome, e.g., tabs.
As reported, Internet Explorer doesn't have this issue.
The root cause is that 1x bitmap (16x16) is always used to create
jumplist icon file by the current jumplist implementation.
This CL fixes this issue. The idea is that for all the display scales
supported by the platform, get/compute all corresponding bitmaps and
write them to the disk. In this way, the appropriate icon bitmap is used
when a user changes the screen display scale.
BUG=676901
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Review-Url: https://codereview.chromium.org/2665623002
Cr-Commit-Position: refs/heads/master@{#449134}
Committed: https://chromium.googlesource.com/chromium/src/+/ae6956325bca345ff31425adc8597d33e45ce3f7
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add faviconSize parameter to GetFaviconImageForPageURL() #
Total comments: 4
Patch Set 3 : Resize large favicon to 32x32 #Patch Set 4 : Use 2x bitmap if any monitor is not at 1x. #Patch Set 5 : Update comments and remove redundant .h files. #
Total comments: 6
Patch Set 6 : Make code local to jumplist. #Patch Set 7 : Get bitmap of suitable size. #
Total comments: 3
Patch Set 8 : Remove superflous checks. #Patch Set 9 : Write all bitmaps needed to the disk. #
Total comments: 10
Patch Set 10 : Use gfx::ImageSkia in ShellLinkItem for thread safety. #
Total comments: 2
Patch Set 11 : Better function names and parameters. #
Messages
Total messages: 120 (83 generated)
Description was changed from ========== fix build Make jumplist icon high res in high DPI Windows display. BUG=676901 ========== to ========== fix build Make jumplist icon high res in high DPI Windows display. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by chengx@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...
Description was changed from ========== fix build Make jumplist icon high res in high DPI Windows display. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix Jumplist favicon to have high resolution in HDPI Windows displays On a HDPI windows machine, if right click Chrome in the taskbar, the jumplist favicons are not displayed in high resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. This CL fixes this bug. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix Jumplist favicon to have high resolution in HDPI Windows displays On a HDPI windows machine, if right click Chrome in the taskbar, the jumplist favicons are not displayed in high resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. This CL fixes this bug. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix Jumplist favicons to have high resolution in HDPI Windows displays On a HDPI windows machine, if right click Chrome in the taskbar, the jumplist favicons are not displayed in high resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. This CL fixes this bug. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
chengx@chromium.org changed reviewers: + grt@chromium.org
This CL is ready for review. PTAL~
This looks okay to me from a stylistic standpoint, but I'm not sure about the PGNness; see below. https://codereview.chromium.org/2665623002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2665623002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:547: gfx::Image::CreateFrom1xPNGBytes(image_result.bitmap_data) Is it necessarily the case that the favicon bitmap_data is a PNG? I don't see this documented. Please ask a favicon owner to review.
chengx@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, I notice that you are the owner of favicon_base, so could you please review this CL? I have asked the jumplist file owner, Greg (grt@), to review this CL, but he said he would defer to the favicon owner on this. Thanks!
https://codereview.chromium.org/2665623002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2665623002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:526: url, favicon_base::FAVICON, 0, Supplying 0 for a size may result in a large sized image. Are you sure you don't want to resize the image below?
Please see my replies to your comments. Thanks! https://codereview.chromium.org/2665623002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2665623002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:526: url, favicon_base::FAVICON, 0, On 2017/01/30 23:56:11, sky wrote: > Supplying 0 for a size may result in a large sized image. Are you sure you don't > want to resize the image below? The original API, i.e., GetFaviconImageForPageURL, returns favicons of a size of 16x16 (favicon_size.cc), which looks blurry in HDPI displays. While 'GetRawFaviconForPageURL' with its 3rd parameter supplied with 0 will return the largest favicon bitmap as documented. I have locally built and launched chrome, and the jumplist favicons are all displayed in the same size in HDPI displays, but in higher resolution. To my best knowledge from local debugging, the new favicons are of 32x32, which look sharper than the old 16x16 favicons. However, 32x32 ones also work for non-HDPI displays. So I think this CL works fine. https://codereview.chromium.org/2665623002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:547: gfx::Image::CreateFrom1xPNGBytes(image_result.bitmap_data) On 2017/01/30 10:02:40, grt (UTC plus 1) wrote: > Is it necessarily the case that the favicon bitmap_data is a PNG? I don't see > this documented. Please ask a favicon owner to review. Sure.
There is no guarantee that you'll get the favicon at 32x32, it may be much bigger and if it's much bigger won't we needlessly be eating memory?
The CQ bit was checked by chengx@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...
On 2017/01/31 05:06:55, sky wrote: > There is no guarantee that you'll get the favicon at 32x32, it may be much > bigger and if it's much bigger won't we needlessly be eating memory? To emphasize this point, a little Google search tells me that 180x180 is recommended by Apple for its <link> assets for touch purposes. My question about CreateFrom1xPNGBytes stands: does Chrome transform whatever it found when it resolved the <link> tag into a png? I think browsers support a variety of image formats, so is CreateFrom1xPNGBytes going to do the right thing for all sites?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yes, in history everything is mapped to a png. -Scott On Mon, Jan 30, 2017 at 11:47 PM, <grt@chromium.org> wrote: > On 2017/01/31 05:06:55, sky wrote: >> There is no guarantee that you'll get the favicon at 32x32, it may be much >> bigger and if it's much bigger won't we needlessly be eating memory? > > To emphasize this point, a little Google search tells me that 180x180 is > recommended by Apple for its <link> assets for touch purposes. > > My question about CreateFrom1xPNGBytes stands: does Chrome transform > whatever it > found when it resolved the <link> tag into a png? I think browsers support a > variety of image formats, so is CreateFrom1xPNGBytes going to do the right > thing > for all sites? > > https://codereview.chromium.org/2665623002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/01/31 05:06:55, sky wrote: > There is no guarantee that you'll get the favicon at 32x32, it may be much > bigger and if it's much bigger won't we needlessly be eating memory? Thanks for the reminder. Firstly, I think I can change the 3rd parameter of GetRawFaviconForPageURL(), i.e., desired_size_in_dip, to 32, which ensures we won't have larger images. However, I think it might be better to upgrade GetFaviconImageForPageURL() API a bit. That is, add a 4th parameter, i.e., const int faviconSize, whose default value is gfx::kFaviconSize (16x16) with another option to be gfx::kHdpiFaviconSize (32x32). I have uploaded another patch w.r.t this second idea. PTAL~
I searched CreateFrom1xPNGBytes() in chromium code base. It seems that it is pretty common that the PNG API is used just to get a bitmap. For example: SkBitmap bitmap = gfx::Image::CreateFrom1xPNGBytes(encoded_image->front(), encoded_image->size()).AsBitmap(); On 2017/01/31 18:12:58, sky wrote: > Yes, in history everything is mapped to a png. > > -Scott > > On Mon, Jan 30, 2017 at 11:47 PM, <mailto:grt@chromium.org> wrote: > > On 2017/01/31 05:06:55, sky wrote: > > My question about CreateFrom1xPNGBytes stands: does Chrome transform > > whatever it > > found when it resolved the <link> tag into a png? I think browsers support a > > variety of image formats, so is CreateFrom1xPNGBytes going to do the right > > thing > > for all sites?
https://codereview.chromium.org/2665623002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2665623002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:108: const int faviconSize = gfx::kFaviconSize); You shouldn't need to specify a size. The gfx::Image will contain multiple images if possible so that you can grab the one appropriate based on the scale. https://codereview.chromium.org/2665623002/diff/20001/ui/gfx/favicon_size.cc File ui/gfx/favicon_size.cc (right): https://codereview.chromium.org/2665623002/diff/20001/ui/gfx/favicon_size.cc#... ui/gfx/favicon_size.cc:11: const int kHdpiFaviconSize = 32; This isn't entirely accurate. High dpi generally means any scale other than 1x. Don't add this constant here.
The CQ bit was checked by chengx@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.
Thanks for the feedback! Please allow me to briefly explain how the jumplist code is dealing with the favicon now. As in jumplist.cc line 525, GetFaviconImageForPageURL() is used to get a FaviconImageResult, which contains gfx::Image. This image has a size of gfx::kFaviconSize (16x16) in DIP coordinate system. In addition, this image contains 2 a bitmap, whose scale factors are 1.0 and 2.0, respectively. Say they are 1x bitmap and 2x bitmap. Out of these 2 bitmaps, only the 1x one is used as in jumplist.cc line 547 (image_result.image.AsBitmap()). The AsBitmap() returns the 1x bitmap. The bitmap is then used to create an icon file using CreateIconFileFromImageFamily() in lumplist.cc line 90. This explains why the current jumplist icons have a size of 16x16. In the 2nd patch of this CL, I used gfx::kHdipFaviconSize (32) to replace gfx::kFaviconSize (16). This explains why the 2nd patch "works" somehow. I understand gfx::Image has multiple bitmaps for different scales. Apart from only using the 1x bitmap, there are two other options obviously: 1) Using both of them. However, since the jumplist icons need to be written to the disk, I suspect we don't want to do this to increase disk IO. 2) Using the 2x bitmap. We don't have a API for this purpose yet in image.cc. But, we can walk around this without adding new APIs. See below. In the 1st patch of this CL, I used GetRawFaviconForPageURL() to replace GetFaviconImageForPageURL(), and supply its 3rd parameter with 0 to get the largest favicon. You are right, it wastes memory when the fetched icon is larger than 32x32. To fix this, I have uploaded a 3rd patch, where I stick with GetRawFaviconForPageURL(), but supply its 3rd parameter with 32 to get favicons with a size of 32x32 or the closest. As documented, if the 32x32 one is unavailable, the 16x16 one will be fetched. I also added code to downsample the favicon to 32x32 if the fetched one is larger than 32x32. Please take a look of the new patch! Thanks! On 2017/01/31 23:28:45, sky wrote: > https://codereview.chromium.org/2665623002/diff/20001/components/favicon/core... > File components/favicon/core/favicon_service.h (right): > > https://codereview.chromium.org/2665623002/diff/20001/components/favicon/core... > components/favicon/core/favicon_service.h:108: const int faviconSize = > gfx::kFaviconSize); > You shouldn't need to specify a size. The gfx::Image will contain multiple > images if possible so that you can grab the one appropriate based on the scale. > > https://codereview.chromium.org/2665623002/diff/20001/ui/gfx/favicon_size.cc > File ui/gfx/favicon_size.cc (right): > > https://codereview.chromium.org/2665623002/diff/20001/ui/gfx/favicon_size.cc#... > ui/gfx/favicon_size.cc:11: const int kHdpiFaviconSize = 32; > This isn't entirely accurate. High dpi generally means any scale other than 1x. > Don't add this constant here.
grt@chromium.org changed reviewers: - grt@chromium.org
Removing myself from R list, as sky@ is clearly in a better position to review this than I am. Thanks, Scott.
Thanks for the detailed analysis. I think you should continue using GetFaviconImageForPageURL and extract the image you care about. By that I mean if the scale is 1x, use the 1x image, if the scale is > 1 use the 2x image. Determining the scale may be tricky. I assume you want to use the 2x if *any* monitor is not at 1x.
The CQ bit was checked by chengx@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.
The CQ bit was checked by chengx@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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by chengx@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 checked by chengx@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...
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Description was changed from ========== Fix Jumplist favicons to have high resolution in HDPI Windows displays On a HDPI windows machine, if right click Chrome in the taskbar, the jumplist favicons are not displayed in high resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. This CL fixes this bug. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix Jumplist favicons to have high resolution in HDPI Windows displays On a HDPI windows machine, if right click Chrome in the taskbar, the jumplist favicons are not displayed in high resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used for jumplist icon by the current jumplist implementation, regardless of the device scale factor(s). This CL fixes this issue. The strategy is that before the jumplist icon file is created, the largest device scale factor of all the monitors is looked up. If the value is greater than 1.0, then 2x bitmap (32x32) is used, otherwise 1x bitmap (16x16) is used. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Patchset #4 (id:100001) has been deleted
I agree this is the right way to fix this issue. I have implemented this idea in the new patch. PTAL~ On 2017/02/01 16:48:57, sky wrote: > Thanks for the detailed analysis. > I think you should continue using GetFaviconImageForPageURL and extract the > image you care about. By that I mean if the scale is 1x, use the 1x image, if > the scale is > 1 use the 2x image. Determining the scale may be tricky. I assume > you want to use the 2x if *any* monitor is not at 1x.
https://codereview.chromium.org/2665623002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2665623002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:108: const int faviconSize = gfx::kFaviconSize); On 2017/01/31 23:28:44, sky wrote: > You shouldn't need to specify a size. The gfx::Image will contain multiple > images if possible so that you can grab the one appropriate based on the scale. Acknowledged. https://codereview.chromium.org/2665623002/diff/20001/ui/gfx/favicon_size.cc File ui/gfx/favicon_size.cc (right): https://codereview.chromium.org/2665623002/diff/20001/ui/gfx/favicon_size.cc#... ui/gfx/favicon_size.cc:11: const int kHdpiFaviconSize = 32; On 2017/01/31 23:28:45, sky wrote: > This isn't entirely accurate. High dpi generally means any scale other than 1x. > Don't add this constant here. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@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 checked by chengx@chromium.org to run a CQ dry run
Patchset #5 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please try to keep the changes specific to jumplist. It is the only place that needs the code. Keeping api used by lots of people (Screen and Image/ImageSkia) to a minimum helps reduce the cognitive overhead of using the classes. https://codereview.chromium.org/2665623002/diff/160001/ui/display/win/screen_... File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2665623002/diff/160001/ui/display/win/screen_... ui/display/win/screen_win.cc:355: float ScreenWin::GetLargestDeviceScaleFactor() { This function is only needed in one place, can you move it there? The code calling this can get all the displays and iterate over it there, right? https://codereview.chromium.org/2665623002/diff/160001/ui/display/win/screen_... ui/display/win/screen_win.cc:360: for (size_t i = 0; i < displays.size(); i++) { no {}, also, I would use enhanced for loop https://codereview.chromium.org/2665623002/diff/160001/ui/gfx/image/image_skia.h File ui/gfx/image/image_skia.h (right): https://codereview.chromium.org/2665623002/diff/160001/ui/gfx/image/image_ski... ui/gfx/image/image_skia.h:106: bool HasRepresentation(float scale) const; Can't you use HasRepresentation and GetRepresentation?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@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...
Patchset #6 (id:180001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the feedback! I have made the code local to jumplist. Now jumplist.cc is the only file touched by this CL. PTAL~ A few small points: 1) I am not using imageskia->HasRepresentation() as it always returns false in my case. Instead, I only use GetRepresentation(). Please see comments below. 2) I feel it maybe unnecessary to also create a helper function called GetLargestDeviceScaleFactor() in jumplist.cc, which was created in screen_win.h. So I just copied the body of this function in JumpList::OnFaviconDataAvailable(). https://codereview.chromium.org/2665623002/diff/160001/ui/display/win/screen_... File ui/display/win/screen_win.cc (right): https://codereview.chromium.org/2665623002/diff/160001/ui/display/win/screen_... ui/display/win/screen_win.cc:355: float ScreenWin::GetLargestDeviceScaleFactor() { On 2017/02/02 22:41:16, sky wrote: > This function is only needed in one place, can you move it there? The code > calling this can get all the displays and iterate over it there, right? Sure. Will make the code local to jumplist. https://codereview.chromium.org/2665623002/diff/160001/ui/display/win/screen_... ui/display/win/screen_win.cc:360: for (size_t i = 0; i < displays.size(); i++) { On 2017/02/02 22:41:16, sky wrote: > no {}, also, I would use enhanced for loop Done. https://codereview.chromium.org/2665623002/diff/160001/ui/gfx/image/image_skia.h File ui/gfx/image/image_skia.h (right): https://codereview.chromium.org/2665623002/diff/160001/ui/gfx/image/image_ski... ui/gfx/image/image_skia.h:106: bool HasRepresentation(float scale) const; On 2017/02/02 22:41:16, sky wrote: > Can't you use HasRepresentation and GetRepresentation? HasRepresentation() doesn't work here as it calls storage_->FindRepresentation(scale, false) with the 2nd parameter set to "false" within itself. As a result, it always returns false which makes it useless. I use GetRepresentation() only, which works. See the new patch uploaded please.
On 2017/02/03 01:51:13, chengx wrote: > Thanks for the feedback! I have made the code local to jumplist. Now jumplist.cc > is the only file touched by this CL. PTAL~ > > A few small points: > 1) I am not using imageskia->HasRepresentation() as it always returns false in > my case. Instead, I only use GetRepresentation(). Please see comments below. > 2) I feel it maybe unnecessary to also create a helper function called > GetLargestDeviceScaleFactor() in jumplist.cc, which was created in screen_win.h. > So I just copied the body of this function in > JumpList::OnFaviconDataAvailable(). > > https://codereview.chromium.org/2665623002/diff/160001/ui/display/win/screen_... > File ui/display/win/screen_win.cc (right): > > https://codereview.chromium.org/2665623002/diff/160001/ui/display/win/screen_... > ui/display/win/screen_win.cc:355: float ScreenWin::GetLargestDeviceScaleFactor() > { > On 2017/02/02 22:41:16, sky wrote: > > This function is only needed in one place, can you move it there? The code > > calling this can get all the displays and iterate over it there, right? > > Sure. Will make the code local to jumplist. > > https://codereview.chromium.org/2665623002/diff/160001/ui/display/win/screen_... > ui/display/win/screen_win.cc:360: for (size_t i = 0; i < displays.size(); i++) { > On 2017/02/02 22:41:16, sky wrote: > > no {}, also, I would use enhanced for loop > > Done. > > https://codereview.chromium.org/2665623002/diff/160001/ui/gfx/image/image_skia.h > File ui/gfx/image/image_skia.h (right): > > https://codereview.chromium.org/2665623002/diff/160001/ui/gfx/image/image_ski... > ui/gfx/image/image_skia.h:106: bool HasRepresentation(float scale) const; > On 2017/02/02 22:41:16, sky wrote: > > Can't you use HasRepresentation and GetRepresentation? > > HasRepresentation() doesn't work here as it calls > storage_->FindRepresentation(scale, false) with the 2nd parameter set to "false" > within itself. As a result, it always returns false which makes it useless. I > use GetRepresentation() only, which works. See the new patch uploaded please. You want to hit this line of code: https://chromium.googlesource.com/chromium/src/+/master/ui/gfx/image/image_sk... If you are not hitting that line of code, then the image is going to be scaled. I think you're saying you don't really care if the image is scaled though, which makes sense. One other comment about the expectation. Are you sure you want to use 1x and 2x and not the biggest scale? If a user has a monitor at 2.5x should use that scale?
The CQ bit was checked by chengx@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 checked by chengx@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 checked by chengx@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...
Patchset #7 (id:220001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The line of code "if (it->scale() == scale)" as you mentioned will be tested as GetRepresentation() will call FindRepresentation(). However, it may not hold (say it->scale() = 2 and scale = 1.75) and the image is going to be scaled. I think this is fine. As to the expectation, I think you are right. Sorry I didn't think deep into this. It turns out that FindRepresentation() handles this pretty well, so a single FindRepresentation(scale) simply works. The line of code as in https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia.cc?l=227 makes sure that the bitmap of size "std::ceil(scale-0.2)" will be used. For example, if scale is 1.25 or 1.5 or 1.75 or 2, 2x bitmap is used. If scale is greater than 2.2, then the smallest bitmap that is larger than 2x will be used, if there is one. I have uploaded another patch. PTAL~ Sorry for so many patches I have uploaded for you to review. I just started fixing UI bugs. One important lesson I learned is that there can be quite a few approaches that can "technically" fix the bug, and it is quite tempting to send out a code review for the current best solution. However, any approach but the optimal one looks messy. It does require a lot of patience to dig into more related code, which is necessary to figure out the best solution. > You want to hit this line of code: > https://chromium.googlesource.com/chromium/src/+/master/ui/gfx/image/image_sk... > If you are not hitting that line of code, then the image is going to be scaled. > I think you're saying you don't really care if the image is scaled though, which > makes sense. > One other comment about the expectation. Are you sure you want to use 1x and 2x > and not the biggest scale? If a user has a monitor at 2.5x should use that > scale?
Description was changed from ========== Fix Jumplist favicons to have high resolution in HDPI Windows displays On a HDPI windows machine, if right click Chrome in the taskbar, the jumplist favicons are not displayed in high resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used for jumplist icon by the current jumplist implementation, regardless of the device scale factor(s). This CL fixes this issue. The strategy is that before the jumplist icon file is created, the largest device scale factor of all the monitors is looked up. If the value is greater than 1.0, then 2x bitmap (32x32) is used, otherwise 1x bitmap (16x16) is used. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix Jumplist favicons to have high resolution in HDPI Windows displays On a HDPI windows machine, if right click Chrome in the taskbar, the jumplist favicons are not displayed in high resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used for jumplist icon by the current jumplist implementation, regardless of the device scale factor(s). This CL fixes this issue. The strategy is that before the jumplist icon file is created, the largest device scale factor of all the monitors is looked up. The appropriate bitmap w.r.t the scale is used then. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
LGTM with the following. No worries about the number of changes, it happens. https://codereview.chromium.org/2665623002/diff/240001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2665623002/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:549: const gfx::ImageSkia* imageskia = image_result.image.ToImageSkia(); The check you want is if (!image_result.image.IsEmpty()) { } The docs describe this.
Please see my reply to your comment below. I am a little confused though. https://codereview.chromium.org/2665623002/diff/240001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2665623002/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:549: const gfx::ImageSkia* imageskia = image_result.image.ToImageSkia(); On 2017/02/04 00:01:59, sky wrote: > The check you want is > if (!image_result.image.IsEmpty()) { > } > > The docs describe this. This check is already used in Line 547 above. Or do you mean Line 550? Although imageskia should not be null as image_result.image is not empty now, but I still feel like adding a null pointer check in Line 550 makes things safe.
I do have another question about this. What does the underlying API we're using say about high dpi? CreateIconFile() in jumplist.cc uses an ImageFamily, should we dump all the images we have to it, not just the 1x? That seems safest. https://codereview.chromium.org/2665623002/diff/240001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2665623002/diff/240001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:549: const gfx::ImageSkia* imageskia = image_result.image.ToImageSkia(); On 2017/02/04 00:18:59, chengx wrote: > On 2017/02/04 00:01:59, sky wrote: > > The check you want is > > if (!image_result.image.IsEmpty()) { > > } > > > > The docs describe this. > > This check is already used in Line 547 above. Or do you mean Line 550? Although > imageskia should not be null as image_result.image is not empty now, but I still > feel like adding a null pointer check in Line 550 makes things safe. Because of the IsEmpty() check you shouldn't need the null check. We generally don't add superflous checks, if you feel strongly use a DCHECK.
The CQ bit was checked by chengx@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.
Please see my replies below. > I do have another question about this. What does the underlying API we're using > say about high dpi? CreateIconFile() in jumplist.cc uses an ImageFamily, should > we dump all the images we have to it, not just the 1x? That seems safest. Actually, I have thought about this before thinking of how to determine the device scale factor in patch 4. Here is what I got. The returned "image_result" as in https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist.cc?q=jumplis... contains a gfx::image, which itself may contain several bitmaps. We can pass this gfx::image to CreateIconFile() directly, rather than a SKBitmap. To do this, we need to make the ShellLinkItem hold a gfx::image rather than a SKBitmap as in https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist_updater.h?ty... Then we can make an ImageFamily object using this gfx::image in 2 possible ways. 1) Push the gfx::image into ImageFamily directly. However, this doesn't work. I mean the jumplist icon is still of low resolution when the device scale factor is greater than 100%. This has something to do with the rendering code or even Windows code. I am not sure yet. However, it won't increase the disk IO. An icon file is still 28k when I ran it locally. 2) Call gfx::Image::CreateFrom1xBitmap() to create an gfx::Image for every bitmap contained by the original gfx::image, then push each gfx::Image just created into ImageFamily. I think this will increase disk IO for sure as we have different gfx::Image objects. Also, it contradicts to the idea that gfx::Image can hold different bitmaps. On the other hand, I think the current implementation is safe enough to get the bitmap of the correct resolution for the current device scale factor. The gfx::ImageSkia.GetRepresentation() API does a pretty good job for this purpose. The only issue is that when an user zoom in the screen to 200% from 100%, the jumplist code needs to be hit once to get the high resolution image. However, considering that the code can be simply hit by closing one tab, I don't think this is a big issue. > Because of the IsEmpty() check you shouldn't need the null check. We generally > don't add superflous checks, if you feel strongly use a DCHECK. Okay, I switched to DCHECK.
On Mon, Feb 6, 2017 at 5:38 PM, <chengx@chromium.org> wrote: > Please see my replies below. > >> I do have another question about this. What does the underlying API we're > using >> say about high dpi? CreateIconFile() in jumplist.cc uses an ImageFamily, > should >> we dump all the images we have to it, not just the 1x? That seems safest. > > Actually, I have thought about this before thinking of how to determine the > device scale factor in patch 4. Here is what I got. The returned > "image_result" > as in > https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist.cc?q=jumplis... > contains a gfx::image, which itself may contain several bitmaps. We can pass > this gfx::image to CreateIconFile() directly, rather than a SKBitmap. To do > this, we need to make the ShellLinkItem hold a gfx::image rather than a > SKBitmap > as in > https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist_updater.h?ty... > > Then we can make an ImageFamily object using this gfx::image in 2 possible > ways. > 1) Push the gfx::image into ImageFamily directly. However, this doesn't > work. I > mean the jumplist icon is still of low resolution when the device scale > factor > is greater than 100%. This has something to do with the rendering code or > even > Windows code. I am not sure yet. However, it won't increase the disk IO. An > icon > file is still 28k when I ran it locally. Are you sure the code was processing multiple items correctly? > 2) Call gfx::Image::CreateFrom1xBitmap() to create an gfx::Image for every > bitmap contained by the original gfx::image, then push each gfx::Image just > created into ImageFamily. I think this will increase disk IO for sure as we > have > different gfx::Image objects. Also, it contradicts to the idea that > gfx::Image > can hold different bitmaps. This seems like a total hack. > On the other hand, I think the current implementation is safe enough to get > the > bitmap of the correct resolution for the current device scale factor. The > gfx::ImageSkia.GetRepresentation() API does a pretty good job for this > purpose. > The only issue is that when an user zoom in the screen to 200% from 100%, > the > jumplist code needs to be hit once to get the high resolution image. > However, > considering that the code can be simply hit by closing one tab, I don't > think > this is a big issue. > >> Because of the IsEmpty() check you shouldn't need the null check. We >> generally >> don't add superflous checks, if you feel strongly use a DCHECK. > > Okay, I switched to DCHECK. > > > > https://codereview.chromium.org/2665623002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> > Actually, I have thought about this before thinking of how to determine the > > device scale factor in patch 4. Here is what I got. The returned "image_result" > > as in https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist.cc?q=jumplis... > > contains a gfx::image, which itself may contain several bitmaps. We can pass > > this gfx::image to CreateIconFile() directly, rather than a SKBitmap. To do > > this, we need to make the ShellLinkItem hold a gfx::image rather than a > > SKBitmap > > as in https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist_updater.h?ty... > > > > Then we can make an ImageFamily object using this gfx::image in 2 possible ways. > > 1) Push the gfx::image into ImageFamily directly. However, this doesn't > > work. I mean the jumplist icon is still of low resolution when the device scale > > factor is greater than 100%. This has something to do with the rendering code or > > even Windows code. I am not sure yet. However, it won't increase the disk IO. An > > icon file is still 28k when I ran it locally. > > Are you sure the code was processing multiple items correctly? Here is the reason why it is not working if we push the gfx::image directly into the ImageFamily. The CreateIconFile() API calls IconUtil::CreateIconFileFromImageFamily() as in https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist.cc?q=jumplis... which itself calls ConvertImageFamilyToBitmaps() to convert images into bitmaps. However, for each gfx::image in the ImageFamily, only its 1x bitmap is used for "converting" to the bitmap, even though the gfx::image can contain multiple bitmaps. This conversion is in https://cs.chromium.org/chromium/src/ui/gfx/icon_util.cc?q=ConvertImageFamily... You can see "SkBitmap bitmap = image.AsBitmap();" here, where AsBitmap() returns the 1x bitmap. That is why I mentioned the 2nd method below. We need to extract all bitmaps from a gfx::image and make multiple gfx::images using them. It may work but looks like very hacky. In short, the issue is that if gfx::image.AsBitmap() is used, we are only using the 1x bitmap in the gfx::image. > > 2) Call gfx::Image::CreateFrom1xBitmap() to create an gfx::Image for every > > bitmap contained by the original gfx::image, then push each gfx::Image just > > created into ImageFamily. I think this will increase disk IO for sure as we > > have > > different gfx::Image objects. Also, it contradicts to the idea that > > gfx::Image > > can hold different bitmaps. > > This seems like a total hack.
On 2017/02/07 08:43:28, chengx wrote: > > > Actually, I have thought about this before thinking of how to determine the > > > device scale factor in patch 4. Here is what I got. The returned > "image_result" > > > as in > https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist.cc?q=jumplis... > > > contains a gfx::image, which itself may contain several bitmaps. We can pass > > > this gfx::image to CreateIconFile() directly, rather than a SKBitmap. To do > > > this, we need to make the ShellLinkItem hold a gfx::image rather than a > > > SKBitmap > > > as in > https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist_updater.h?ty... > > > > > > Then we can make an ImageFamily object using this gfx::image in 2 possible > ways. > > > 1) Push the gfx::image into ImageFamily directly. However, this doesn't > > > work. I mean the jumplist icon is still of low resolution when the device > scale > > > factor is greater than 100%. This has something to do with the rendering > code or > > > even Windows code. I am not sure yet. However, it won't increase the disk > IO. An > > > icon file is still 28k when I ran it locally. > > > > Are you sure the code was processing multiple items correctly? > > Here is the reason why it is not working if we push the gfx::image directly into > the ImageFamily. The CreateIconFile() API calls > IconUtil::CreateIconFileFromImageFamily() as in > https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist.cc?q=jumplis... > which itself calls ConvertImageFamilyToBitmaps() to convert images into bitmaps. > However, for each gfx::image in the ImageFamily, only its 1x bitmap is used for > "converting" to the bitmap, even though the gfx::image can contain multiple > bitmaps. This conversion is in > https://cs.chromium.org/chromium/src/ui/gfx/icon_util.cc?q=ConvertImageFamily... > You can see "SkBitmap bitmap = image.AsBitmap();" here, where AsBitmap() returns > the 1x bitmap. > > That is why I mentioned the 2nd method below. We need to extract all bitmaps > from a gfx::image and make multiple gfx::images using them. It may work but > looks like very hacky. > > In short, the issue is that if gfx::image.AsBitmap() is used, we are only using > the 1x bitmap in the gfx::image. > > > > 2) Call gfx::Image::CreateFrom1xBitmap() to create an gfx::Image for every > > > bitmap contained by the original gfx::image, then push each gfx::Image just > > > created into ImageFamily. I think this will increase disk IO for sure as we > > > have > > > different gfx::Image objects. Also, it contradicts to the idea that > > > gfx::Image > > > can hold different bitmaps. > > > > This seems like a total hack. According to the docs ImageFamily isn't meant to be used for storing images at multiple dpi. Image/ImageSkia is for that. On a quick glance it looks like BuildResizedImageFamily() seems to be highly tuned toward windows ico files, and I'm not sure it's really appropriate for your uses. But again, I don't think you really answered my questions. What *should* we be doing here? What does IE/Firefox do when running in high dpi? What happens at different scales and after you've cleared caches so that you know you're getting fresh behavior? Are there any docs on this?
> According to the docs ImageFamily isn't meant to be used for storing images at > multiple dpi. Image/ImageSkia is for that. On a quick glance it looks like > BuildResizedImageFamily() seems to be highly tuned toward windows ico files, and > I'm not sure it's really appropriate for your uses. But again, I don't think you > really answered my questions. What *should* we be doing here? What does > IE/Firefox do when running in high dpi? What happens at different scales and > after you've cleared caches so that you know you're getting fresh behavior? Are > there any docs on this? Thanks for the reply. Firefox's jumplist also has this low resolution issue for HDPI displays. For IE/EDGE, I think they have disabled jumplists. Right clicking IE/EDGE icons in the task bar doesn't show any jumplist at all. I also tried to search relevant docs for IE/EDGE, but no luck till now. Thanks!
Is there any documentation of the api we're using? I seem to remember the ability to change whether jumplists are used. Is there an option you can use to turn it on? -Scott On Tue, Feb 7, 2017 at 1:04 PM, <chengx@chromium.org> wrote: >> According to the docs ImageFamily isn't meant to be used for storing >> images at >> multiple dpi. Image/ImageSkia is for that. On a quick glance it looks like >> BuildResizedImageFamily() seems to be highly tuned toward windows ico >> files, > and >> I'm not sure it's really appropriate for your uses. But again, I don't >> think > you >> really answered my questions. What *should* we be doing here? What does >> IE/Firefox do when running in high dpi? What happens at different scales >> and >> after you've cleared caches so that you know you're getting fresh >> behavior? > Are >> there any docs on this? > > Thanks for the reply. Firefox's jumplist also has this low resolution issue > for > HDPI displays. For IE/EDGE, I think they have disabled jumplists. Right > clicking > IE/EDGE icons in the task bar doesn't show any jumplist at all. > > I also tried to search relevant docs for IE/EDGE, but no luck till now. > > Thanks! > > > > > https://codereview.chromium.org/2665623002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by chengx@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...
Patchset #9 (id:280001) has been deleted
The CQ bit was checked by chengx@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...
Managed to make IE show jumplist on my machine. Edge doesn't support the jumplist anymore, so I am going to ignore it. You are right. IE must have written all bitmaps to the disk, rather than trying to find the one fitting to the current device scale factor which is what I do here. This is confirmed as when I increase the device scale factor, the IE jumplist icon automatically switches to high resolution. The current patch in this CL doesn't support this. Instead, a tab needs to be closed to trigger the jumplist code which fetches high resolution icons. To fix this, I have uploaded another patch, which writes all the bitmaps with scales that the platform supports. This enables the jumplist icon to increase resolution automatically when increasing the device scale factor. Thanks for pushing me this far. It is a great experience. PTAL~ On 2017/02/07 22:42:41, sky wrote: > Is there any documentation of the api we're using? I seem to remember > the ability to change whether jumplists are used. Is there an option > you can use to turn it on? > > -Scott
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:93: const gfx::ImageSkia* imageskia = image.ToImageSkia(); image_skia. https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:95: std::vector<float> supportedScales = imageskia->GetSupportedScales(); supported_scales (or use directly in for statement). https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:97: gfx::ImageSkiaRep imageSkiaRep = imageskia->GetRepresentation(scale); image_skia_rep. https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:560: data->icon_urls_.front().second->set_icon_data(image_result.image); I'm worried about using the image from multiple threads. See ImageSkia::DeepCopy for details of my concerns. I recommend using ImageSkia in ShellLinkItem. https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... File chrome/browser/win/jumplist_updater.h (right): https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... chrome/browser/win/jumplist_updater.h:46: void set_icon_data(const gfx::Image& data) { icon_data_ = data; } Keep the names consistent, so that rather than set_icon_data this should be set_image. Same thing with the member names and the functions.
The CQ bit was checked by chengx@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.
PTAL~ https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:93: const gfx::ImageSkia* imageskia = image.ToImageSkia(); On 2017/02/08 03:14:54, sky wrote: > image_skia. Done. https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:95: std::vector<float> supportedScales = imageskia->GetSupportedScales(); On 2017/02/08 03:14:54, sky wrote: > supported_scales (or use directly in for statement). Done. https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:97: gfx::ImageSkiaRep imageSkiaRep = imageskia->GetRepresentation(scale); On 2017/02/08 03:14:54, sky wrote: > image_skia_rep. Done. https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:560: data->icon_urls_.front().second->set_icon_data(image_result.image); On 2017/02/08 03:14:54, sky wrote: > I'm worried about using the image from multiple threads. See ImageSkia::DeepCopy > for details of my concerns. I recommend using ImageSkia in ShellLinkItem. Agreed. I switched to ImageSkia. https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... File chrome/browser/win/jumplist_updater.h (right): https://codereview.chromium.org/2665623002/diff/300001/chrome/browser/win/jum... chrome/browser/win/jumplist_updater.h:46: void set_icon_data(const gfx::Image& data) { icon_data_ = data; } On 2017/02/08 03:14:54, sky wrote: > Keep the names consistent, so that rather than set_icon_data this should be > set_image. Same thing with the member names and the functions. Done.
Description was changed from ========== Fix Jumplist favicons to have high resolution in HDPI Windows displays On a HDPI windows machine, if right click Chrome in the taskbar, the jumplist favicons are not displayed in high resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used for jumplist icon by the current jumplist implementation, regardless of the device scale factor(s). This CL fixes this issue. The strategy is that before the jumplist icon file is created, the largest device scale factor of all the monitors is looked up. The appropriate bitmap w.r.t the scale is used then. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix Jumplist favicons to have high resolution in HDPI Windows displays On a HDPI windows machine, if right click Chrome in the taskbar, the jumplist favicons are not displayed in high resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used for jumplist icon by the current jumplist implementation. This CL fixes this issue. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Thanks for the patience! https://codereview.chromium.org/2665623002/diff/320001/chrome/browser/win/jum... File chrome/browser/win/jumplist_updater.h (right): https://codereview.chromium.org/2665623002/diff/320001/chrome/browser/win/jum... chrome/browser/win/jumplist_updater.h:46: void set_image(std::unique_ptr<gfx::ImageSkia> image) { Can you make the getter/getter and member consistent? If you want icon_image_, then have this be set_icon_image. image(), set_image() and image_ is equally fine. Also, why the unique_ptr? ImageSkia supports copy semantics.
The CQ bit was checked by chengx@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~ Thanks! https://codereview.chromium.org/2665623002/diff/320001/chrome/browser/win/jum... File chrome/browser/win/jumplist_updater.h (right): https://codereview.chromium.org/2665623002/diff/320001/chrome/browser/win/jum... chrome/browser/win/jumplist_updater.h:46: void set_image(std::unique_ptr<gfx::ImageSkia> image) { On 2017/02/08 16:56:09, sky wrote: > Can you make the getter/getter and member consistent? If you want icon_image_, > then have this be set_icon_image. image(), set_image() and image_ is equally > fine. > > Also, why the unique_ptr? ImageSkia supports copy semantics. Function name updated. Yea, unique_ptr is not needed here. Sorry about that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix Jumplist favicons to have high resolution in HDPI Windows displays On a HDPI windows machine, if right click Chrome in the taskbar, the jumplist favicons are not displayed in high resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used for jumplist icon by the current jumplist implementation. This CL fixes this issue. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix Jumplist favicons to have high-resolution in HDPI Windows displays On a HDPI windows machine, if right-click Chrome in the taskbar, the jumplist favicons are not displayed in high-resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used to create jumplist icon file by the current jumplist implementation. This CL fixes this issue. The idea is that for all the display scales supported by the platform, get/compute all corresponding bitmaps and write them to the disk. In this way, the appropriate icon bitmap is used when a user changes the screen display scale. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Fix Jumplist favicons to have high-resolution in HDPI Windows displays On a HDPI windows machine, if right-click Chrome in the taskbar, the jumplist favicons are not displayed in high-resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used to create jumplist icon file by the current jumplist implementation. This CL fixes this issue. The idea is that for all the display scales supported by the platform, get/compute all corresponding bitmaps and write them to the disk. In this way, the appropriate icon bitmap is used when a user changes the screen display scale. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix Jumplist favicons to have high-resolution in HDPI Windows displays On a HDPI windows machine, if right-click Chrome in the taskbar, the jumplist favicons are not displayed in high-resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used to create jumplist icon file by the current jumplist implementation. This CL fixes this issue. The idea is that for all the display scales supported by the platform, get/compute all corresponding bitmaps and write them to the disk. In this way, the appropriate icon bitmap is used when a user changes the screen display scale. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
LGTM
The CQ bit was checked by chengx@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": 340001, "attempt_start_ts": 1486595804827850,
"parent_rev": "5dc5c67c91f37b85db7333304b5bec57c42a3810", "commit_rev":
"ae6956325bca345ff31425adc8597d33e45ce3f7"}
Message was sent while issue was closed.
Description was changed from ========== Fix Jumplist favicons to have high-resolution in HDPI Windows displays On a HDPI windows machine, if right-click Chrome in the taskbar, the jumplist favicons are not displayed in high-resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used to create jumplist icon file by the current jumplist implementation. This CL fixes this issue. The idea is that for all the display scales supported by the platform, get/compute all corresponding bitmaps and write them to the disk. In this way, the appropriate icon bitmap is used when a user changes the screen display scale. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Fix Jumplist favicons to have high-resolution in HDPI Windows displays On a HDPI windows machine, if right-click Chrome in the taskbar, the jumplist favicons are not displayed in high-resolution. They look blurry, different from other favicons in the rest of Chrome, e.g., tabs. As reported, Internet Explorer doesn't have this issue. The root cause is that 1x bitmap (16x16) is always used to create jumplist icon file by the current jumplist implementation. This CL fixes this issue. The idea is that for all the display scales supported by the platform, get/compute all corresponding bitmaps and write them to the disk. In this way, the appropriate icon bitmap is used when a user changes the screen display scale. BUG=676901 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2665623002 Cr-Commit-Position: refs/heads/master@{#449134} Committed: https://chromium.googlesource.com/chromium/src/+/ae6956325bca345ff31425adc859... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:340001) as https://chromium.googlesource.com/chromium/src/+/ae6956325bca345ff31425adc859... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
