New issue
Advanced search Search tips

Issue 920885 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 17
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

App icon size unification on web apps and bookmarks

Project Member Reported by sgabr...@chromium.org, Jan 11

Issue description

Version 73.0.3644.0 Canary

-- 

See the image attached. @Vlad, I cannot recall if the size unification is now active on web apps and bookmarks icons in shelf/launcher etc... If you look at the showtime bookmarks, you can see that it doesn't appear to be unified.
 
preview.png
228 KB View Download
Status: Started (was: Assigned)
Owner: benwells@chromium.org
Status: Assigned (was: Started)
Ben, is there anything special about the source of the Showtime app icon? I was not able to track it down. 

The Showtime icon bitmap has an alphaType == kOpaque_SkAlphaType which is unusual for an extension icon. The resizing logic in md_icon_normalizer.cc currently works only for icons that support transparency. For this particular icon the code bails out here: https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/md_icon_normalizer.cc?rcl=0207eefb445f9855c2ed46280cb835b6f08bdb30&l=88, and no resizing is attempted.
Cc: dominickn@google.com
Web app installs are sourced from the web sites manifest, so we get the app directly from showtime. Possibly the bookmark app system is processing these differently but it would be the same as the spotify icon for example.

What is the actual problem? I don't understand what unified means in this context.

Adding Dom who thinks / cares about all apps being treated equally.
Cc: -dominickn@google.com dominickn@chromium.org
I'm guessing that Showtime has a square icon that's exported without an alpha channel, and we don't add alpha at any point in the installation process. I would not be surprised if there were plenty of sites which did this.

What exactly is the MD icon normalizer trying to do?

Comment 5 by kaznacheev@chromium.org, Jan 16 (6 days ago)

Owner: kaznacheev@chromium.org
The normalizer adjusts the icon scale to enforce these requirements:
https://material.io/design/iconography/product-icons.html#grid-keyline-shapes

This results in app grid icons (and shelf icons as well) having roughly similar "visual weight". 

This was implemented in http://crrev.com/c/1183811 (see http://crbug.com/848007).

As of Q4'18 most 1st party apps have their icons already MD compliant at the source. The normalizer have been doing a good job adjusting the scale for most 3rd party icons, but bailed out when encountering an icon with no alpha channel. This CL fixes this: http://crrev.com/c/1415511

Comment 6 by kaznacheev@chromium.org, Jan 16 (6 days ago)

Attaching the screenshot of the resized shelf icon for Showtime.
Screenshot 2019-01-16 at 10.14.32 AM.png
49.4 KB View Download

Comment 7 by kaznacheev@chromium.org, Jan 16 (6 days ago)

Status: Started (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 17 (5 days ago)

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

commit a9b7bf941b7d08afb0db34c59b37659f56d606fb
Author: Vladislav Kaznacheev <kaznacheev@google.com>
Date: Thu Jan 17 18:26:27 2019

Apply icon normalization to icons with no alpha channel

Previously Material Design icon normalization was not attempted for
icons with no alpha channel. However, it makes perfect sense to treat
such icons as fully opaque and scale them accordingly.

Bug:  920885 
Test: MdIconNormalizerTest*, ChromeAppIcon*
Change-Id: I4063eff7df5fddfc8a34cc6455cc9dc805b2b71a
Reviewed-on: https://chromium-review.googlesource.com/c/1415511
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Commit-Queue: Vladislav Kaznacheev <kaznacheev@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623757}
[modify] https://crrev.com/a9b7bf941b7d08afb0db34c59b37659f56d606fb/chrome/browser/extensions/chrome_app_icon_unittest.cc
[modify] https://crrev.com/a9b7bf941b7d08afb0db34c59b37659f56d606fb/chrome/browser/ui/app_list/md_icon_normalizer.cc
[modify] https://crrev.com/a9b7bf941b7d08afb0db34c59b37659f56d606fb/chrome/browser/ui/app_list/md_icon_normalizer_unittest.cc

Comment 9 by kaznacheev@chromium.org, Jan 17 (5 days ago)

Status: Fixed (was: Started)

Comment 10 by sgabr...@chromium.org, Jan 17 (5 days ago)

Thanks for the quick fix!

Sign in to add a comment