Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
535753002:
ServiceWorker: Implement navigator.serviceWorker.getRegistration [2/3] (Closed)
|
Created:
6 years, 3 months ago by Kunihiko Sakamoto Modified:
6 years, 3 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, kinuko+serviceworker, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Implement navigator.serviceWorker.getRegistration [2/3]
This patch implements the Chromium-side change of
ServiceWorkerContainer#getRegistration().
Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#navigator-service-worker-getRegistration
[1/3] https://codereview.chromium.org/553983010/
[3/3] https://codereview.chromium.org/540823003/
BUG=404951
TEST=content_unittests
Committed: https://crrev.com/0d1c4968ac6d14ee8d250df9641b6434eb9aaaf0
Cr-Commit-Position: refs/heads/master@{#294720}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : rebase #Patch Set 3 : patch for review #
Total comments: 20
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : rebase #
Total comments: 13
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Messages
Total messages: 29 (12 generated)
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
ksakamoto@chromium.org changed reviewers: + nhiroki@chromium.org
Hiroki-san, This is still WIP and no tests yet, but could you take a quick look to see if this approach is correct or not? The corresponding Blink side change is https://codereview.chromium.org/540823003/. https://codereview.chromium.org/535753002/diff/100001/content/browser/service... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/535753002/diff/100001/content/browser/service... content/browser/service_worker/service_worker_context_core.cc:229: storage()->FindRegistrationForDocument( I wonder if we need to post a job to ServiceWorkerJobCoordinator here, instead of calling storage method directly. It's unclear to me how to do so because RegistrationJobMap is keyed by pattern, not document URL.
Looks pretty good. Please proceed in this direction :) https://codereview.chromium.org/535753002/diff/100001/content/browser/service... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/535753002/diff/100001/content/browser/service... content/browser/service_worker/service_worker_context_core.cc:229: storage()->FindRegistrationForDocument( On 2014/09/05 01:35:17, Kunihiko Sakamoto wrote: > I wonder if we need to post a job to ServiceWorkerJobCoordinator here, instead > of calling storage method directly. It's unclear to me how to do so because > RegistrationJobMap is keyed by pattern, not document URL. No problem. You can call the storage method directly as is. NOTE: Multiple register/unregister jobs for the same scope (pattern) could simultaneously be requested because users are likely to open multiple pages in the same domain. JobCoordinator is responsible for serializing those requests. https://codereview.chromium.org/535753002/diff/100001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/535753002/diff/100001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:626: registration->active_version()); Since RegistrationComplete does the same thing, can you factor out the body of this if-statement into a separate function? https://codereview.chromium.org/535753002/diff/100001/content/child/service_w... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/535753002/diff/100001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:314: callbacks->onSuccess(registration); I guess this could be resolved with 'null' if a registration is not found (ie. |registration| is null), but as per the spec[1], we need to resolve a promise with 'undefined' in the case. [1] https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#na...
Patchset #3 (id:140001) has been deleted
ksakamoto@chromium.org changed reviewers: + horo@chromium.org
Thanks for the feedback. Now it's ready for review, PTAL. https://codereview.chromium.org/535753002/diff/100001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/535753002/diff/100001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:626: registration->active_version()); On 2014/09/05 04:55:09, nhiroki wrote: > Since RegistrationComplete does the same thing, can you factor out the body of > this if-statement into a separate function? Done. https://codereview.chromium.org/535753002/diff/100001/content/child/service_w... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/535753002/diff/100001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:314: callbacks->onSuccess(registration); On 2014/09/05 04:55:09, nhiroki wrote: > I guess this could be resolved with 'null' if a registration is not found (ie. > |registration| is null), but as per the spec[1], we need to resolve a promise > with 'undefined' in the case. > > [1] > https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#na... Done (in Blink side).
Looks good overall. https://codereview.chromium.org/535753002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:68: return base::Bind(&SaveRegistrationIdCallback, called, store_registration_id); nit: This indirection doesn't seem very helpful, others do the same thing though. https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:502: EXPECT_EQ(registration_id, result_registration_id); Can you test a case that there is no registration for document_url? https://codereview.chromium.org/535753002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:338: You may want to check that the origin of |document_url| is equal to the origin of |provider_host_->document_url()|. (Please see CanRegisterServiceWorker() and CanUnregisterServiceWorker()) https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:339: GetContext()->GetRegistration( You may want to add "TRACE_EVENT_ASYNC_BEGIN" here (a separate CL is ok) https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:686: thread_id, request_id, info, attrs)); "TRACE_EVENT_ASYNC_END" (a separate CL is ok) https://codereview.chromium.org/535753002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:266: How about testing a CrossOrigin case like others? https://codereview.chromium.org/535753002/diff/160001/content/child/service_w... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/535753002/diff/160001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:337: GetServiceWorkerRegistration(info, true); FYI: I guess you copied these registration handling from OnRegistered(). It's definitely ok, but the original code will be changed in my CL (see ServiceWorkerDispatcher::OnRegistered in [1]). So let's coordinate when we are about to land. [1] https://codereview.chromium.org/477593007/ https://codereview.chromium.org/535753002/diff/160001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:400: const base::string16& message) { Can you add TRACE_EVENTs like OnRegistrationError? (a separate CL is ok) https://codereview.chromium.org/535753002/diff/160001/content/common/service_... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/535753002/diff/160001/content/common/service_... content/common/service_worker/service_worker_messages.h:193: // Response to ServiceWorkerMsg_RegisterServiceWorker. (Although not related to this CL), can you fix this comment, too? s/ServiceWorkerMsg/ServiceWorkerHostMsg/ https://codereview.chromium.org/535753002/diff/160001/content/common/service_... content/common/service_worker/service_worker_messages.h:200: // Response to ServiceWorkerMsg_UnregisterServiceWorker. ditto.
Thanks for review! https://codereview.chromium.org/535753002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:68: return base::Bind(&SaveRegistrationIdCallback, called, store_registration_id); On 2014/09/10 04:07:22, nhiroki wrote: > nit: This indirection doesn't seem very helpful, others do the same thing > though. Agreed - inlined it. https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_context_unittest.cc:502: EXPECT_EQ(registration_id, result_registration_id); On 2014/09/10 04:07:22, nhiroki wrote: > Can you test a case that there is no registration for document_url? Done. https://codereview.chromium.org/535753002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:338: On 2014/09/10 04:07:22, nhiroki wrote: > You may want to check that the origin of |document_url| is equal to the origin > of |provider_host_->document_url()|. > > (Please see CanRegisterServiceWorker() and CanUnregisterServiceWorker()) Done. https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:339: GetContext()->GetRegistration( On 2014/09/10 04:07:22, nhiroki wrote: > You may want to add "TRACE_EVENT_ASYNC_BEGIN" here (a separate CL is ok) Done. https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:686: thread_id, request_id, info, attrs)); On 2014/09/10 04:07:22, nhiroki wrote: > "TRACE_EVENT_ASYNC_END" (a separate CL is ok) Done. https://codereview.chromium.org/535753002/diff/160001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/535753002/diff/160001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:266: On 2014/09/10 04:07:22, nhiroki wrote: > How about testing a CrossOrigin case like others? Done. https://codereview.chromium.org/535753002/diff/160001/content/child/service_w... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/535753002/diff/160001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:337: GetServiceWorkerRegistration(info, true); On 2014/09/10 04:07:22, nhiroki wrote: > FYI: I guess you copied these registration handling from OnRegistered(). It's > definitely ok, but the original code will be changed in my CL (see > ServiceWorkerDispatcher::OnRegistered in [1]). > > So let's coordinate when we are about to land. > > [1] https://codereview.chromium.org/477593007/ Acknowledged. https://codereview.chromium.org/535753002/diff/160001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:400: const base::string16& message) { On 2014/09/10 04:07:22, nhiroki wrote: > Can you add TRACE_EVENTs like OnRegistrationError? (a separate CL is ok) Done. https://codereview.chromium.org/535753002/diff/160001/content/common/service_... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/535753002/diff/160001/content/common/service_... content/common/service_worker/service_worker_messages.h:193: // Response to ServiceWorkerMsg_RegisterServiceWorker. On 2014/09/10 04:07:22, nhiroki wrote: > (Although not related to this CL), can you fix this comment, too? > > s/ServiceWorkerMsg/ServiceWorkerHostMsg/ Done. https://codereview.chromium.org/535753002/diff/160001/content/common/service_... content/common/service_worker/service_worker_messages.h:200: // Response to ServiceWorkerMsg_UnregisterServiceWorker. On 2014/09/10 04:07:22, nhiroki wrote: > ditto. Done.
michaeln@chromium.org changed reviewers: + michaeln@chromium.org
drive-by comments https://codereview.chromium.org/535753002/diff/180001/content/browser/service... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/535753002/diff/180001/content/browser/service... content/browser/service_worker/service_worker_context_core.cc:229: storage()->FindRegistrationForDocument( Not sure we need this method, seems like the caller could use storage()->FindRegistrationForDocument directly. https://codereview.chromium.org/535753002/diff/180001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/535753002/diff/180001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:686: int64 registration_id) { This method is relying on a 'live' registration object with this id actually being there, a const scoped_refptr<T>& return value would ensure that (like FindRegForDocument provides). (i see RegistrationComplete() has the same kind of hidden dependency/requirement, but that doesn't make it right)
michaeln@chromium.org changed reviewers: - michaeln@chromium.org
Thank you for the useful comments! https://codereview.chromium.org/535753002/diff/180001/content/browser/service... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/535753002/diff/180001/content/browser/service... content/browser/service_worker/service_worker_context_core.cc:229: storage()->FindRegistrationForDocument( On 2014/09/10 23:52:24, michaeln wrote: > Not sure we need this method, seems like the caller could use > storage()->FindRegistrationForDocument directly. I see, removed. One good thing of having this method was ease of testing, but this method was essentially just single FindRegistrationForDocument() call, so ServiceWorkerStorageTest already have enough test coverage. https://codereview.chromium.org/535753002/diff/180001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/535753002/diff/180001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:686: int64 registration_id) { On 2014/09/10 23:52:24, michaeln wrote: > This method is relying on a 'live' registration object with this id actually > being there, a const scoped_refptr<T>& return value would ensure that (like > FindRegForDocument provides). > > (i see RegistrationComplete() has the same kind of hidden > dependency/requirement, but that doesn't make it right) Done (as a consequence of using FindRegistrationForDocument directly).
https://codereview.chromium.org/535753002/diff/220001/content/child/service_w... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/535753002/diff/220001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:396: FindServiceWorkerRegistration(info, true); nhiroki@: Since your patch has landed, I've rebased this patch and updated registration handling here (well, just copied from OnRegistered again). Please take another look.
https://codereview.chromium.org/535753002/diff/220001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/535753002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:354: "Document URL", document_url.spec()); The corresponding ASYNC_END couldn't be called due to an early return (for example, when the storage is disabled at line 357). How about moving this after line 362? (If fixing TRACE_EVENT issues seems to need more time, you can omit them from this CL and revisit later.) https://codereview.chromium.org/535753002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:465: DCHECK(registration); This assumption that GetLiveRegistration() always returns a valid registration looks a bit strange. How about moving this callsite of GetLiveRegistration() to the caller and making this function receive |registration| instead? https://codereview.chromium.org/535753002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:714: "Registration ID", registration_id); ditto: moving this before line 695. https://codereview.chromium.org/535753002/diff/220001/content/child/service_w... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/535753002/diff/220001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:396: FindServiceWorkerRegistration(info, true); On 2014/09/11 04:23:52, Kunihiko Sakamoto wrote: > nhiroki@: Since your patch has landed, I've rebased this patch and updated > registration handling here (well, just copied from OnRegistered again). Please > take another look. If a registration object hasn't been created, FindServiceWorkerRegistration could return NULL even if |info.handle_id| is valid. In that case, you need to create a new registration impl here. How about doing like this? // ========== START ========== if (info.handle_id == kInvalid) { // Reg is not found. callback->onSuccess(NULL); pending_get_reg_callbacks_.Removed(request_id); return; } RegImpl* reg = FindReg(info, true); if (!reg) { reg = CreateReg(info, true); reg->SetInstalling(...); reg->SetWaiting(...); reg->SetActive(...); } else { Adopt(attrs.installing); Adopt(attrs.waiting); Adopt(attrs.active); } callbacks->onSuccess(reg); pending_get_reg_callbacks_.Removed(request_id); // ========== END ========== https://codereview.chromium.org/535753002/diff/220001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:411: request_id); This is also likely to be skipped due to an early return. https://codereview.chromium.org/535753002/diff/220001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:485: request_id); ditto.
https://codereview.chromium.org/535753002/diff/220001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/535753002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:354: "Document URL", document_url.spec()); On 2014/09/11 07:23:49, nhiroki wrote: > The corresponding ASYNC_END couldn't be called due to an early return (for > example, when the storage is disabled at line 357). How about moving this after > line 362? > > (If fixing TRACE_EVENT issues seems to need more time, you can omit them from > this CL and revisit later.) Good catch, thanks! Fixed. https://codereview.chromium.org/535753002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:465: DCHECK(registration); On 2014/09/11 07:23:49, nhiroki wrote: > This assumption that GetLiveRegistration() always returns a valid registration > looks a bit strange. How about moving this callsite of GetLiveRegistration() to > the caller and making this function receive |registration| instead? Done. It simplified GetRegistrationComplete a bit. https://codereview.chromium.org/535753002/diff/220001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:714: "Registration ID", registration_id); On 2014/09/11 07:23:49, nhiroki wrote: > ditto: moving this before line 695. Done. https://codereview.chromium.org/535753002/diff/220001/content/child/service_w... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/535753002/diff/220001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:396: FindServiceWorkerRegistration(info, true); On 2014/09/11 07:23:49, nhiroki wrote: > On 2014/09/11 04:23:52, Kunihiko Sakamoto wrote: > > nhiroki@: Since your patch has landed, I've rebased this patch and updated > > registration handling here (well, just copied from OnRegistered again). Please > > take another look. > > If a registration object hasn't been created, FindServiceWorkerRegistration > could return NULL even if |info.handle_id| is valid. In that case, you need to > create a new registration impl here. How about doing like this? > > // ========== START ========== > > if (info.handle_id == kInvalid) { > // Reg is not found. > callback->onSuccess(NULL); > pending_get_reg_callbacks_.Removed(request_id); > return; > } > > RegImpl* reg = FindReg(info, true); > if (!reg) { > reg = CreateReg(info, true); > reg->SetInstalling(...); > reg->SetWaiting(...); > reg->SetActive(...); > } else { > Adopt(attrs.installing); > Adopt(attrs.waiting); > Adopt(attrs.active); > } > callbacks->onSuccess(reg); > pending_get_reg_callbacks_.Removed(request_id); > > // ========== END ========== Thanks for the explanation. Fixed by introducing a helper function. https://codereview.chromium.org/535753002/diff/220001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:411: request_id); On 2014/09/11 07:23:50, nhiroki wrote: > This is also likely to be skipped due to an early return. Since that is an exceptional case (DCHECK fails), I think we don't have to worry about TRACEs too much. I would like to revisit later if necessary. https://codereview.chromium.org/535753002/diff/220001/content/child/service_w... content/child/service_worker/service_worker_dispatcher.cc:485: request_id); On 2014/09/11 07:23:50, nhiroki wrote: > ditto. see the comment above.
LGTM! https://codereview.chromium.org/535753002/diff/240001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/535753002/diff/240001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:463: nit: there is an empty line.
nhiroki@chromium.org changed reviewers: + nasko@chromium.org
+nasko@ for service_worker_messages.h
https://codereview.chromium.org/535753002/diff/240001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/535753002/diff/240001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:463: On 2014/09/11 10:52:40, nhiroki wrote: > nit: there is an empty line. Removed.
IPC LGTM
The CQ bit was checked by ksakamoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/535753002/260001
Message was sent while issue was closed.
Committed patchset #8 (id:260001) as 4fb1b9a3060117586b5bd2b6358445432c1be2b6
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0d1c4968ac6d14ee8d250df9641b6434eb9aaaf0 Cr-Commit-Position: refs/heads/master@{#294720} |
