Bug 180261 – Implement https://w3c.github.io/ServiceWorker/#clients-claim

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Implement https://w3c.github.io/ServiceWorker/#clients-claim

[youenn fablet]

Created attachment 328118 [details]
Patch

[youenn fablet]

Created attachment 328365 [details]
Patch

[Chris Dumez]

Comment on attachment 328365 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Update setting active service woker within ScriptExecutionContext to not do any notification to the storage process.

typo: woker

> Source/WebCore/workers/service/SWClientConnection.cpp:222
> +        client->setActiveServiceWorker(ServiceWorker::getOrCreate(*client, ServiceWorkerData { newController }), ScriptExecutionContext::ShouldNotifyServiceWorkerConnection::No);

notifyClientsOfControllerChange() is also called from activate() for example. How does the server know the client's new controlling worker if you use ShouldNotifyServiceWorkerConnection::No here?

> Source/WebCore/workers/service/server/SWServer.cpp:371
> +            auto controllingRegistration = m_registrations.get(m_runningOrTerminatingWorkers.get(result.iterator->value)->registrationKey());

The spec says to call "Handle Service Worker Client Unload". Where do you do this?

[youenn fablet]

> > Source/WebCore/workers/service/SWClientConnection.cpp:222
> > +        client->setActiveServiceWorker(ServiceWorker::getOrCreate(*client, ServiceWorkerData { newController }), ScriptExecutionContext::ShouldNotifyServiceWorkerConnection::No);
> 
> notifyClientsOfControllerChange() is also called from activate() for
> example. How does the server know the client's new controlling worker if you
> use ShouldNotifyServiceWorkerConnection::No here?

I see, it is used for that purpose as well.
I think SWServer should set it in StorageProcess without having to wait for WebProcess to set it on its client.
 
> > Source/WebCore/workers/service/server/SWServer.cpp:371
> > +            auto controllingRegistration = m_registrations.get(m_runningOrTerminatingWorkers.get(result.iterator->value)->registrationKey());
> 
> The spec says to call "Handle Service Worker Client Unload". Where do you do
> this?

In the line just after, when removing client from registration.

[Chris Dumez]

(In reply to youenn fablet from comment #4)
> > > Source/WebCore/workers/service/SWClientConnection.cpp:222
> > > +        client->setActiveServiceWorker(ServiceWorker::getOrCreate(*client, ServiceWorkerData { newController }), ScriptExecutionContext::ShouldNotifyServiceWorkerConnection::No);
> > 
> > notifyClientsOfControllerChange() is also called from activate() for
> > example. How does the server know the client's new controlling worker if you
> > use ShouldNotifyServiceWorkerConnection::No here?
> 
> I see, it is used for that purpose as well.
> I think SWServer should set it in StorageProcess without having to wait for
> WebProcess to set it on its client.

Sure, but I do not think it is done currently so the patch would regress things.

>  
> > > Source/WebCore/workers/service/server/SWServer.cpp:371
> > > +            auto controllingRegistration = m_registrations.get(m_runningOrTerminatingWorkers.get(result.iterator->value)->registrationKey());
> > 
> > The spec says to call "Handle Service Worker Client Unload". Where do you do
> > this?
> 
> In the line just after, when removing client from registration.

Actually, client unload should do these too:
If registration’s uninstalling flag is set, invoke Try Clear Registration with registration.
If registration is not null, invoke Try Activate with registration.

[youenn fablet]

Created attachment 328470 [details]
Patch

[Chris Dumez]

Comment on attachment 328470 [details]
Patch

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

r=me

> Source/WebCore/workers/service/server/SWServerWorker.cpp:142
> +    auto* registration = m_server.getRegistration(m_registrationKey);

I think a comment pointing the which part of which algorithm is implemented would be helpful here.

> LayoutTests/http/tests/workers/service/serviceworkerclients-claim-worker.js:2
> +    if (event.request.url.indexOf("pinkelephant") !== -1)

lol

[youenn fablet]

Created attachment 328487 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

> > Source/WebCore/workers/service/server/SWServerWorker.cpp:142
> > +    auto* registration = m_server.getRegistration(m_registrationKey);
> 
> I think a comment pointing the which part of which algorithm is implemented
> would be helpful here.

Done

[WebKit Commit Bot]

Comment on attachment 328487 [details]
Patch for landing

Rejecting attachment 328487 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 328487, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
.webkit.org/git/WebKit
   6a810d9..9772276  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 225530 = 6a810d93379060e6a3c4a9b02a287328d9de8c19
r225531 = 977227696fd292541c136dc33a2194e3c3116f3c
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/5505205

[youenn fablet]

Created attachment 328490 [details]
Rebasing

[WebKit Commit Bot]

Comment on attachment 328490 [details]
Rebasing

Clearing flags on attachment: 328490

Committed r225537: <https://trac.webkit.org/changeset/225537>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35863924>