App manifest: Unrecognised icon "purpose" is treated as "any" rather than ignored |
||||||||
Issue descriptionChrome 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.
,
Dec 14
WIP CL chain: 1. https://chromium-review.googlesource.com/c/chromium/src/+/1375195/ 2. https://chromium-review.googlesource.com/c/chromium/src/+/1377471/
,
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
,
Jan 4
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.
,
Jan 4
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
,
Jan 4
Thanks mgiuca@ - how safe is this merge overall? Are you sure it won't introduce any new regressions?
,
Jan 7
#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).
,
Jan 7
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
,
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
,
Jan 7
Is #9 also needed for M72? Seems like it just landed in trunk.
,
Jan 8
#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.
,
Jan 8
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.
,
Jan 8
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}
,
Jan 8
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 |
||||||||
Comment 1 by mgiuca@chromium.org
, Dec 12Status: Assigned (was: ExternalDependency)