New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 596757 link

Starred by 10 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Context menu has 1x extension icons on Retina screen

Project Member Reported by lgar...@chromium.org, Mar 22 2016

Issue description

Chrome 51.0.2679.0 
OSX 10.11.3

What steps will reproduce the problem?
(1) Install the 1Password extension (https://agilebits.com/onepassword/extensions) using a computer with/connected to a Retina screen.
(2) Visit twitter.com
(3) Click "Log in", and right-click on the password field.

What is the expected output?
The context menu icon for 1Password renders at 2x.

What do you see instead?
The context menu icon for 1Password renders at 1x.
 
Screen Shot 2016-03-21 at 18.06.01.png
81.5 KB View Download
Cc: rdevlin....@chromium.org
Status: Available (was: Untriaged)
Hmm, in MenuManager::GetIconForExtension we end up using ExtensionIconManager::GetIcon which returns a kFavIconSize icon. Maybe ExtensionIconManager hasn't been updated to know about high DPI display?

Devlin, did you have to deal with this for the browser action redesign? Do you know off the top of your head where we're getting the icons for browser actions? Presumably we don't also have this problem for them. 



Hi. I experience the same issue on Windows 10 build 14393.447 with Chrome 56.0.2924.10 and Save to Google extension (https://chrome.google.com/webstore/detail/save-to-google/meoeeoaohbmgbocpdpnjklmfmjjagkkf) except for scaling is even worse.

Please see attached screenshot as an example.
Context menu.png
11.8 KB View Download

Comment 3 by est...@chromium.org, Dec 14 2016

Labels: -Pri-3 OS-Chrome OS-Linux OS-Windows Pri-2
Owner: est...@chromium.org
Status: Started (was: Available)

Comment 4 by est...@chromium.org, Dec 16 2016

Devlin requested before/after screenshots. Comment 2 has the before, here's an after
7MhCHNvSC0a.png
270 KB View Download
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/32426e0c6511485debbf3b7c77e87791b6b3ec93

commit 32426e0c6511485debbf3b7c77e87791b6b3ec93
Author: estade <estade@chromium.org>
Date: Sun Dec 18 01:26:17 2016

Make some updates to extension iconography.

Several related changes in one go:

1. Remove padding from extension search icon in location bar
   (linux, cros, mac). This was causing misalignment between the popup
   icon and the location bar icon, and unintended discrepancies between
   platforms.
2. Make ExtensionIconManager handle all supported scale factors rather
   than just 1x.
3. Remove some obsolete code in the apps page. Apps will always be given
   a default icon, and we never use the small icon codepath any more.
   This also means FaviconWebUIHandler is no longer needed.

BUG= 674259 , 596757 
TBR=kinaba@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2576833002
Cr-Commit-Position: refs/heads/master@{#439361}

[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/chromeos/file_manager/file_tasks.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/api/management/chrome_management_api_delegate.h
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/api/omnibox/omnibox_api.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/api/omnibox/omnibox_api.h
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/context_menu_matcher.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/extension_icon_manager.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/extension_icon_manager.h
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/extension_icon_manager_unittest.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/extension_util.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/menu_manager.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/extensions/menu_manager.h
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/resources/ntp4/apps_page.css
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/resources/ntp4/apps_page.js
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/resources/ntp4/new_tab.html
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/resources/ntp4/new_tab.js
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/resources/ntp4/page_list_view.js
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/ui/omnibox/chrome_omnibox_client.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/ui/webui/app_launcher_page_ui.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/ui/webui/extensions/extension_icon_source.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/ui/webui/extensions/extension_icon_source.h
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/ui/webui/ntp/app_launcher_handler.cc
[delete] https://crrev.com/111fdd030ce49ce1de525ac10a983bed8fd8720e/chrome/browser/ui/webui/ntp/favicon_webui_handler.cc
[delete] https://crrev.com/111fdd030ce49ce1de525ac10a983bed8fd8720e/chrome/browser/ui/webui/ntp/favicon_webui_handler.h
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/browser/ui/webui/ntp/new_tab_ui.cc
[add] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/test/data/extensions/context_menus/icons/16.png
[add] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/test/data/extensions/context_menus/icons/24.png
[add] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/test/data/extensions/context_menus/icons/32.png
[add] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/test/data/extensions/context_menus/icons/manifest.json
[add] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/chrome/test/data/extensions/context_menus/icons/sample.js
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/extensions/browser/api/management/management_api.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/extensions/browser/api/management/management_api_delegate.h
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/extensions/browser/image_loader.cc
[modify] https://crrev.com/32426e0c6511485debbf3b7c77e87791b6b3ec93/extensions/browser/image_loader.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/47ce132c79896db4fe4832a96b097adc643e6e99

commit 47ce132c79896db4fe4832a96b097adc643e6e99
Author: estade <estade@chromium.org>
Date: Tue Jan 17 20:37:47 2017

Load extension icons for more scale factors.

The "supported" scale factors are just the ones that we load raster
assets for (in ResourceBundle). This patch additionally looks for
icons in scale factors that correlate with the currently attached
displays.

Practically speaking, this means a machine that's running at 1.5x such
as a Surface Pro will have better looking extension icons in the
renderer context menu.

BUG= 596757 

Review-Url: https://codereview.chromium.org/2609853003
Cr-Commit-Position: refs/heads/master@{#444128}

[modify] https://crrev.com/47ce132c79896db4fe4832a96b097adc643e6e99/chrome/browser/extensions/extension_icon_manager_unittest.cc
[modify] https://crrev.com/47ce132c79896db4fe4832a96b097adc643e6e99/extensions/browser/extension_icon_image.cc
[modify] https://crrev.com/47ce132c79896db4fe4832a96b097adc643e6e99/extensions/browser/extension_icon_image.h
[modify] https://crrev.com/47ce132c79896db4fe4832a96b097adc643e6e99/extensions/browser/image_loader.cc
[modify] https://crrev.com/47ce132c79896db4fe4832a96b097adc643e6e99/extensions/browser/image_loader.h
[modify] https://crrev.com/47ce132c79896db4fe4832a96b097adc643e6e99/extensions/browser/image_loader_unittest.cc
[modify] https://crrev.com/47ce132c79896db4fe4832a96b097adc643e6e99/ui/display/test/test_screen.cc

Cc: sureshkumari@chromium.org
Labels: Needs-Feedback
Tested the issue on Windows-7, Mac-10.12.2 and linux Ubuntu-14.04 using chrome version 57.0.2986.0 with below steps
1.opened Chrome and added 1Password extention .
2.Navigated to Twitter.com
3.Clicked "Log in", and right-clicked on the password field.
Observed that the context menu is same in version 57.0.2986.0 and in reported version 51.0.2679.0 .
Please find the attached screencasts for the same and please let us know if anything missed from our side to verify the issue.

Thanks..

57.0.2986.0.mp4
374 KB View Download
51.0.2679.0.mp4
377 KB View Download
Hi. I can confirm that the icon is now rendered properly in context menu. Google Chrome Canary 57.0.2986.0 on Windows 10 14393.693.

Thanks for the quick fix!
context menu.png
15.1 KB View Download

Comment 9 by est...@chromium.org, Jan 19 2017

Status: Fixed (was: Started)
thanks for verifying (the second patch above was necessary for your windows 10 Chrome which is probably using a dsf of 150%)
@estade, glad to help. Sorry, forgot to specify the scaling. It's indeed a HiDPI screen and the scaling is 125%.
Cc: jackhou@chromium.org tkonch...@chromium.org
 Issue 418380  has been merged into this issue.
Cc: krajshree@chromium.org est...@chromium.org
 Issue 692587  has been merged into this issue.

Sign in to add a comment