Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2245063003:
ServiceWorker: Call SyncMatchingRegistration when document_url is changed (Closed)
|
Created:
4 years, 4 months ago by shimazu Modified:
4 years, 4 months ago Reviewers:
nhiroki CC:
michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, nhiroki, falken, kinuko+serviceworker, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Call SyncMatchingRegistration when document_url is changed
For details: https://crbug.com/634222
BUG=634222, 619294, 454250
Committed: https://crrev.com/90ac0459a2f02d2eaae991314f9b540f9fa97f4a
Cr-Commit-Position: refs/heads/master@{#413682}
Patch Set 1 #Patch Set 2 : Update #Patch Set 3 : Add SyncMatchingRegistration and update tests #
Total comments: 33
Patch Set 4 : Incorporate with nhiroki's comments #
Total comments: 29
Patch Set 5 : Use arrow function and add a comment #
Total comments: 20
Patch Set 6 : Incorporate with the review comments #
Total comments: 6
Patch Set 7 : Updated comments #Messages
Total messages: 21 (6 generated)
Description was changed from ========== ServiceWorker: Add a layouttest for claim after redirect BUG=634222,619294 ========== to ========== ServiceWorker: Add a layouttest for claim after redirect BUG=634222,619294 ==========
Description was changed from ========== ServiceWorker: Add a layouttest for claim after redirect BUG=634222,619294 ========== to ========== ServiceWorker: Call SyncMatchingRegistration when document_url is changed For details: https://crbug.com/634222 BUG=634222,619294,454250 ==========
shimazu@chromium.org changed reviewers: + nhiroki@chromium.org
ptal
https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:596: auto* registration = it.second.get(); line 605 does not use auto*. Can you make them consistent? (I'd prefer to clarify the type here like line 605 because |it.second.get()| does not really imply its type) https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:600: matching_registrations_.clear(); How about making... void ServiceWorkerProviderHost::RemoveAllMatchingRegistrations() { for (const auto& it : matching_registrations_) { auto* registration = it.second.get(); DecreaseProcessReference(registration->pattern()); registration->RemoveListener(this); } matching_registrations_.clear(); } and calling it here and in ~ServiceWorkerProviderHost()? https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:602: const std::map<int64_t, ServiceWorkerRegistration*>& registrations = const auto& ? https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_provider_host_unittest.cc (right): https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_provider_host_unittest.cc:157: ASSERT_EQ(provider_host1_->MatchRegistration(), registration2_); An expected value should be placed at left-side (this is inverse to JS's assertion...) https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html (right): https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:18: /* Different origin from the registration */ "//" is more common than "/**/" in our layout tests. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:20: + 'resources/claim-with-redirect-iframe.html?redirected'; '+' operator should be placed at the end of the previous line. [1] http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style "Where this style guide is not explicit, follow the Google JavaScript style guide because it is at least comprehensive." [2] https://google.github.io/styleguide/javascriptguide.xml#Code_formatting "all wrapped lines should be indented either left-aligned to a sibling expression above" https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:35: console.log('onmessage: ', e.data); Can you remove this debug log? https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:41: nit: There is an extra line. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:47: .then(function(data) { I suspect |data| is undefined because the previous {} block returns nothing. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html (right): https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:10: function get_query_params(url) { URLSearchParams could be available instead of this: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:30: /* Execute task for each parameter */ ditto(comment format) https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:34: console.log('[iframe] register: ', worker_url); Can you remove these console outputs? https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:44: r.unregister(); Probably we should wait until unregister() is complete and then send a result to the window for avoiding flakiness. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:59: } Can you reorder these if/else-if blocks in register->redirect->update->unregister to make them consistent with claim-with-redirect.html? I think it's a bit more readable. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:63: executeTask(e.data); We may want to wait until the task is complete like this: aEL('message', function(e) { // Make executeTask() return a promise. e.waitUntil(executeTask(e.data)); }); https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php (right): https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:7: echo <<<EOF Here-document may not be necessary if you put "?>" here. <?php header(...); header(...); header(...); echo '// ' . microtime(); ?> addEventListener(...);
updated this patch! ptal again:) https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:596: auto* registration = it.second.get(); On 2016/08/19 05:26:51, nhiroki wrote: > line 605 does not use auto*. Can you make them consistent? > > (I'd prefer to clarify the type here like line 605 because |it.second.get()| > does not really imply its type) Done. https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:600: matching_registrations_.clear(); On 2016/08/19 05:26:51, nhiroki wrote: > How about making... > > void ServiceWorkerProviderHost::RemoveAllMatchingRegistrations() { > for (const auto& it : matching_registrations_) { > auto* registration = it.second.get(); > DecreaseProcessReference(registration->pattern()); > registration->RemoveListener(this); > } > matching_registrations_.clear(); > } > > and calling it here and in ~ServiceWorkerProviderHost()? Done. https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:602: const std::map<int64_t, ServiceWorkerRegistration*>& registrations = On 2016/08/19 05:26:51, nhiroki wrote: > const auto& ? Done. https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_provider_host_unittest.cc (right): https://codereview.chromium.org/2245063003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_provider_host_unittest.cc:157: ASSERT_EQ(provider_host1_->MatchRegistration(), registration2_); On 2016/08/19 05:26:51, nhiroki wrote: > An expected value should be placed at left-side (this is inverse to JS's > assertion...) Done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html (right): https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:18: /* Different origin from the registration */ On 2016/08/19 05:26:51, nhiroki wrote: > "//" is more common than "/**/" in our layout tests. Done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:20: + 'resources/claim-with-redirect-iframe.html?redirected'; On 2016/08/19 05:26:52, nhiroki wrote: > '+' operator should be placed at the end of the previous line. > > [1] http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style > "Where this style guide is not explicit, follow the Google JavaScript style > guide because it is at least comprehensive." > > [2] https://google.github.io/styleguide/javascriptguide.xml#Code_formatting > "all wrapped lines should be indented either left-aligned to a sibling > expression above" Done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:35: console.log('onmessage: ', e.data); On 2016/08/19 05:26:52, nhiroki wrote: > Can you remove this debug log? Oops, done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:41: On 2016/08/19 05:26:51, nhiroki wrote: > nit: There is an extra line. Done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:47: .then(function(data) { On 2016/08/19 05:26:52, nhiroki wrote: > I suspect |data| is undefined because the previous {} block returns nothing. My aim was that waiting_resolver will be resolved when onmessage is called, so data will be a message posted from an iframe. Is this correct? https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html (right): https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:10: function get_query_params(url) { On 2016/08/19 05:26:52, nhiroki wrote: > URLSearchParams could be available instead of this: > https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams Done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:30: /* Execute task for each parameter */ On 2016/08/19 05:26:52, nhiroki wrote: > ditto(comment format) Done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:34: console.log('[iframe] register: ', worker_url); On 2016/08/19 05:26:52, nhiroki wrote: > Can you remove these console outputs? Done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:44: r.unregister(); On 2016/08/19 05:26:52, nhiroki wrote: > Probably we should wait until unregister() is complete and then send a result to > the window for avoiding flakiness. Done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:59: } On 2016/08/19 05:26:52, nhiroki wrote: > Can you reorder these if/else-if blocks in > register->redirect->update->unregister to make them consistent with > claim-with-redirect.html? I think it's a bit more readable. Done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:63: executeTask(e.data); On 2016/08/19 05:26:52, nhiroki wrote: > We may want to wait until the task is complete like this: > > aEL('message', function(e) { > // Make executeTask() return a promise. > e.waitUntil(executeTask(e.data)); > }); Done. https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php (right): https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:7: echo <<<EOF On 2016/08/19 05:26:52, nhiroki wrote: > Here-document may not be necessary if you put "?>" here. > > > <?php > header(...); > header(...); > header(...); > echo '// ' . microtime(); > ?> > > addEventListener(...); Done.
https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html (right): https://codereview.chromium.org/2245063003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:47: .then(function(data) { On 2016/08/19 09:43:15, shimazu wrote: > On 2016/08/19 05:26:52, nhiroki wrote: > > I suspect |data| is undefined because the previous {} block returns nothing. > > My aim was that waiting_resolver will be resolved when onmessage is called, so > data will be a message posted from an iframe. > Is this correct? You're right. Sorry, I misread this part :p https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:34: addEventListener('message', function(e) { Optional: These days we use arrow-functions in layout tests. For example, https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:43: with_iframe(url); Should we remove frames before the test finishes? https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:50: promise_test(function(t) { I think this test wouldn't be so descriptive. Can you add a comment about what this test wants to confirm? https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:53: return assert_with_iframe(REDIRECT_IFRAME_URL, 'redirected'); Can you explain how this redirect case works? As far as I can see, this just opens a new iframe with REDIRECT_IFRAME_URL. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:10: function get_query_params(url) { Is this no longer necessary? https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:67: addEventListener('message', function(e) { Who sends a message to this frame? https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:68: e.waitUntil(executeTask(e.data)); Oops, sorry!! I assumed this file is a sw script, but this is actually iframe html. Therefore, we cannot (don't have to) use waitUntil() here. And we also don't have to return a promise in executeTask()... https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:11: }); Is this noop handler necessary? https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:14: console.log('install; skipwaiting'); Please remove console logs. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:15: self.skipWaiting(); event.waitUntil(self.skipWaiting()); (I'm assuming this is a sw script :p) https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:19: console.log('activated; claim will be called'); ditto. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:20: self.clients.claim(); event.waitUntil(self.clients.claim());
https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:53: return assert_with_iframe(REDIRECT_IFRAME_URL, 'redirected'); On 2016/08/19 14:32:53, nhiroki wrote: > Can you explain how this redirect case works? As far as I can see, this just > opens a new iframe with REDIRECT_IFRAME_URL. OK, I understand that REDIRECT_IFRAME_URL points to redirect.php and it redirects a request to a URL given by URL parameter.
Updated. PTAL again:) https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:34: addEventListener('message', function(e) { On 2016/08/19 14:32:53, nhiroki wrote: > Optional: These days we use arrow-functions in layout tests. For example, > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... Done. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:43: with_iframe(url); On 2016/08/19 14:32:53, nhiroki wrote: > Should we remove frames before the test finishes? with_iframe removes its own frame after all tests are finished: https://crrev.com/1859343002 https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:50: promise_test(function(t) { On 2016/08/19 14:32:53, nhiroki wrote: > I think this test wouldn't be so descriptive. Can you add a comment about what > this test wants to confirm? Done. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:53: return assert_with_iframe(REDIRECT_IFRAME_URL, 'redirected'); On 2016/08/19 14:40:18, nhiroki wrote: > On 2016/08/19 14:32:53, nhiroki wrote: > > Can you explain how this redirect case works? As far as I can see, this just > > opens a new iframe with REDIRECT_IFRAME_URL. > > OK, I understand that REDIRECT_IFRAME_URL points to redirect.php and it > redirects a request to a URL given by URL parameter. Acknowledged. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:10: function get_query_params(url) { On 2016/08/19 14:32:53, nhiroki wrote: > Is this no longer necessary? Yes, sorry! Removed. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:67: addEventListener('message', function(e) { On 2016/08/19 14:32:53, nhiroki wrote: > Who sends a message to this frame? Ah, I just forgot to remove it. This was used for a temporal testing. Removed. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:68: e.waitUntil(executeTask(e.data)); On 2016/08/19 14:32:53, nhiroki wrote: > Oops, sorry!! I assumed this file is a sw script, but this is actually iframe > html. Therefore, we cannot (don't have to) use waitUntil() here. And we also > don't have to return a promise in executeTask()... Oh, I don't know the message event doesn't have waitUntil. I reverted not to return a promise. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:11: }); On 2016/08/19 14:32:53, nhiroki wrote: > Is this noop handler necessary? Yes, necessary. This is for the browser not to skip this script. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:14: console.log('install; skipwaiting'); On 2016/08/19 14:32:53, nhiroki wrote: > Please remove console logs. Oops, sorry. Done. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:19: console.log('activated; claim will be called'); On 2016/08/19 14:32:54, nhiroki wrote: > ditto. Done. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:20: self.clients.claim(); On 2016/08/19 14:32:54, nhiroki wrote: > event.waitUntil(self.clients.claim()); Done.
Looks good overall. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:11: }); On 2016/08/22 02:38:10, shimazu wrote: > On 2016/08/19 14:32:53, nhiroki wrote: > > Is this noop handler necessary? > > Yes, necessary. This is for the browser not to skip this script. I may not still understand this. 'install' and 'activate' event handlers should be fired regardless of the existence of 'fetch' event handler, right? Or, do you want to fire the no-op 'fetch' event handler for some reason? https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:182: SyncMatchingRegistrations(); We may not have to sync up matching registrations if SetDocumentUrl() is called for controller via ServiceWorkerDispatcherHost::OnSetHostedVersion(). if (IsProviderForClient()) SyncMatchingRegistrations(); https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:140: void SetDocumentUrl(const GURL& url); Can you add a comment that this also updates matching registrations? https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:311: // Sync all live and matching registrations up with the context core This sounds a bit strange because matching registrations should be synced up not with the context core but with all live registrations. We could rewrite this like... // Syncs up matching registrations with live registrations. https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:312: void SyncMatchingRegistrations(); Can you add a blank line after this line? https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:313: // Discard all references to matching registrations s/Discard/Discard/ From the style guide: "These comments should be descriptive ("Opens the file") rather than imperative ("Open the file")" https://google.github.io/styleguide/cppguide.html#Function_Comments https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:314: void RemoveAllMatchingRegistration(); RemoveAllMatchingRegistration"s"() https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_provider_host_unittest.cc (right): https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host_unittest.cc:106: ASSERT_TRUE(PatternHasProcessToRun(registration1_->pattern())); Can you add a blank line after this line? https://codereview.chromium.org/2245063003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html (right): https://codereview.chromium.org/2245063003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:40: function assert_with_iframe(url, message) { s/message/expected_message/ for readablity https://codereview.chromium.org/2245063003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:45: .then(data => { It looks a bit strange that "})" is aligned with ".then". Maybe should it be like this? return new Promise(resolve => { waiting_resolver = resolve; with_iframe(url); }) .then(data => assert_equals(data.message, message)); https://codereview.chromium.org/2245063003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html (right): https://codereview.chromium.org/2245063003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:23: }); .then(r => send_result('registered')); ... would be simpler here and elsewhere.
Updated:) https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:11: }); On 2016/08/22 06:35:49, nhiroki wrote: > On 2016/08/22 02:38:10, shimazu wrote: > > On 2016/08/19 14:32:53, nhiroki wrote: > > > Is this noop handler necessary? > > > > Yes, necessary. This is for the browser not to skip this script. > > I may not still understand this. > > 'install' and 'activate' event handlers should be fired regardless of the > existence of 'fetch' event handler, right? Yes, install and activate will be fired when registration.update() is called. > Or, do you want to fire the no-op > 'fetch' event handler for some reason? This fetch is triggered when requesting redirect.php, and I thought if sw was skipped, initialization of swph would be also skipped. As far as now I check the code around SWPH::DidLookupRegistrationForMainResource again, our current implementation seems to have no problem, but I think it might be better to have fetch handler for simulating real use cases (and bypassing the no-fetch optimization clearly). WDYT? https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:182: SyncMatchingRegistrations(); On 2016/08/22 06:35:49, nhiroki wrote: > We may not have to sync up matching registrations if SetDocumentUrl() is called > for controller via ServiceWorkerDispatcherHost::OnSetHostedVersion(). > > if (IsProviderForClient()) > SyncMatchingRegistrations(); Done. https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:140: void SetDocumentUrl(const GURL& url); On 2016/08/22 06:35:49, nhiroki wrote: > Can you add a comment that this also updates matching registrations? Done. https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:311: // Sync all live and matching registrations up with the context core On 2016/08/22 06:35:49, nhiroki wrote: > This sounds a bit strange because matching registrations should be synced up not > with the context core but with all live registrations. We could rewrite this > like... > > // Syncs up matching registrations with live registrations. Done. https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:312: void SyncMatchingRegistrations(); On 2016/08/22 06:35:49, nhiroki wrote: > Can you add a blank line after this line? Done. https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:313: // Discard all references to matching registrations On 2016/08/22 06:35:49, nhiroki wrote: > s/Discard/Discard/ > > From the style guide: > > "These comments should be descriptive ("Opens the file") rather than imperative > ("Open the file")" > https://google.github.io/styleguide/cppguide.html#Function_Comments Done. https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:314: void RemoveAllMatchingRegistration(); On 2016/08/22 06:35:49, nhiroki wrote: > RemoveAllMatchingRegistration"s"() Done. https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_provider_host_unittest.cc (right): https://codereview.chromium.org/2245063003/diff/80001/content/browser/service... content/browser/service_worker/service_worker_provider_host_unittest.cc:106: ASSERT_TRUE(PatternHasProcessToRun(registration1_->pattern())); On 2016/08/22 06:35:49, nhiroki wrote: > Can you add a blank line after this line? Done. https://codereview.chromium.org/2245063003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html (right): https://codereview.chromium.org/2245063003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:40: function assert_with_iframe(url, message) { On 2016/08/22 06:35:49, nhiroki wrote: > s/message/expected_message/ for readablity Done. https://codereview.chromium.org/2245063003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/claim-with-redirect.html:45: .then(data => { On 2016/08/22 06:35:49, nhiroki wrote: > It looks a bit strange that "})" is aligned with ".then". Maybe should it be > like this? > > return new Promise(resolve => { > waiting_resolver = resolve; > with_iframe(url); > }) > .then(data => assert_equals(data.message, message)); Done. https://codereview.chromium.org/2245063003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html (right): https://codereview.chromium.org/2245063003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/claim-with-redirect-iframe.html:23: }); On 2016/08/22 06:35:50, nhiroki wrote: > .then(r => send_result('registered')); > > ... would be simpler here and elsewhere. Done.
Almost there. Minor comments only. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:11: }); On 2016/08/22 09:29:40, shimazu wrote: > On 2016/08/22 06:35:49, nhiroki wrote: > > On 2016/08/22 02:38:10, shimazu wrote: > > > On 2016/08/19 14:32:53, nhiroki wrote: > > > > Is this noop handler necessary? > > > > > > Yes, necessary. This is for the browser not to skip this script. > > > > I may not still understand this. > > > > > 'install' and 'activate' event handlers should be fired regardless of the > > existence of 'fetch' event handler, right? > > Yes, install and activate will be fired when registration.update() is called. > > > Or, do you want to fire the no-op > > 'fetch' event handler for some reason? > > This fetch is triggered when requesting redirect.php, and I thought if sw was > skipped, initialization of swph would be also skipped. > > As far as now I check the code around SWPH::DidLookupRegistrationForMainResource > again, our current implementation seems to have no problem, but I think it might > be better to have fetch handler for simulating real use cases (and bypassing the > no-fetch optimization clearly). > > WDYT? I see, thank you for the clarification. Keeping a fetch handler for explicitly avoiding the optimization sounds reasonable. I think future code readers may not know this optimization, so can we have a comment why we have a no-op handler here? https://codereview.chromium.org/2245063003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2245063003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:140: // SetDocumentUrl sets the |document_url_|. When this object is for a client, "SetDocumentUrl" looks redundant. "Sets the |document_url_|." would be sufficient. https://codereview.chromium.org/2245063003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:314: // Syncs matching registrations with live registrations Missing trailing dot. https://codereview.chromium.org/2245063003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:317: // Discards all references to matching registrations ditto.
https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:11: }); On 2016/08/22 12:41:37, nhiroki wrote: > On 2016/08/22 09:29:40, shimazu wrote: > > On 2016/08/22 06:35:49, nhiroki wrote: > > > On 2016/08/22 02:38:10, shimazu wrote: > > > > On 2016/08/19 14:32:53, nhiroki wrote: > > > > > Is this noop handler necessary? > > > > > > > > Yes, necessary. This is for the browser not to skip this script. > > > > > > I may not still understand this. > > > > > > > > 'install' and 'activate' event handlers should be fired regardless of the > > > existence of 'fetch' event handler, right? > > > > Yes, install and activate will be fired when registration.update() is called. > > > > > Or, do you want to fire the no-op > > > 'fetch' event handler for some reason? > > > > This fetch is triggered when requesting redirect.php, and I thought if sw was > > skipped, initialization of swph would be also skipped. > > > > As far as now I check the code around > SWPH::DidLookupRegistrationForMainResource > > again, our current implementation seems to have no problem, but I think it > might > > be better to have fetch handler for simulating real use cases (and bypassing > the > > no-fetch optimization clearly). > > > > WDYT? > > I see, thank you for the clarification. Keeping a fetch handler for explicitly > avoiding the optimization sounds reasonable. I think future code readers may not > know this optimization, so can we have a comment why we have a no-op handler > here? By the way, if this test depends on Chromium-specific behavior, it'd be better to move it to http/tests/serviceworker/chromium.
Updated comments:) ptal again. https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php (right): https://codereview.chromium.org/2245063003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/update-claim-worker.php:11: }); On 2016/08/23 01:31:46, nhiroki wrote: > On 2016/08/22 12:41:37, nhiroki wrote: > > On 2016/08/22 09:29:40, shimazu wrote: > > > On 2016/08/22 06:35:49, nhiroki wrote: > > > > On 2016/08/22 02:38:10, shimazu wrote: > > > > > On 2016/08/19 14:32:53, nhiroki wrote: > > > > > > Is this noop handler necessary? > > > > > > > > > > Yes, necessary. This is for the browser not to skip this script. > > > > > > > > I may not still understand this. > > > > > > > > > > > 'install' and 'activate' event handlers should be fired regardless of the > > > > existence of 'fetch' event handler, right? > > > > > > Yes, install and activate will be fired when registration.update() is > called. > > > > > > > Or, do you want to fire the no-op > > > > 'fetch' event handler for some reason? > > > > > > This fetch is triggered when requesting redirect.php, and I thought if sw > was > > > skipped, initialization of swph would be also skipped. > > > > > > As far as now I check the code around > > SWPH::DidLookupRegistrationForMainResource > > > again, our current implementation seems to have no problem, but I think it > > might > > > be better to have fetch handler for simulating real use cases (and bypassing > > the > > > no-fetch optimization clearly). > > > > > > WDYT? > > > > I see, thank you for the clarification. Keeping a fetch handler for explicitly > > avoiding the optimization sounds reasonable. I think future code readers may > not > > know this optimization, so can we have a comment why we have a no-op handler > > here? > > By the way, if this test depends on Chromium-specific behavior, it'd be better > to move it to http/tests/serviceworker/chromium. Added a comment. This optimization has been already spec'ed at [1], so I think here is fine. [1]: Green box at 15. https://w3c.github.io/ServiceWorker/spec/service_worker/index.html#on-fetch-r... https://codereview.chromium.org/2245063003/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2245063003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:140: // SetDocumentUrl sets the |document_url_|. When this object is for a client, On 2016/08/22 12:41:37, nhiroki wrote: > "SetDocumentUrl" looks redundant. "Sets the |document_url_|." would be > sufficient. Done. https://codereview.chromium.org/2245063003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:314: // Syncs matching registrations with live registrations On 2016/08/22 12:41:37, nhiroki wrote: > Missing trailing dot. Done. https://codereview.chromium.org/2245063003/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:317: // Discards all references to matching registrations On 2016/08/22 12:41:37, nhiroki wrote: > ditto. Done.
LGTM :)
The CQ bit was checked by shimazu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Call SyncMatchingRegistration when document_url is changed For details: https://crbug.com/634222 BUG=634222,619294,454250 ========== to ========== ServiceWorker: Call SyncMatchingRegistration when document_url is changed For details: https://crbug.com/634222 BUG=634222,619294,454250 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Call SyncMatchingRegistration when document_url is changed For details: https://crbug.com/634222 BUG=634222,619294,454250 ========== to ========== ServiceWorker: Call SyncMatchingRegistration when document_url is changed For details: https://crbug.com/634222 BUG=634222,619294,454250 Committed: https://crrev.com/90ac0459a2f02d2eaae991314f9b540f9fa97f4a Cr-Commit-Position: refs/heads/master@{#413682} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/90ac0459a2f02d2eaae991314f9b540f9fa97f4a Cr-Commit-Position: refs/heads/master@{#413682} |
