Resized the icon in the Uninstall dialog to be 64x64 px

Resized the icon in the Uninstall dialog to be 64x64 px

Resized the icon in the Uninstall dialog to be 64x64 px instead of
69x69px, so that it matches the icon in the App Info dialog where
it can be launched. This also removes fuzzing effects on icons
that have to be resized from 64x64 icons.
Also fixed the duplication of this constant by letting the view
resize the icon it receives, rather than having it request one size of
icon and then resize it a second time.

[Mac] XIB change:
  - Resized the icon in the Install dialogs from 69x69 to 64x64

Screenshots on bug.

BUG=379613
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274487

[tapted, 2014-05-29 05:14:27]

https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_uninstall_dialog.cc (left):

https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_uninstall_dialog.cc:54: static const int
kIconSize = 69;
drive-by: This constant is used in some mac .xib (interface builder) files as
well, which might get messed up if it changes here.

What's the rationale for this change? (BUG=?) Depending what that is... you
might need to update the xib files as well.

[sashab, 2014-05-29 05:19:17]

https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_uninstall_dialog.cc (left):

https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_uninstall_dialog.cc:54: static const int
kIconSize = 69;
On 2014/05/29 05:14:28, tapted wrote:
> drive-by: This constant is used in some mac .xib (interface builder) files as
> well, which might get messed up if it changes here.
> 
> What's the rationale for this change? (BUG=?) Depending what that is... you
> might need to update the xib files as well.

This was feedback from a UX reviewer. The icon size should match the App Info
dialog where this dialog can be launched, which is 64x64px.

Where are these .xib files?

[tapted, 2014-05-29 05:22:35]

On 2014/05/29 05:19:17, sasha_b wrote:
>
https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_uninstall_dialog.cc (left):
> 
>
https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_uninstall_dialog.cc:54: static const int
> kIconSize = 69;
> On 2014/05/29 05:14:28, tapted wrote:
> > drive-by: This constant is used in some mac .xib (interface builder) files
as
> > well, which might get messed up if it changes here.
> > 
> > What's the rationale for this change? (BUG=?) Depending what that is... you
> > might need to update the xib files as well.
> 
> This was feedback from a UX reviewer. The icon size should match the App Info
> dialog where this dialog can be launched, which is 64x64px.
> 
> Where are these .xib files?

These ones :)

$ git grep '{69, 69}' -- '*.xib'
chrome/app/nibs/ExtensionInstallPrompt.xib:                                     
       <string key="NSFrame">{{380, 64}, {69, 69}}</string>
chrome/app/nibs/ExtensionInstallPromptNoWarnings.xib:                           
               <string key="NSFrame">{{380, 66}, {69, 69}}</string>
chrome/app/nibs/ExtensionInstallPromptWebstoreData.xib:                         
               <string key="NSFrame">{{380, 129}, {69, 69}}</string>

[calamity, 2014-05-29 05:36:20]

FYI, the ExtensionInstallDialog also uses a 69x69 version. We should look for UI
approval to change that to be consistent.

https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_uninstall_dialog.cc (right):

https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_uninstall_dialog.cc:80: :
extension_misc::EXTENSION_ICON_LARGE;
I have a hunch that you can now change this to use 64 as well. (Even bookmark
apps should work with 64x64 [at least, it should for the latest set of bookmark
app icons]).

This means that we'll use a 64x64 icon where available whereas currently we
always use the 128x128 downscaled. However I think it's reasonable that all our
surfaces that display a 64x64 icon look the same.

https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_uninstall_dialog.cc:95: gfx::Size(0, 0),
gfx::Size()

[sashab, 2014-05-29 05:46:15]

Will confirm this change with UX as well.

https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_uninstall_dialog.cc (right):

https://codereview.chromium.org/301063003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_uninstall_dialog.cc:95: gfx::Size(0, 0),
On 2014/05/29 05:36:21, calamity wrote:
> gfx::Size()

