[webkit-reviews] review granted: [Bug 178985] [Service Workers] Fire updatefound event after resolving the registration promise : [Attachment 325376] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 30 14:33:29 PDT 2017


youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 178985: [Service Workers] Fire updatefound event after resolving the
registration promise
https://bugs.webkit.org/show_bug.cgi?id=178985

Attachment 325376: Patch

https://bugs.webkit.org/attachment.cgi?id=325376&action=review




--- Comment #16 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 325376
  --> https://bugs.webkit.org/attachment.cgi?id=325376
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325376&action=review

> Source/WebCore/workers/service/ServiceWorker.cpp:41
> +ServiceWorker::ServiceWorker(ScriptExecutionContext& context, uint64_t
serviceWorkerIdentifier, const URL& scriptURL)

URL&& probably

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:77
>  ServiceWorker* ServiceWorkerContainer::controller() const

CallWith=ScriptExecutionContext might be better.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:216
> +	   context->setActiveServiceWorker(ServiceWorker::create(*context,
data.identifier, data.scriptURL));

We should probably not create a ServiceWorker here.
Probably we should get it from the selected ServiceWorkerRegistration that we
create below.
I guess the above FIXME might cover this though.
Or maybe some refactoring would get us closer to the end target?

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:225
> +    // a few events to make it look like we are installing and activing the
service worker.

Let's hope we can remove that code and FIXME soon.
But end result in terms of testing is great!

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:254
> +	   context->setActiveServiceWorker(nullptr);

I would prefer clearActiveServiceWorker and setActiveServiceWorker take a
Ref<>. Both might be inlined anyway.

> Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:54
>      return nullptr;

Do we not do it in the other way usually by returning nullptr early?

> Source/WebCore/workers/service/ServiceWorkerRegistration.h:74
> +    RefPtr<ServiceWorker> m_serviceWorker;

Right now I guess it can it be a Ref<> but in the future it may not. Would use
a Ref<> though if possible now.


More information about the webkit-reviews mailing list