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

Issue 905561 link

Starred by 3 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

App manifest: Unrecognised icon "purpose" is treated as "any" rather than ignored

Project Member Reported by mgiuca@chromium.org, Nov 15

Issue description

Chrome Version: (copy from chrome://version)
OS: (e.g. Win10, MacOS 10.12, etc...)

What steps will reproduce the problem?
(1) https://killer-marmot.appspot.com/web/
(2) Open dev tools -> Application -> Manifest.

What is the expected result?
There is a 432x432 icon in the app manifest with purpose "maskable". Chrome doesn't yet recognise "maskable" (though this is changing:  Issue 904354 , so this expected behaviour will change).

Since the purpose is not recognised, I would expect an error to the effect of:

    Found icon with invalid purpose. It is ignored.

And the icon should not show up in the icons list.

Once "maskable" is implemented, the maskable icon should still be ignored for all purposes other than generating an Android adaptive icon.

What happens instead?
Error:

    found icon with invalid purpose. Using default value 'any'.".

The 432x432 icon is seen in the icons list, even though its purpose is unrecognised.

This is a manifest spec bug; see https://github.com/w3c/manifest/issues/728. We should wait until spec changes land before fixing it in Chrome.
 
Labels: M-72
Status: Assigned (was: ExternalDependency)
Manifest change has landed.

We need to update this in Chrome. I'm starting to see the Maskable icon for Killer Marmot on (at least) Windows and macOS installs for this app, which is incorrect. (Although it conforms to the old spec, it isn't the author's intention, and it is no longer conforming to the spec as of today.)

Chrome's manifest parser applies the (old) Manifest logic: if the purpose is one of the 3 recognised ones, it sets the enum ANY, BADGE or MASKABLE. If it isn't recognised, it defaults to ANY. So when we added MASKABLE in M72, this actually changed the way "purpose": "manifest" icons are processed, but didn't actually stop desktop platforms from using maskable icons.

There are actually two separate issues here in Chrome (thanks @dominickn for helping diagnose this):

1. chrome/browser/web_applications/components/web_app_install_utils.cc GetValidIconUrlsToDownload doesn't care what the (enum) purpose of an icon is; it downloads them all, even if the purpose is BADGE or MASKABLE.
2. content/renderer/manifest/manifest_parser.cc ParseIconPurpose follows the old manifest logic of defaulting the purpose to ANY if the purpose is not recognised. Instead, it should discard the icon completely.

Both of these need to be fixed. Fixing 1 solves it for "maskable" and "badge", but not unrecognised purposes. Fixing 2 solves it for unrecognised purposes, but not "maskable" and "badge".

We should also probably refactor the CRX installer to use the InstallableManager's logic for selecting an appropriate icon (which does take purpose into account).

Marking as M-72 since this might actually need to be merged (we can consider it a regression since we're launching "purpose": "maskable" in M72, sites will start using "maskable" which will show up badly on non-Android platforms).
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 17

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

commit b50a11759992020f899929087973fe528d70e880
Author: Matt Giuca <mgiuca@chromium.org>
Date: Mon Dec 17 05:09:15 2018

Web app installer: Do not use special-purpose icons as the app icon.

A Manifest icon with a recognised, non-"any" purpose (i.e., "badge" or
"maskable") is no longer considered as a normal icon when installing the
app.

This updates Chrome to match the latest version of the Manifest standard
(which was updated by https://github.com/w3c/manifest/pull/741), which
asks that "User agents SHOULD NOT use an icon other than for its stated
purpose."

This fixes "maskable" icons (which are full-bleed square-shaped icons
designed to be masked on Android) from showing up as the normal app icon
on desktop platforms.

Bug: 905561
Change-Id: I48e044f45641d2a8f1ddbc185d4214734708b765
Reviewed-on: https://chromium-review.googlesource.com/c/1375195
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617049}
[modify] https://crrev.com/b50a11759992020f899929087973fe528d70e880/chrome/browser/extensions/bookmark_app_helper_unittest.cc
[modify] https://crrev.com/b50a11759992020f899929087973fe528d70e880/chrome/browser/web_applications/components/web_app_install_utils.cc
[modify] https://crrev.com/b50a11759992020f899929087973fe528d70e880/chrome/browser/web_applications/components/web_app_install_utils_unittest.cc

Labels: Merge-Request-72
Requesting a merge of b50a1175999 to M-72. Rationale: This wasn't specifically a regression in M-72 (it's always been an issue), but M-72 introduced the concept of a Maskable icon, and we're now encouraging developers to use it. In doing so, we're effectively regressing this because any developer who adds a maskable icon will start to see it showing up in places where it shouldn't, without this fix.
Project Member

Comment 5 by sheriffbot@chromium.org, Jan 4

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Thanks mgiuca@ - how safe is this merge overall? Are you sure it won't introduce any new regressions?
#6: I just confirmed fixed on Canary 73.0.3663.0.

It's very safe. Just rewrites a loop in web_app_install_utils.cc. The worst case if there's a bug in it, is we generate a bad app icon file (but that doesn't appear to be the case from manual testing).
Cc: -dominickn@chromium.org mgiuca@chromium.org
Owner: dominickn@chromium.org
Assigning to Dominick (since I am going on leave). To deal with:

a) the merge
b) landing follow-up CL https://crrev.com/c/1377471
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 7

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