Done.

[calamity, 2014-05-30 06:42:03]

Can you also make the equivalent change that you made for
ExtensionUninstallDialog in ExtensionInstallPrompt? (Regarding the max_size
stuff)

https://codereview.chromium.org/301063003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_uninstall_dialog.cc (left):

https://codereview.chromium.org/301063003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_uninstall_dialog.cc:91: :
extension_misc::EXTENSION_ICON_LARGE;
On second thought, I think we need to keep this for the HiDPI case. We should
always use 128x128 to display consistently across all dialogs on all displays.
The 128x128 is also guaranteed to exist.

This also means you can get rid of kIconSize.

[calamity, 2014-05-30 06:42:46]

Can you also make the equivalent change that you made for
ExtensionUninstallDialog in ExtensionInstallPrompt? (Regarding the max_size
stuff)

[calamity, 2014-05-30 06:42:59]

Can you also make the equivalent change that you made for
ExtensionUninstallDialog in ExtensionInstallPrompt? (Regarding the max_size
stuff)

[sashab, 2014-05-30 07:20:46]

Done. Also, tapted - can you please verify in mac? Not sure if these number
changes have dependencies elsewhere in those .xib files.

https://codereview.chromium.org/301063003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_uninstall_dialog.cc (left):

https://codereview.chromium.org/301063003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_uninstall_dialog.cc:91: :
extension_misc::EXTENSION_ICON_LARGE;
On 2014/05/30 06:42:03, calamity wrote:
> On second thought, I think we need to keep this for the HiDPI case. We should
> always use 128x128 to display consistently across all dialogs on all displays.
> The 128x128 is also guaranteed to exist.
> 
> This also means you can get rid of kIconSize.

Done.

[calamity, 2014-05-30 08:45:25]

lgtm!

https://codereview.chromium.org/301063003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/301063003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:701: //     lazily.
Leave this TODO.

https://codereview.chromium.org/301063003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_uninstall_dialog.cc (right):

https://codereview.chromium.org/301063003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_uninstall_dialog.cc:77: // Bookmark apps may
not have 128x128 icons so accept 48x48 icons.
Update comment to say 64x64.

[tapted, 2014-06-02 02:34:52]

On 2014/05/30 07:20:46, sasha_b wrote:
> Done. Also, tapted - can you please verify in mac? Not sure if these number
> changes have dependencies elsewhere in those .xib files.

It looks fine - but can you create a BUG= so I can attach screenshots?

You'll also need someone from chrome/app/nibs/OWNERS to approve changes - these
particular ones haven't changed for a while, and the original authors have moved
along. rsesek perhaps.

