Invalidate installable state on manifest URL change |
||||
Issue descriptionCurrently, changing the manifest URL after the installability checks have run does nothing, as the underlying InstallableManager caches the manifest it fetches. We should properly handle changes to the manifest URL by listening to the WebContentsObserver::DidUpdateWebManifestURL method. InstallableManager should invalidate its internal state on a manifest URL change. AppBannerManager will be more complex, as we could possibly have dispatched BeforeInstallPromptEvent at the time of a manifest URL change. We'll need to invalidate internal state and destroy all outstanding callbacks. Ideally we'd spec that changing manifest URL after the banner pipeline has run (and events dispatched) would result in no installation prompts being shown until a full recheck is run (since we can't guarantee the site is installable any more and would have to recheck).
,
Dec 8 2017
Requesting merge of c#1 to M64. It's been on canary for a couple of days with no issues. This is necessary for the desktop PWAs dogfood in ChromeOS M64.
,
Dec 9 2017
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b646416269a56aea4c13e62aa23aa96d47f17464 commit b646416269a56aea4c13e62aa23aa96d47f17464 Author: Dominick Ng <dominickn@chromium.org> Date: Sun Dec 10 19:40:00 2017 Flush InstallableManager state when the manifest URL changes. InstallableManager caches the data it fetches as part of the installability check. This means that sites which change their manifest, or inject a manifest after the check is run are incorrectly classified. This CL makes InstallableManager listen to the WebContentsObserver::DidUpdateWebManifestURL() method, and invalidates its internal state when the method fires. This addresses the issue by making InstallableManager refetch data when we know that the manifest URL is different. Tests are added to ensure the correct behaviour. BUG=792299 Change-Id: If73128b7b86bd741c16235c8a8e7ec5f0caeb165 Reviewed-on: https://chromium-review.googlesource.com/809994 Commit-Queue: Dominick Ng <dominickn@chromium.org> Reviewed-by: Ben Wells <benwells@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#522038}(cherry picked from commit ae3d53515f19b9870bcfd26770818814fba9ee9a) Reviewed-on: https://chromium-review.googlesource.com/818362 Reviewed-by: Dominick Ng <dominickn@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#115} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/b646416269a56aea4c13e62aa23aa96d47f17464/chrome/browser/installable/installable_manager.cc [modify] https://crrev.com/b646416269a56aea4c13e62aa23aa96d47f17464/chrome/browser/installable/installable_manager.h [modify] https://crrev.com/b646416269a56aea4c13e62aa23aa96d47f17464/chrome/browser/installable/installable_manager_browsertest.cc [modify] https://crrev.com/b646416269a56aea4c13e62aa23aa96d47f17464/chrome/test/data/banners/main.js
,
Dec 10 2017
Solving the banner case here is more gnarly since we need to work out what the behaviour should be if a site changes manifest mid-pipeline (e.g. after beforeinstallprompt event is fired). Spec changes may be required. |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Dec 6 2017