commit 3976ff075e1950bbf32393c7aa79489a57f42be2
Author: Matt Giuca <mgiuca@chromium.org>
Date: Mon Jan 07 14:57:55 2019

Manifest: Discard icons with unrecognised purpose.

An icon with an unrecognised purpose (and no valid purpose) is now
ignored completely by the manifest parser, as opposed to given the
default purpose "any". If there is also a valid purpose, the invalid
purposes are ignored but the icon is kept.

This updates Chrome to match the latest version of the Manifest standard
(which was updated by https://github.com/w3c/manifest/pull/741). The
previous design was not forwards-compatible, since whenever we add a new
purpose, older user agents that do not yet recognise it will treat those
special-purpose icons as standard icons.

Bug: 905561
Change-Id: Ie66912e5aff3685ae5f13aeadae9bf3132b0dbf5
Reviewed-on: https://chromium-review.googlesource.com/c/1377471
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620319}
[modify] https://crrev.com/3976ff075e1950bbf32393c7aa79489a57f42be2/content/renderer/manifest/manifest_parser.cc
[modify] https://crrev.com/3976ff075e1950bbf32393c7aa79489a57f42be2/content/renderer/manifest/manifest_parser.h
[modify] https://crrev.com/3976ff075e1950bbf32393c7aa79489a57f42be2/content/renderer/manifest/manifest_parser_unittest.cc
[modify] https://crrev.com/3976ff075e1950bbf32393c7aa79489a57f42be2/third_party/blink/public/common/manifest/manifest.h
[modify] https://crrev.com/3976ff075e1950bbf32393c7aa79489a57f42be2/third_party/blink/public/mojom/manifest/manifest.mojom

Is #9 also needed for M72? Seems like it just landed in trunk.
#10: we don't need #9 for M72. Just #3 is necessary, thanks! I'll merge if you can give approval based on our replies above.
Cc: abdulsyed@chromium.org
Labels: -Merge-Review-72 Merge-Approved-72
Approving merge to M72 branch 3626 for CL listed at #3 based on comment #11. Pls merge ASAP so we can pick it up for this week beta release. Thank you.
Labels: -Merge-Approved-72 Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/5cdedba04813f439f92f78c7398b5437fdb5a884

Commit: 5cdedba04813f439f92f78c7398b5437fdb5a884
Author: mgiuca@chromium.org
Commiter: dominickn@chromium.org
Date: 2019-01-08 06:13:56 +0000 UTC

Web app installer: Do not use special-purpose icons as the app icon.

A Manifest icon with a recognised, non-"any" purpose (i.e., "badge" or
"maskable") is no longer considered as a normal icon when installing the
app.

This updates Chrome to match the latest version of the Manifest standard
(which was updated by https://github.com/w3c/manifest/pull/741), which
asks that "User agents SHOULD NOT use an icon other than for its stated
purpose."

This fixes "maskable" icons (which are full-bleed square-shaped icons
designed to be masked on Android) from showing up as the normal app icon
on desktop platforms.

Bug: 905561
Change-Id: I48e044f45641d2a8f1ddbc185d4214734708b765
Reviewed-on: https://chromium-review.googlesource.com/c/1375195
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#617049}(cherry picked from commit b50a11759992020f899929087973fe528d70e880)
Reviewed-on: https://chromium-review.googlesource.com/c/1399778
Cr-Commit-Position: refs/branch-heads/3626@{#603}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Project Member

Comment 14 by bugdroid1@chromium.org, Jan 8

Labels: merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cdedba04813f439f92f78c7398b5437fdb5a884

commit 5cdedba04813f439f92f78c7398b5437fdb5a884
Author: Matt Giuca <mgiuca@chromium.org>
Date: Tue Jan 08 06:13:56 2019

Web app installer: Do not use special-purpose icons as the app icon.

A Manifest icon with a recognised, non-"any" purpose (i.e., "badge" or
"maskable") is no longer considered as a normal icon when installing the
app.

This updates Chrome to match the latest version of the Manifest standard
(which was updated by https://github.com/w3c/manifest/pull/741), which
asks that "User agents SHOULD NOT use an icon other than for its stated
purpose."

This fixes "maskable" icons (which are full-bleed square-shaped icons
designed to be masked on Android) from showing up as the normal app icon
on desktop platforms.

Bug: 905561
Change-Id: I48e044f45641d2a8f1ddbc185d4214734708b765
Reviewed-on: https://chromium-review.googlesource.com/c/1375195
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#617049}(cherry picked from commit b50a11759992020f899929087973fe528d70e880)
Reviewed-on: https://chromium-review.googlesource.com/c/1399778
Cr-Commit-Position: refs/branch-heads/3626@{#603}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/5cdedba04813f439f92f78c7398b5437fdb5a884/chrome/browser/extensions/bookmark_app_helper_unittest.cc
[modify] https://crrev.com/5cdedba04813f439f92f78c7398b5437fdb5a884/chrome/browser/web_applications/components/web_app_install_utils.cc
[modify] https://crrev.com/5cdedba04813f439f92f78c7398b5437fdb5a884/chrome/browser/web_applications/components/web_app_install_utils_unittest.cc

Sign in to add a comment