Also, XIB changes need an extra paragraph in the CL description, since it's hard
to glean what's changing from the actual file - `git lg
ExtensionInstallPrompt.xib` for some examples of how this looks.

[tapted, 2014-06-02 02:38:45]

On 2014/06/02 02:34:52, tapted wrote:
> `git lg ExtensionInstallPrompt.xib` for some examples of how this looks.

uhh `git log` that is - I have an alias in ~/.gitconfig :p

[alias]
  lg = log --abbrev-commit --date local

[sashab, 2014-06-02 03:25:34]

https://codereview.chromium.org/301063003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_uninstall_dialog.cc (right):

https://codereview.chromium.org/301063003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_uninstall_dialog.cc:77: // Bookmark apps may
not have 128x128 icons so accept 48x48 icons.
On 2014/05/30 08:45:25, calamity wrote:
> Update comment to say 64x64.

Done.

[sashab, 2014-06-02 03:31:13]

rsesek:
  Please review changes in chrome/app/nibs

msw:
  Please review changes in chrome/browser/ui

benwells:
  Please review changes in chrome/browser

[benwells, 2014-06-02 06:07:02]

https://codereview.chromium.org/301063003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/301063003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:696:
extensions::ImageLoader::ImageRepresentation::NEVER_RESIZE,
Sorry if you've already been asked these questions, but what happens with the
NEVER_RESIZE if there is not an appropriate image?

https://codereview.chromium.org/301063003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:698:
ui::SCALE_FACTOR_100P));
What does this scale factor mean exactly? In particular what happens on hi dpi
devices?

[Robert Sesek, 2014-06-02 15:02:45]

chrome/app/nibs LGTM

[msw, 2014-06-02 20:54:56]

c/b/ui lgtm

[sashab, 2014-06-03 01:17:32]

benwells: we can talk offline about DPI stuff if you'd like :) Sorry for the
rant

https://codereview.chromium.org/301063003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/301063003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:696:
extensions::ImageLoader::ImageRepresentation::NEVER_RESIZE,
On 2014/06/02 06:07:02, benwells wrote:
> Sorry if you've already been asked these questions, but what happens with the
> NEVER_RESIZE if there is not an appropriate image?

If there is no appropriate image, OnImageLoaded() is called with an empty image.
This then causes SetIcon() to be called with NULL, which  then loads the default
icon for the app/extension.

https://codereview.chromium.org/301063003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_install_prompt.cc:698:
ui::SCALE_FACTOR_100P));
On 2014/06/02 06:07:02, benwells wrote:
> What does this scale factor mean exactly? In particular what happens on hi dpi
> devices?

If you look at ImageLoader::LoadImageAsync() (the function that was being called
before), this is just an expansion of this function so that we can use
NEVER_RESIZE instead of RESIZE_WHEN_LARGER for the second argument. So although
I'm not answering your question, this behaviour matches the previous behaviour.

Now, to answer your question - I'm not totally 100% confident with my knowledge
of this DPI stuff, but from what I understand this enum is saying 'load the NxN
image and display at NxN pixels'. If I passed SCALE_FACTOR_200P, it would say
'load this NxN image and display it at 2Nx2N pixels', which would effectively
stretch it by 2X but because it's a 200% DPI display, it would appear as the
same *actual* size. You could imagine a chromebook, for example, where doubling
the resolution didn't actually change the size of anything, it just increased
the *resolution*, making edges smoother and corners rounder. I think that's
where this is trying to head - where you actually pass the scale factor of the
display in here, and it displays it accordingly.

Unfortunately, there's no easy way to get the 'DPI' of a display since you could
have two monitors, each with a different DPI, and you can even drag one window
between monitors and stuff like that. But we can get a good estimate of the DPI,
and you can see that in ResourceBundle::InitSharedInstance(). Basically,
especially on Windows, we are just picking a target DPI that looks nice so that
when we re-size images, they don't look too blurry.

Update: After talking to calamity@, this SCALE_FACTOR_100P is passed to
ImageSkia as 'choose the image with this DPI'. Since we are just using ImageSkia
as an image container for a single image, and not properly using it to load
images for each DPI and then choose the correct one with the given DPI at
runtime, we should only ever pass SCALE_FACTOR_100P since there will only be one
image in our ImageSkia, and it will have this DPI. For apps and extensions,
since we want to display the app's actual icon wherever possible (and not our
default icon), we always want to choose the app's icon, no matter what DPI
display the user has.

I think the proper fix here would be to go through all the icons an app has for
our target resolution, assign each one to a DPI, and then actually choose an
icon that best matches the target display. However, since we don't provide apps
with a way to specify DPI (and only a way to specify resolution), there is a lot
more work to be done before we can 'smart' choose app icons for high DPI
monitors.

Long story short: this is a hack.

[benwells, 2014-06-03 03:36:52]

lgtm after discussing offline. Doing resizing in the view, not the loader sounds
good. Would be good to investigate using the 64x64 image instead of the 128x128
at some point.

[sashab, 2014-06-03 03:38:16]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-06-03 03:39:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/301063003/60001

[commit-bot: I haz the power, 2014-06-03 07:33:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-03 12:40:48]

Change committed as 274487