[Webkit-unassigned] [Bug 179318] Implement "UpdateWorkerState", "fire updatefound event", and actually rely on the real update/install algorithms.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=179318

--- Comment #4 from Brady Eidson <beidson at apple.com> ---
(In reply to Chris Dumez from comment #3)
> Comment on attachment 326120 [details]
> WIP, trying to reconcile that microtask without causing test failures
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326120&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:136
> > +    // FIXME: Here is the point in time where we need to create a ServiceWorkerRegistration object
> 
> This seems premature, no? What if the registration fails?
> In the spec, it is constructed when we resolve the promise
> (https://w3c.github.io/ServiceWorker/#resolve-job-promise), early in the
> Install steps.

That's not true.

It's created at step 6. of the register algorithm by invoking Set Registration.

https://w3c.github.io/ServiceWorker/#register-algorithm

> 
> > Source/WebCore/workers/service/server/SWClientConnection.cpp:200
> > +            registration->dispatchEvent(Event::create(eventNames().updatefoundEvent, false, false));
> 
> You now fire this event twice?

Why do you say that? (If this patch happens to fire it twice, that's because I'm trying to refactor out one of them.

> 
> > Source/WebCore/workers/service/server/SWClientConnection.cpp:219
> > +    auto* registrations = m_registrations.get(key);
> 
> I believe the registrations are on the container nowadays.

After I started this Friday, yes - As you can see from the patch not applying it's simply out of date.

> 
> > Source/WebCore/workers/service/server/SWClientConnection.cpp:229
> > +        registration->dispatchEvent(Event::create(eventNames().updatefoundEvent, false, false));
> 
> Why are we moving more of the event firing logic into SWClientConnection?
> This does not seem like the right place for this.

SWClientConnection isn't firing the event. It's telling the registration to.

> 
> > Source/WebCore/workers/service/server/SWServer.cpp:262
> > +    registration->fireUpdateFoundEvent();
> 
> This is technically part of the install steps, maybe this should be moved to
> an install function for clarity?

Maybe. Once I rebase to trunk will take a look.

> 
> > Source/WebCore/workers/service/server/SWServerRegistration.cpp:98
> > +    for (auto& connectionIdentifierWithClients : m_clientRegistrationsByConnection.keys()) {
> 
> In my experience, the client registration is not added to
> m_clientRegistrationsByConnection yet by the time we reach this code, in the
> case of a registration, so it did not end up firing the event at the new
> registration object.

Right, and that's what I'm fixing above.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171106/ffe176f4/attachment-0001.html>