Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1432823003:
Add a test to cover service worker update codepath when extension is updated (Closed)
|
Created:
5 years, 1 month ago by lazyboy Modified:
5 years ago CC:
chromium-reviews, michaeln, extensions-reviews_chromium.org, tzik, serviceworker-reviews, jam, kinuko+serviceworker, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, chromium-apps-reviews_chromium.org, kinuko+watch, jsbell+serviceworker_chromium.org, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a test to cover service worker update codepath when extension is updated.
The updated extension's service worker calls skipWaiting() on its oninstall
handler to let the new service worker control its clients right away.
BUG=533065
Committed: https://crrev.com/c3e763a2164e421055b2a01d5b21426a9abca83a
Cr-Commit-Position: refs/heads/master@{#364195}
Patch Set 1 #Patch Set 2 : add missing files #Patch Set 3 : test updated #
Total comments: 4
Patch Set 4 : use set_force_update_on_page_load() + git cl format #
Total comments: 12
Patch Set 5 : address comments + git cl format #
Total comments: 2
Patch Set 6 : use swglobalctx.oninstall + onactivate to replace active worker in test == no code change! #Patch Set 7 : remove clients.claim #
Total comments: 10
Patch Set 8 : remove SW.register() from v2 extension #Patch Set 9 : remove SW.register() from v2 extension #Messages
Total messages: 31 (11 generated)
Description was changed from ========== [WIP] Test for invalidation of SW on extension upgrade. The test is correct, it should pass once the underlying issue is fixed. BUG=533065 ========== to ========== [WIP] Invalid SW on extension upgrade. The test is correct, and it at patchset #3 BUG=533065 ==========
Description was changed from ========== [WIP] Invalid SW on extension upgrade. The test is correct, and it at patchset #3 BUG=533065 ========== to ========== [WIP] Invalidate SW on extension upgrade. The test is correct, and it at patchset #3 BUG=533065 ==========
lazyboy@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Question below: https://chromiumcodereview.appspot.com/1432823003/diff/40001/extensions/brows... File extensions/browser/service_worker_manager.cc (right): https://chromiumcodereview.appspot.com/1432823003/diff/40001/extensions/brows... extensions/browser/service_worker_manager.cc:58: // TODO(lazyboy): This is racy because this relies on DeleteForOrigin() to How can we fix this? Or, is there any guarantee that would make the deletion code run before the browser sees the next SW registration? (== no race) I'm still learning the threading here, how many threads would involve this call below? SW thread + File thread to store/retrieve the worker?
https://chromiumcodereview.appspot.com/1432823003/diff/40001/extensions/brows... File extensions/browser/service_worker_manager.cc (right): https://chromiumcodereview.appspot.com/1432823003/diff/40001/extensions/brows... extensions/browser/service_worker_manager.cc:58: // TODO(lazyboy): This is racy because this relies on DeleteForOrigin() to On 2015/11/10 00:45:44, lazyboy wrote: > How can we fix this? > Or, is there any guarantee that would make the deletion code run before the > browser sees the next SW registration? (== no race) > > I'm still learning the threading here, how many threads would involve this call > below? SW thread + File thread to store/retrieve the worker? Hmm... I would hope that it wouldn't be racy, because any registration would also have to do the same thread-hopping, and we ensure that posted tasks are FIFO. Or does registration/access somehow bypass the thread-hop? And I think most of these SW calls execute on the IO thread, so we should only be involving the UI thread (here) and the IO thread (for the SW update), IIUC. https://chromiumcodereview.appspot.com/1432823003/diff/40001/extensions/brows... extensions/browser/service_worker_manager.cc:63: ->DeleteForOrigin(extension->url(), base::Bind(&EmptySuccessCallback)); (Sorry if this was just a placeholder and I'm saying something you already know.) We don't actually want to delete the SW for the orgin, because we don't want to invalidate any cached data or lose the registration. We just want to make sure we re-fetch the service worker (similar to what we do after 24 hours on a normal web page).
https://chromiumcodereview.appspot.com/1432823003/diff/40001/extensions/brows... File extensions/browser/service_worker_manager.cc (right): https://chromiumcodereview.appspot.com/1432823003/diff/40001/extensions/brows... extensions/browser/service_worker_manager.cc:63: ->DeleteForOrigin(extension->url(), base::Bind(&EmptySuccessCallback)); On 2015/11/10 16:59:50, Devlin wrote: > (Sorry if this was just a placeholder and I'm saying something you already > know.) > We don't actually want to delete the SW for the orgin, because we don't want to > invalidate any cached data or lose the registration. We just want to make sure > we re-fetch the service worker (similar to what we do after 24 hours on a normal > web page). I've changed this to eventually call ServiceWorkerRegistration::set_force_update_on_page_load(). That seems to PASS the tests. Let me know if this sounds about right, then I'll pass the CL to an SW OWNER.
Description was changed from ========== [WIP] Invalidate SW on extension upgrade. The test is correct, and it at patchset #3 BUG=533065 ========== to ========== [WIP] Invalidate SW on extension upgrade. The test is correct, and it passes at patchset #3/#4 BUG=533065 ==========
LGTM with a few small nits. falken@ has been handling a lot of the service worker + extension patches in the past, so I'd probably recommend him as an SW owner since he'll have the most context (though I'm sure anyone would be fine). https://chromiumcodereview.appspot.com/1432823003/diff/60001/chrome/browser/e... File chrome/browser/extensions/service_worker_apitest.cc (right): https://chromiumcodereview.appspot.com/1432823003/diff/60001/chrome/browser/e... chrome/browser/extensions/service_worker_apitest.cc:152: ExtensionService* service = nit: there's an extension_service() accessor on ExtensionBrowserTest (which this extends). https://chromiumcodereview.appspot.com/1432823003/diff/60001/chrome/browser/e... chrome/browser/extensions/service_worker_apitest.cc:158: EXPECT_TRUE(service->GetExtensionById(kId, false) != NULL); nit: GetExtensionById is deprecated. Prefer ExtensionRegistry::enabled_extensions().GetByID(). https://chromiumcodereview.appspot.com/1432823003/diff/60001/chrome/browser/e... chrome/browser/extensions/service_worker_apitest.cc:160: const char* kScript = "window.testResolver();"; nitty nit: pick one between const char[] (line 154) and const char*. :) https://chromiumcodereview.appspot.com/1432823003/diff/60001/chrome/browser/e... chrome/browser/extensions/service_worker_apitest.cc:161: EXPECT_EQ("true", ExecuteScriptInBackgroundPage(kId, kScript)); This is an interesting pattern. Usually we would use ExtensionTestMessageListener + chrome.test.sendMessage for this type of thing, but this is kinda nifty, so I'm fine with this approach. My only nit would be that I'd like to see this compare the result to "version 1" and "version 2" explicitly - I'm just a little worried about JS's general happiness to convert things to true/false. (I'm sure it's fine, but peace of mind is worth a lot. :)) https://chromiumcodereview.appspot.com/1432823003/diff/60001/content/public/b... File content/public/browser/service_worker_context.h (right): https://chromiumcodereview.appspot.com/1432823003/diff/60001/content/public/b... content/public/browser/service_worker_context.h:72: virtual void SetForceUpdateOnPageLoadForOrigin(const GURL& origin) = 0; Nit: This probably doesn't fall into the comment above. Let's add a comment for this method. https://chromiumcodereview.appspot.com/1432823003/diff/60001/extensions/brows... File extensions/browser/service_worker_manager.cc (right): https://chromiumcodereview.appspot.com/1432823003/diff/60001/extensions/brows... extensions/browser/service_worker_manager.cc:57: if (is_update) { nit: A brief comment saying why we do this would probably be worthwhile.
Description was changed from ========== [WIP] Invalidate SW on extension upgrade. The test is correct, and it passes at patchset #3/#4 BUG=533065 ========== to ========== Invalidate SW on extension upgrade. Once an extension is updated (new version), we will re-fetch the ServiceWorkers on next registration call. BUG=533065 ==========
lazyboy@chromium.org changed reviewers: + falken@chromium.org
+falken for ServiceWorker changes. https://chromiumcodereview.appspot.com/1432823003/diff/60001/chrome/browser/e... File chrome/browser/extensions/service_worker_apitest.cc (right): https://chromiumcodereview.appspot.com/1432823003/diff/60001/chrome/browser/e... chrome/browser/extensions/service_worker_apitest.cc:152: ExtensionService* service = On 2015/11/11 17:10:29, Devlin wrote: > nit: there's an extension_service() accessor on ExtensionBrowserTest (which this > extends). Not needed anymore. https://chromiumcodereview.appspot.com/1432823003/diff/60001/chrome/browser/e... chrome/browser/extensions/service_worker_apitest.cc:158: EXPECT_TRUE(service->GetExtensionById(kId, false) != NULL); On 2015/11/11 17:10:29, Devlin wrote: > nit: GetExtensionById is deprecated. Prefer > ExtensionRegistry::enabled_extensions().GetByID(). Done. https://chromiumcodereview.appspot.com/1432823003/diff/60001/chrome/browser/e... chrome/browser/extensions/service_worker_apitest.cc:160: const char* kScript = "window.testResolver();"; On 2015/11/11 17:10:29, Devlin wrote: > nitty nit: pick one between const char[] (line 154) and const char*. :) Done. https://chromiumcodereview.appspot.com/1432823003/diff/60001/chrome/browser/e... chrome/browser/extensions/service_worker_apitest.cc:161: EXPECT_EQ("true", ExecuteScriptInBackgroundPage(kId, kScript)); On 2015/11/11 17:10:29, Devlin wrote: > This is an interesting pattern. Usually we would use > ExtensionTestMessageListener + chrome.test.sendMessage for this type of thing, > but this is kinda nifty, so I'm fine with this approach. My only nit would be > that I'd like to see this compare the result to "version 1" and "version 2" > explicitly - I'm just a little worried about JS's general happiness to convert > things to true/false. (I'm sure it's fine, but peace of mind is worth a lot. > :)) Changed to compare with the actual received message from SW. So, the chance of accidentally converting sth to "true" is gone.
(missed hitting "Done" for some comments) https://chromiumcodereview.appspot.com/1432823003/diff/60001/content/public/b... File content/public/browser/service_worker_context.h (right): https://chromiumcodereview.appspot.com/1432823003/diff/60001/content/public/b... content/public/browser/service_worker_context.h:72: virtual void SetForceUpdateOnPageLoadForOrigin(const GURL& origin) = 0; On 2015/11/11 17:10:29, Devlin wrote: > Nit: This probably doesn't fall into the comment above. Let's add a comment for > this method. Done. https://chromiumcodereview.appspot.com/1432823003/diff/60001/extensions/brows... File extensions/browser/service_worker_manager.cc (right): https://chromiumcodereview.appspot.com/1432823003/diff/60001/extensions/brows... extensions/browser/service_worker_manager.cc:57: if (is_update) { On 2015/11/11 17:10:29, Devlin wrote: > nit: A brief comment saying why we do this would probably be worthwhile. Done.
Sorry I'm just getting back from traveling. Will take a look as soon as possible (likely tomorrow).
https://codereview.chromium.org/1432823003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1432823003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.cc:339: registration->set_force_update_on_page_load(true); This is a hacky bit used for the "Force update on page load" checkbox in DevTools. If you set this bit, all page loads in the scope of these service workers are blocked until the worker updates. The worker will update each time regardless of whether the worker script changed. In this patch, I see a couple problems with this approach: - The bit is never unset. So all page loads in this extension will block on an update. - This still looks racy since SetForceUpdateOnPageLoadForOrigin calls the async function GetRegistrationsForOrigin. I'm wondering if it's really necessary for the updated sw to be active the next time the extension is accessed, or if this can be more in the spirit of the SW update model: can extension sw's have soft updates like web sw's (we can also schedule one when the extension is updated). The author can additionally use the SW API to force update when needed: e.g., with ServiceWorkerRegistration.update() and skipWaiting()/claim().
https://codereview.chromium.org/1432823003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1432823003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.cc:339: registration->set_force_update_on_page_load(true); On 2015/11/17 02:10:22, falken (maybe slow) wrote: > This is a hacky bit used for the "Force update on page load" checkbox in > DevTools. If you set this bit, all page loads in the scope of these service > workers are blocked until the worker updates. The worker will update each time > regardless of whether the worker script changed. > > In this patch, I see a couple problems with this approach: > - The bit is never unset. So all page loads in this extension will block on an > update. > - This still looks racy since SetForceUpdateOnPageLoadForOrigin calls the async > function GetRegistrationsForOrigin. > > I'm wondering if it's really necessary for the updated sw to be active the next > time the extension is accessed, or if this can be more in the spirit of the SW > update model: can extension sw's have soft updates like web sw's (we can also > schedule one when the extension is updated). The author can additionally use the > SW API to force update when needed: e.g., with > ServiceWorkerRegistration.update() and skipWaiting()/claim(). Dang - that bit looked so promising! ;) Scheduling an update when the extension is updated sounds perfect (and was the goal of this CL - we just want to make sure we don't have to wait for the normal 24-hour period) - can you give us a pointer to how we would do that?
On 2015/11/17 19:23:56, Devlin wrote: > https://codereview.chromium.org/1432823003/diff/80001/content/browser/service... > File content/browser/service_worker/service_worker_context_wrapper.cc (right): > > https://codereview.chromium.org/1432823003/diff/80001/content/browser/service... > content/browser/service_worker/service_worker_context_wrapper.cc:339: > registration->set_force_update_on_page_load(true); > On 2015/11/17 02:10:22, falken (maybe slow) wrote: > > This is a hacky bit used for the "Force update on page load" checkbox in > > DevTools. If you set this bit, all page loads in the scope of these service > > workers are blocked until the worker updates. The worker will update each time > > regardless of whether the worker script changed. > > > > In this patch, I see a couple problems with this approach: > > - The bit is never unset. So all page loads in this extension will block on an > > update. > > - This still looks racy since SetForceUpdateOnPageLoadForOrigin calls the > async > > function GetRegistrationsForOrigin. > > > > I'm wondering if it's really necessary for the updated sw to be active the > next > > time the extension is accessed, or if this can be more in the spirit of the SW > > update model: can extension sw's have soft updates like web sw's (we can also > > schedule one when the extension is updated). The author can additionally use > the > > SW API to force update when needed: e.g., with > > ServiceWorkerRegistration.update() and skipWaiting()/claim(). > > Dang - that bit looked so promising! ;) > > Scheduling an update when the extension is updated sounds perfect (and was the > goal of this CL - we just want to make sure we don't have to wait for the normal > 24-hour period) - can you give us a pointer to how we would do that? ServiceWorkerContextWrapper::UpdateRegistration which calls ServiceWorkerContextCore::UpdateServiceWorker should do the trick. You'll want to update the comment above that callsite which claims it's only used for DevTools. I'm not totally sure whether you want to force bypassing the HTTP cache or not. If resources fetched from extensions are stored in the HTTP cache you probably do. Hopefully this approach is simpler :)
On 2015/11/18 01:48:06, falken wrote: > On 2015/11/17 19:23:56, Devlin wrote: > > > https://codereview.chromium.org/1432823003/diff/80001/content/browser/service... > > File content/browser/service_worker/service_worker_context_wrapper.cc (right): > > > > > https://codereview.chromium.org/1432823003/diff/80001/content/browser/service... > > content/browser/service_worker/service_worker_context_wrapper.cc:339: > > registration->set_force_update_on_page_load(true); > > On 2015/11/17 02:10:22, falken (maybe slow) wrote: > > > This is a hacky bit used for the "Force update on page load" checkbox in > > > DevTools. If you set this bit, all page loads in the scope of these service > > > workers are blocked until the worker updates. The worker will update each > time > > > regardless of whether the worker script changed. > > > > > > In this patch, I see a couple problems with this approach: > > > - The bit is never unset. So all page loads in this extension will block on > an > > > update. > > > - This still looks racy since SetForceUpdateOnPageLoadForOrigin calls the > > async > > > function GetRegistrationsForOrigin. > > > > > > I'm wondering if it's really necessary for the updated sw to be active the > > next > > > time the extension is accessed, or if this can be more in the spirit of the > SW > > > update model: can extension sw's have soft updates like web sw's (we can > also > > > schedule one when the extension is updated). The author can additionally use > > the > > > SW API to force update when needed: e.g., with > > > ServiceWorkerRegistration.update() and skipWaiting()/claim(). > > > > Dang - that bit looked so promising! ;) > > > > Scheduling an update when the extension is updated sounds perfect (and was the > > goal of this CL - we just want to make sure we don't have to wait for the > normal > > 24-hour period) - can you give us a pointer to how we would do that? > > ServiceWorkerContextWrapper::UpdateRegistration which calls > ServiceWorkerContextCore::UpdateServiceWorker should do the trick. You'll want > to update the comment above that callsite which claims it's only used for > DevTools. I'm not totally sure whether you want to force bypassing the HTTP > cache or not. If resources fetched from extensions are stored in the HTTP cache > you probably do. > > Hopefully this approach is simpler :) With oninstall + onfetch approach, it seems that no change is necessary to retrieve the updated ServiceWorker on the update extension! The oninstall fires correctly on the new ServiceWorker as is. I've updated the tests and reverted the changes on patch set #6.
Awesome if it's working as is! I'd recommend confirming the behavior when the developer forgets to use skipWaiting() or claim() though. Are both necessary? For non-extension SWs: - if skipWaiting() is not called, the incumbent worker remains active until all pages controlled by it are closed - if claim() is not called, then an in-scope page that was opened before the registration was created (via register()) will not be controlled by the worker I'm not totally sure how this works in the land of extension SWs, I imagine an extension that's running in Chrome is considered a page. Do extensions ever "close" like a page? Also, look into whether it matters that the URLs passed to register() differ between v1 and v2. register() early exits if the URL hasn't changed from the installed worker.
FYI, I haven't had a chance to complete testing the scenarios you mention here. I'll get back to working on it soon.
Description was changed from ========== Invalidate SW on extension upgrade. Once an extension is updated (new version), we will re-fetch the ServiceWorkers on next registration call. BUG=533065 ========== to ========== Add a test to cover service worker update codepath when extension is updated. The updated extension's service worker calls skipWaiting() on its oninstall handler to let the new service worker control its clients right away. BUG=533065 ==========
On 2015/11/20 03:19:47, falken wrote: > Awesome if it's working as is! I'd recommend confirming the behavior when the > developer forgets to use skipWaiting() or claim() though. Are both necessary? No, only skipWaiting() is enough. See note about claim() below. > For non-extension SWs: > - if skipWaiting() is not called, the incumbent worker remains active until all > pages controlled by it are closed This seems to work as expected with extensions: What I've done to test this is once the extension is updated, I open an extension page right away and see that old SW is controlling the page still. If I open more extension pages, the old SW controls them as well. Once I close all of the open extension pages and open a new one, I see the new service worker controlling the pages. In a browsertest, I've seen one issue: closing all pages and then opening a new extension page from test seems to run too quickly. I'm assuming that b/c the new page opens before ServiceWorkerHostMsg_ProviderDestroyed is sent, I'm still seeing the old SW on the new page. Instead of closing the page, if I navigated the old pages to sth else (about:blank) and wait for that to finish then it seems to work. Here's the CL to add that test: https://chromiumcodereview.appspot.com/1510573003/ > - if claim() is not called, then an in-scope page that was opened before the > registration was created (via register()) will not be controlled by the worker > We close all extension pages when an extension is updated. So pages "opened before" isn't applicable to extensions IIUC. > I'm not totally sure how this works in the land of extension SWs, I imagine an > extension that's running in Chrome is considered a page. Do extensions ever > "close" like a page? > > Also, look into whether it matters that the URLs passed to register() differ > between v1 and v2. register() early exits if the URL hasn't changed from the > installed worker. I didn't see any difference in my two tests (this CL and 1510573003) when I've changed the filename in two different versions of the extensions. Should I expect sth different?
LGTM! > I didn't see any difference in my two tests (this CL and 1510573003) when I've > changed the filename in two different versions of the extensions. > Should I expect sth different? Responded innline... I was worried the test relies on the URLs changing (e.g., "//v1/sw.js" -> "//v2/sw.js"). I tested locally and it turns out the URL is just "//sw.js" for both versions so there is no change, and update is still occurring so we're good. https://codereview.chromium.org/1432823003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/service_worker_apitest.cc (right): https://codereview.chromium.org/1432823003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/service_worker_apitest.cc:242: .GetByID(kId) != NULL); nit: prefer nullptr for new code (2 places) https://codereview.chromium.org/1432823003/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/service_worker/update/v2/background.js (right): https://codereview.chromium.org/1432823003/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/service_worker/update/v2/background.js:16: navigator.serviceWorker.register('sw.js').then(function() { Since you're testing update, I would just get rid of this register() call. I was worried the test is "cheating" by calling register, which could trigger an update, whereas we want to test that the update occurs automatically. It turns out no cheating is going on, since the script url has not changed, which turns register() into a no-op. For simplicity, you can just remove it though. For that matter, you could get rid of this whole registerServiceWorkerPromise, and only use controllerChangePromise.
lgtm https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/browser/... File chrome/browser/extensions/service_worker_apitest.cc (right): https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/browser/... chrome/browser/extensions/service_worker_apitest.cc:242: .GetByID(kId) != NULL); On 2015/12/09 02:31:41, falken wrote: > nit: prefer nullptr for new code (2 places) Also, I think we can just omit the != nullptr for EXPECT_TRUE. https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/test/dat... File chrome/test/data/extensions/api_test/service_worker/update/v2/background.js (right): https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/test/dat... chrome/test/data/extensions/api_test/service_worker/update/v2/background.js:16: navigator.serviceWorker.register('sw.js').then(function() { On 2015/12/09 02:31:41, falken wrote: > Since you're testing update, I would just get rid of this register() call. I was > worried the test is "cheating" by calling register, which could trigger an > update, whereas we want to test that the update occurs automatically. > > It turns out no cheating is going on, since the script url has not changed, > which turns register() into a no-op. For simplicity, you can just remove it > though. > > For that matter, you could get rid of this whole registerServiceWorkerPromise, > and only use controllerChangePromise. +1. If calling register with the same script url is ever not a no-op, this would invalidate the test (silently). https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/test/dat... chrome/test/data/extensions/api_test/service_worker/update/v2/background.js:30: console.log('Message received from SW: ' + e.data); chrome.test.log (or remove)
https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/browser/... File chrome/browser/extensions/service_worker_apitest.cc (right): https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/browser/... chrome/browser/extensions/service_worker_apitest.cc:242: .GetByID(kId) != NULL); On 2015/12/09 17:09:07, Devlin wrote: > On 2015/12/09 02:31:41, falken wrote: > > nit: prefer nullptr for new code (2 places) > > Also, I think we can just omit the != nullptr for EXPECT_TRUE. Done. https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/browser/... chrome/browser/extensions/service_worker_apitest.cc:242: .GetByID(kId) != NULL); On 2015/12/09 02:31:41, falken wrote: > nit: prefer nullptr for new code (2 places) Obsolete now. https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/test/dat... File chrome/test/data/extensions/api_test/service_worker/update/v2/background.js (right): https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/test/dat... chrome/test/data/extensions/api_test/service_worker/update/v2/background.js:16: navigator.serviceWorker.register('sw.js').then(function() { On 2015/12/09 02:31:41, falken wrote: > Since you're testing update, I would just get rid of this register() call. I was > worried the test is "cheating" by calling register, which could trigger an > update, whereas we want to test that the update occurs automatically. > > It turns out no cheating is going on, since the script url has not changed, > which turns register() into a no-op. For simplicity, you can just remove it > though. > > For that matter, you could get rid of this whole registerServiceWorkerPromise, > and only use controllerChangePromise. Done. https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/test/dat... chrome/test/data/extensions/api_test/service_worker/update/v2/background.js:16: navigator.serviceWorker.register('sw.js').then(function() { On 2015/12/09 17:09:07, Devlin wrote: > On 2015/12/09 02:31:41, falken wrote: > > Since you're testing update, I would just get rid of this register() call. I > was > > worried the test is "cheating" by calling register, which could trigger an > > update, whereas we want to test that the update occurs automatically. > > > > It turns out no cheating is going on, since the script url has not changed, > > which turns register() into a no-op. For simplicity, you can just remove it > > though. > > > > For that matter, you could get rid of this whole registerServiceWorkerPromise, > > and only use controllerChangePromise. > > +1. If calling register with the same script url is ever not a no-op, this would > invalidate the test (silently). Done. https://chromiumcodereview.appspot.com/1432823003/diff/120001/chrome/test/dat... chrome/test/data/extensions/api_test/service_worker/update/v2/background.js:30: console.log('Message received from SW: ' + e.data); On 2015/12/09 17:09:07, Devlin wrote: > chrome.test.log (or remove) Done.
The CQ bit was checked by lazyboy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1432823003/#ps160001 (title: "remove SW.register() from v2 extension")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1432823003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432823003/160001
Message was sent while issue was closed.
Description was changed from ========== Add a test to cover service worker update codepath when extension is updated. The updated extension's service worker calls skipWaiting() on its oninstall handler to let the new service worker control its clients right away. BUG=533065 ========== to ========== Add a test to cover service worker update codepath when extension is updated. The updated extension's service worker calls skipWaiting() on its oninstall handler to let the new service worker control its clients right away. BUG=533065 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add a test to cover service worker update codepath when extension is updated. The updated extension's service worker calls skipWaiting() on its oninstall handler to let the new service worker control its clients right away. BUG=533065 ========== to ========== Add a test to cover service worker update codepath when extension is updated. The updated extension's service worker calls skipWaiting() on its oninstall handler to let the new service worker control its clients right away. BUG=533065 Committed: https://crrev.com/c3e763a2164e421055b2a01d5b21426a9abca83a Cr-Commit-Position: refs/heads/master@{#364195} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c3e763a2164e421055b2a01d5b21426a9abca83a Cr-Commit-Position: refs/heads/master@{#364195} |
