Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2942513002:
Allow banners to trigger on sites which don't register a service worker onload. (Closed)
|
Created:
3 years, 6 months ago by dominickn Modified:
3 years, 6 months ago CC:
chromium-reviews, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow banners to trigger on sites which don't register a service worker onload.
This CL makes the InstallableManager a ServiceWorkerContextObserver,
which allows it to fall back to waiting for a service worker
registration event in the case where a site has not loaded its service
worker when the banner check is triggered.
All clients of the InstallableManager will inherit the waiting behaviour,
unless they explicitly opt out via the InstallableParams passed to
InstallableManager::GetData. In particular, adding WebAPKs from the
add to homescreen menu item will also use the wait-for-worker
behaviour.
A key difference is that InstallableManager::GetData may now wait
forever (or until the page is navigated) without calling the specified
callback in the case where a site never registers a service worker.
New browser tests for the InstallableManager are added to ensure
correctness.
BUG=721881
Review-Url: https://codereview.chromium.org/2942513002
Cr-Commit-Position: refs/heads/master@{#480751}
Committed: https://chromium.googlesource.com/chromium/src/+/14132ac523af4ee5ee38bd5ae0166d151015a786
Patch Set 1 #Patch Set 2 : Rebase dependency #Patch Set 3 : Fix DCHECK failure #Patch Set 4 : Fix unit tests #Patch Set 5 : Fix browser_tests on ChromeOS #Patch Set 6 : Fix windows compile #
Total comments: 14
Patch Set 7 : Rebase #Patch Set 8 : Fix crashing Android test. Comments #
Total comments: 2
Patch Set 9 : Less flaky test #
Total comments: 4
Patch Set 10 : Don't cache NO_MATCHING_SERVICE_WORKER #Dependent Patchsets: Messages
Total messages: 71 (48 generated)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Description was changed from ========== Allow banners to trigger on sites which don't load a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. Additional tests are added across the InstallableManager and app banner code paths to ensure correctness. BUG=625051 ========== to ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. Additional tests are added across the InstallableManager and app banner code paths to ensure correctness. BUG=625051 ==========
dominickn@chromium.org changed reviewers: + benwells@chromium.org, isherman@chromium.org
isherman: PTAL at enums.xml benwells: PTAL at everything else Thanks!
Description was changed from ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. Additional tests are added across the InstallableManager and app banner code paths to ensure correctness. BUG=625051 ========== to ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. All clients of the InstallableManager will inherit the waiting behaviour, unless they explicitly opt out via the InstallableParams passed to InstallableManager::GetData. In particular, adding WebAPKs from the add to homescreen menu item will also use the wait-for-worker behaviour. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. Additional tests are added across the InstallableManager and app banner code paths to ensure correctness. BUG=625051 ==========
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/banners... File chrome/browser/banners/app_banner_manager.cc (right): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/banners... chrome/browser/banners/app_banner_manager.cc:197: return is_debug_mode_ || Nit: This is a bit confusing - IsDebugMode and is_debug_mode_ refer to different concepts. I think you should change is_debug_mode_ to something like from_devtools_ or something to clarify, but I'd be cool with a TODO for now. https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/banners... chrome/browser/banners/app_banner_manager.cc:244: // wait for the worker in that instance). Maybe it's just late but this confuses me. Edit: After seeing the rest of the patch I understand now. I think just renaming is_debug_mode_ to from_devools_ will make it pretty obvious why you just want to check that and not the full IsDebugMode. https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager_browsertest.cc (left): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager_browsertest.cc:626: CheckPageWithManifestAndNoServiceWorker) { Is the no service worker case tested at all?
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; Drive by: This should be false for add_to_homescreen_data_fetcher.cc ?
On 2017/06/14 05:54:24, dominickn wrote: > isherman: PTAL at enums.xml > > benwells: PTAL at everything else > > Thanks! You don't need my review to add entries to enums.xml
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dominickn@chromium.org changed reviewers: - isherman@chromium.org
Description was changed from ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. All clients of the InstallableManager will inherit the waiting behaviour, unless they explicitly opt out via the InstallableParams passed to InstallableManager::GetData. In particular, adding WebAPKs from the add to homescreen menu item will also use the wait-for-worker behaviour. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. Additional tests are added across the InstallableManager and app banner code paths to ensure correctness. BUG=625051 ========== to ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. All clients of the InstallableManager will inherit the waiting behaviour, unless they explicitly opt out via the InstallableParams passed to InstallableManager::GetData. In particular, adding WebAPKs from the add to homescreen menu item will also use the wait-for-worker behaviour. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. Additional tests are added across the InstallableManager and app banner code paths to ensure correctness. BUG=625051, 721881 ==========
https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/banners... File chrome/browser/banners/app_banner_manager.cc (right): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/banners... chrome/browser/banners/app_banner_manager.cc:197: return is_debug_mode_ || On 2017/06/14 11:39:06, benwells wrote: > Nit: This is a bit confusing - IsDebugMode and is_debug_mode_ refer to different > concepts. I think you should change is_debug_mode_ to something like > from_devtools_ or something to clarify, but I'd be cool with a TODO for now. Done. https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/banners... chrome/browser/banners/app_banner_manager.cc:244: // wait for the worker in that instance). On 2017/06/14 11:39:06, benwells wrote: > Maybe it's just late but this confuses me. > > Edit: After seeing the rest of the patch I understand now. I think just renaming > is_debug_mode_ to from_devools_ will make it pretty obvious why you just want to > check that and not the full IsDebugMode. Done. https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; On 2017/06/14 15:34:36, pkotwicz wrote: > Drive by: This should be false for add_to_homescreen_data_fetcher.cc ? Nope, it should not be false for add_to_homescreen_data_fetcher. 1. Setting this to false means that https://crbug.com/721881 won't be fixed, since we need to wait for the service worker to fix it. 2. add_to_homescreen_data_fetcher.cc already has a 4 second wait timeout so it handles the case where this waits indefinitely. https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager_browsertest.cc (left): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager_browsertest.cc:626: CheckPageWithManifestAndNoServiceWorker) { On 2017/06/14 11:39:06, benwells wrote: > Is the no service worker case tested at all? I can restore the no service worker test with the waiting disabled. Otherwise, the manager waits indefinitely so it's really difficult to properly test the no-worker but wait forever version. Now we have tests for: - lazy worker, don't wait - lazy worker, wait - no worker, don't wait
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Just one little question... https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... File chrome/browser/installable/installable_manager_browsertest.cc (right): https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... chrome/browser/installable/installable_manager_browsertest.cc:691: "/banners/lazy_service_worker_test_page.html?swdelay=500"); Is the 500 important here? Just asking as 300 changed to 500 in this version, am hoping we don't have the test results depend on this value.
https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... File chrome/browser/installable/installable_manager_browsertest.cc (right): https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... chrome/browser/installable/installable_manager_browsertest.cc:691: "/banners/lazy_service_worker_test_page.html?swdelay=500"); On 2017/06/15 04:42:43, benwells wrote: > Is the 500 important here? Just asking as 300 changed to 500 in this version, am > hoping we don't have the test results depend on this value. In my empirical testing, 300ms was originally long enough for both Android and desktop. But it started flaking on desktop when I re-added this test (it would pass the check when it should have failed), hence the extension to 500. There's not really any other sane way to have a test for this. I could do a lot more mocking to have the manager quit when it gets to the waiting for SW step, then restart it, but that's getting to be a lot of test-only code that could be quite fragile. :(
On 2017/06/15 04:50:12, dominickn wrote: > https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... > File chrome/browser/installable/installable_manager_browsertest.cc (right): > > https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... > chrome/browser/installable/installable_manager_browsertest.cc:691: > "/banners/lazy_service_worker_test_page.html?swdelay=500"); > On 2017/06/15 04:42:43, benwells wrote: > > Is the 500 important here? Just asking as 300 changed to 500 in this version, > am > > hoping we don't have the test results depend on this value. > > In my empirical testing, 300ms was originally long enough for both Android and > desktop. But it started flaking on desktop when I re-added this test (it would > pass the check when it should have failed), hence the extension to 500. > > There's not really any other sane way to have a test for this. I could do a lot > more mocking to have the manager quit when it gets to the waiting for SW step, > then restart it, but that's getting to be a lot of test-only code that could be > quite fragile. :(
On 2017/06/15 05:41:50, benwells wrote: > On 2017/06/15 04:50:12, dominickn wrote: > > > https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... > > File chrome/browser/installable/installable_manager_browsertest.cc (right): > > > > > https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... > > chrome/browser/installable/installable_manager_browsertest.cc:691: > > "/banners/lazy_service_worker_test_page.html?swdelay=500"); > > On 2017/06/15 04:42:43, benwells wrote: > > > Is the 500 important here? Just asking as 300 changed to 500 in this > version, > > am > > > hoping we don't have the test results depend on this value. > > > > In my empirical testing, 300ms was originally long enough for both Android and > > desktop. But it started flaking on desktop when I re-added this test (it would > > pass the check when it should have failed), hence the extension to 500. > > > > There's not really any other sane way to have a test for this. I could do a > lot > > more mocking to have the manager quit when it gets to the waiting for SW step, > > then restart it, but that's getting to be a lot of test-only code that could > be > > quite fragile. :( My worry is that it will be fragile like this. If bots run fast (or slowly maybe?) for some reason, the tests will fail. Can you use one of the ExecuteScript varieties in https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.h to load the service worker, instead of having it on a delay?
On 2017/06/15 05:47:42, benwells wrote: > On 2017/06/15 05:41:50, benwells wrote: > > On 2017/06/15 04:50:12, dominickn wrote: > > > > > > https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... > > > File chrome/browser/installable/installable_manager_browsertest.cc (right): > > > > > > > > > https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... > > > chrome/browser/installable/installable_manager_browsertest.cc:691: > > > "/banners/lazy_service_worker_test_page.html?swdelay=500"); > > > On 2017/06/15 04:42:43, benwells wrote: > > > > Is the 500 important here? Just asking as 300 changed to 500 in this > > version, > > > am > > > > hoping we don't have the test results depend on this value. > > > > > > In my empirical testing, 300ms was originally long enough for both Android > and > > > desktop. But it started flaking on desktop when I re-added this test (it > would > > > pass the check when it should have failed), hence the extension to 500. > > > > > > There's not really any other sane way to have a test for this. I could do a > > lot > > > more mocking to have the manager quit when it gets to the waiting for SW > step, > > > then restart it, but that's getting to be a lot of test-only code that could > > be > > > quite fragile. :( > > My worry is that it will be fragile like this. If bots run fast (or slowly > maybe?) for some reason, the tests will fail. > > Can you use one of the ExecuteScript varieties in > https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.h to > load the service worker, instead of having it on a delay? This would require the mocking I mentioned earlier - reworking the way all the tests run quite significantly. We'd have to have the existing RunLoop break when the manager is in the waiting-for-service-worker state (requiring a new virtual method on AppBannerManager so we know when that happens). We'd then need to inject a new run loop when the worker is detected so we can then run and block till completion. I can explore this if you feel strongly about it.
On 2017/06/15 05:58:16, dominickn wrote: > On 2017/06/15 05:47:42, benwells wrote: > > On 2017/06/15 05:41:50, benwells wrote: > > > On 2017/06/15 04:50:12, dominickn wrote: > > > > > > > > > > https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... > > > > File chrome/browser/installable/installable_manager_browsertest.cc > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2942513002/diff/140001/chrome/browser/install... > > > > chrome/browser/installable/installable_manager_browsertest.cc:691: > > > > "/banners/lazy_service_worker_test_page.html?swdelay=500"); > > > > On 2017/06/15 04:42:43, benwells wrote: > > > > > Is the 500 important here? Just asking as 300 changed to 500 in this > > > version, > > > > am > > > > > hoping we don't have the test results depend on this value. > > > > > > > > In my empirical testing, 300ms was originally long enough for both Android > > and > > > > desktop. But it started flaking on desktop when I re-added this test (it > > would > > > > pass the check when it should have failed), hence the extension to 500. > > > > > > > > There's not really any other sane way to have a test for this. I could do > a > > > lot > > > > more mocking to have the manager quit when it gets to the waiting for SW > > step, > > > > then restart it, but that's getting to be a lot of test-only code that > could > > > be > > > > quite fragile. :( > > > > My worry is that it will be fragile like this. If bots run fast (or slowly > > maybe?) for some reason, the tests will fail. > > > > Can you use one of the ExecuteScript varieties in > > https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.h > to > > load the service worker, instead of having it on a delay? > > This would require the mocking I mentioned earlier - reworking the way all the > tests run quite significantly. We'd have to have the existing RunLoop break when > the manager is in the waiting-for-service-worker state (requiring a new virtual > method on AppBannerManager so we know when that happens). We'd then need to > inject a new run loop when the worker is detected so we can then run and block > till completion. > > I can explore this if you feel strongly about it. I feel strongly that we shouldn't have tests that depend on timing constants of a certain value. So I think either explore this method or remove the tests.
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Now with less flaky test.
Description was changed from ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. All clients of the InstallableManager will inherit the waiting behaviour, unless they explicitly opt out via the InstallableParams passed to InstallableManager::GetData. In particular, adding WebAPKs from the add to homescreen menu item will also use the wait-for-worker behaviour. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. Additional tests are added across the InstallableManager and app banner code paths to ensure correctness. BUG=625051, 721881 ========== to ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. All clients of the InstallableManager will inherit the waiting behaviour, unless they explicitly opt out via the InstallableParams passed to InstallableManager::GetData. In particular, adding WebAPKs from the add to homescreen menu item will also use the wait-for-worker behaviour. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. New browser tests for the InstallableManager are added to ensure correctness. BUG=625051, 721881 ==========
Description was changed from ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. All clients of the InstallableManager will inherit the waiting behaviour, unless they explicitly opt out via the InstallableParams passed to InstallableManager::GetData. In particular, adding WebAPKs from the add to homescreen menu item will also use the wait-for-worker behaviour. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. New browser tests for the InstallableManager are added to ensure correctness. BUG=625051, 721881 ========== to ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. All clients of the InstallableManager will inherit the waiting behaviour, unless they explicitly opt out via the InstallableParams passed to InstallableManager::GetData. In particular, adding WebAPKs from the add to homescreen menu item will also use the wait-for-worker behaviour. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. New browser tests for the InstallableManager are added to ensure correctness. BUG=721881 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
lgtm, thanks for updating the tests!
https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; 2. Doesn't your change cause the Web Manifest to be ignored if the page does not have a service worker? e.g. https://googlechrome.github.io/samples/web-application-manifest/index.html 1. I don't fully understand https://crbug.com/721881 It seems that InstallableManager::OnRegistrationStored() should update the cached value regardless of InstallableManager::worker_::is_waiting
https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; On 2017/06/15 15:10:04, pkotwicz wrote: > 2. Doesn't your change cause the Web Manifest to be ignored if the page does not > have a service worker? e.g. > https://googlechrome.github.io/samples/web-application-manifest/index.html The fix for this is to make add to homescreen do what app banners do - get the manifest first, pull necessary things out of it, and then do the rest of the check. That's a follow up. > 1. I don't fully understand https://crbug.com/721881 It seems that > InstallableManager::OnRegistrationStored() should update the cached value > regardless of InstallableManager::worker_::is_waiting If the check has already finished, we've already dispatched the callback and then there's no way for the InstallableManager to tell the add to homescreen fetcher that something has changed. You end up with this: 1. User presses add to homescreen. No service worker registered yet 2. We do the check, there's no service worker. Run the callback 3. Fetcher determines it's not WebAPK compatible and shows the usual A2HS dialog 4. Service worker is registered. But the fetcher has already finished so there's no point doing anything now. The simple thing to do is as above: ask for the manifest first, and then do the rest of the check.
https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; On 2017/06/15 23:45:38, dominickn wrote: > On 2017/06/15 15:10:04, pkotwicz wrote: > > 2. Doesn't your change cause the Web Manifest to be ignored if the page does > not > > have a service worker? e.g. > > https://googlechrome.github.io/samples/web-application-manifest/index.html > > The fix for this is to make add to homescreen do what app banners do - get the > manifest first, pull necessary things out of it, and then do the rest of the > check. That's a follow up. We will need to fetch the icons in the first call which gets the manifest. Otherwise, we would ignore icons in the Web Manifest if the web page does not have a service worker We will need to add a timeout to webapk_update_data_fetcher.cc We want to update WebAPKs for critical updates even if they no longer have a service worker > > > 1. I don't fully understand https://crbug.com/721881 It seems that > > InstallableManager::OnRegistrationStored() should update the cached value > > regardless of InstallableManager::worker_::is_waiting > > If the check has already finished, we've already dispatched the callback and > then there's no way for the InstallableManager to tell the add to homescreen > fetcher that something has changed. You end up with this: > > 1. User presses add to homescreen. No service worker registered yet > 2. We do the check, there's no service worker. Run the callback > 3. Fetcher determines it's not WebAPK compatible and shows the usual A2HS dialog > 4. Service worker is registered. But the fetcher has already finished so there's > no point doing anything now. > > The simple thing to do is as above: ask for the manifest first, and then do the > rest of the check. https://codereview.chromium.org/2942513002/diff/160001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/160001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; There aren't any callers which set this to false. Should we have the parameter? If we have the parameter, we should handle the case of 1) InstallableManager::GetData() being called with wait_for_worker=false before the service worker being registered then 2) The service worker being registered then 3) InstallableManager::GetData() being called with wait_for_worker=true
https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; On 2017/06/16 23:07:43, pkotwicz wrote: > On 2017/06/15 23:45:38, dominickn wrote: > > On 2017/06/15 15:10:04, pkotwicz wrote: > > > 2. Doesn't your change cause the Web Manifest to be ignored if the page does > > not > > > have a service worker? e.g. > > > https://googlechrome.github.io/samples/web-application-manifest/index.html > > > > The fix for this is to make add to homescreen do what app banners do - get the > > manifest first, pull necessary things out of it, and then do the rest of the > > check. That's a follow up. > > We will need to fetch the icons in the first call which gets the manifest. > Otherwise, we would ignore icons in the Web Manifest if the web page does not > have a service worker No, we'll ignore icons if the web page has a manifest *that's WebAPKable*, because that is checked before a service worker. We can easily fetch manifest + icon, then check installability with InstallableManager. > > We will need to add a timeout to webapk_update_data_fetcher.cc We want to update > WebAPKs for critical updates even if they no longer have a service worker Off topic: I feel like what should happen is if a WebAPK loses its service worker is that it should just start opening in a tab in Chrome. It's a pretty bad experience for users if their WebAPK suddenly can't work offline any more. https://codereview.chromium.org/2942513002/diff/160001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/160001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; On 2017/06/16 23:07:43, pkotwicz wrote: > There aren't any callers which set this to false. Should we have the parameter? > If we have the parameter, we should handle the case of > > 1) InstallableManager::GetData() being called with wait_for_worker=false before > the service worker being registered > then > 2) The service worker being registered > then > 3) InstallableManager::GetData() being called with wait_for_worker=true This is set to false in app_banner_manager.cc when |triggered_by_devtools| is true. This is so developers who are testing banners via devtools never wait for a service worker (that scenario doesn't make sense). All three of your scenarios are explicitly tested in installable_manager_browsertest.cc.
pkotwicz: unless you have any other concerns here, I'll land this tomorrow.
https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; We need to decide whether to launch a page in full screen mode prior to loading the page. Thus, we cannot check whether the page has a service worker registered prior to deciding whether to launch the page in full screen mode. https://codereview.chromium.org/2942513002/diff/160001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/160001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; Sorry for the confusion. I had this test in particular in mind IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest, CheckDontWaitThenWaitForServiceWorker) { content::WebContents* web_contents = browser()->tab_strip_model()->GetActiveWebContents(); base::RunLoop sw_run_loop; auto manager = base::MakeUnique<LazyWorkerInstallableManager>( web_contents, sw_run_loop.QuitClosure()); // Load a URL with no service worker. GURL test_url = embedded_test_server()->GetURL( "/banners/manifest_no_service_worker.html"); ui_test_utils::NavigateToURL(browser(), test_url); { base::RunLoop tester_run_loop; std::unique_ptr<CallbackTester> tester( new CallbackTester(tester_run_loop.QuitClosure())); InstallableParams params = GetWebAppParams(); params.wait_for_worker = false; manager->GetData(params, base::Bind(&CallbackTester::OnDidFinishInstallableCheck, base::Unretained(tester.get()))); tester_run_loop.Run(); // We should have returned with an error. EXPECT_FALSE(tester->manifest().IsEmpty()); EXPECT_FALSE(tester->is_installable()); EXPECT_EQ(NO_MATCHING_SERVICE_WORKER, tester->error_code()); } { base::RunLoop tester_run_loop; std::unique_ptr<CallbackTester> tester( new CallbackTester(tester_run_loop.QuitClosure())); InstallableParams params = GetWebAppParams(); params.wait_for_worker = true; manager->GetData(params, base::Bind(&CallbackTester::OnDidFinishInstallableCheck, base::Unretained(tester.get()))); sw_run_loop.Run(); EXPECT_TRUE(content::ExecuteScript( web_contents, "navigator.serviceWorker.register('service_worker.js');")); tester_run_loop.Run(); // The callback should tell us that the page is installable EXPECT_FALSE(tester->manifest().IsEmpty()); EXPECT_TRUE(tester->is_installable()); EXPECT_EQ(NO_ERROR_DETECTED, tester->error_code()); } }
The CQ bit was checked by dominickn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/100001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; On 2017/06/19 18:46:50, pkotwicz wrote: > We need to decide whether to launch a page in full screen mode prior to loading > the page. Thus, we cannot check whether the page has a service worker registered > prior to deciding whether to launch the page in full screen mode. As discussed offline, this shouldn't be an issue because updating happens after launch, and affects the next launch, not the current one. https://codereview.chromium.org/2942513002/diff/160001/chrome/browser/install... File chrome/browser/installable/installable_manager.h (right): https://codereview.chromium.org/2942513002/diff/160001/chrome/browser/install... chrome/browser/installable/installable_manager.h:54: bool wait_for_worker = true; That makes sense now. Added the proposed test and stopped caching a NO_MATCHING_SERVICE_WORKER error.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. Thank you for addressing my concerns :)
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benwells@chromium.org Link to the patchset: https://codereview.chromium.org/2942513002/#ps180001 (title: "Don't cache NO_MATCHING_SERVICE_WORKER")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1497938738832980,
"parent_rev": "84990bcd8d57908c3a790cecbba7cdb2649252a3", "commit_rev":
"14132ac523af4ee5ee38bd5ae0166d151015a786"}
Message was sent while issue was closed.
Description was changed from ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. All clients of the InstallableManager will inherit the waiting behaviour, unless they explicitly opt out via the InstallableParams passed to InstallableManager::GetData. In particular, adding WebAPKs from the add to homescreen menu item will also use the wait-for-worker behaviour. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. New browser tests for the InstallableManager are added to ensure correctness. BUG=721881 ========== to ========== Allow banners to trigger on sites which don't register a service worker onload. This CL makes the InstallableManager a ServiceWorkerContextObserver, which allows it to fall back to waiting for a service worker registration event in the case where a site has not loaded its service worker when the banner check is triggered. All clients of the InstallableManager will inherit the waiting behaviour, unless they explicitly opt out via the InstallableParams passed to InstallableManager::GetData. In particular, adding WebAPKs from the add to homescreen menu item will also use the wait-for-worker behaviour. A key difference is that InstallableManager::GetData may now wait forever (or until the page is navigated) without calling the specified callback in the case where a site never registers a service worker. New browser tests for the InstallableManager are added to ensure correctness. BUG=721881 Review-Url: https://codereview.chromium.org/2942513002 Cr-Commit-Position: refs/heads/master@{#480751} Committed: https://chromium.googlesource.com/chromium/src/+/14132ac523af4ee5ee38bd5ae016... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/14132ac523af4ee5ee38bd5ae016... |
