-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve.ready and Activate #1277
Conversation
Before this change, .ready method was waiting for the change of the state of the regisration's active worker without being synchronized with the steps that set the corresponding ServiceWorkerRegistration object's active ServiceWorker object's state. This changes both .ready and Activate such that .ready tries to resolve the ready promise, if pending, and just returns. And Activate resolves pending ready promises after updating active ServiceWorker object's state. Fixes #1011
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. I think our WPT tests already assumed this.
docs/index.bs
Outdated
1. Return <a>context object</a>'s [=ServiceWorkerContainer/ready promise=]. | ||
1. Let |readyPromise| be the [=context object=]'s [=ServiceWorkerContainer/ready promise=]. | ||
1. If |readyPromise| is pending, then: | ||
1. Let |registration| be the result of running [=Match Service Worker Registration=] with the [=context object=]'s [=ServiceWorkerContainer/service worker client=]'s [=creation URL=]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be running this on the main thread since it's a disk read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I made it run in parallel and queue a task to check out the ready promise.
docs/index.bs
Outdated
1. For each [=/service worker client=] |client| whose <a>creation URL</a> <a lt="Match Service Worker Registration">matches</a> |registration|'s [=service worker registration/scope url=]: | ||
1. Let |matchedClients| be a [=list=] of [=/service worker clients=] whose <a>creation URL</a> <a lt="Match Service Worker Registration">matches</a> |registration|'s [=service worker registration/scope url=]. | ||
1. [=list/For each=] |client| of |matchedClients|: | ||
1. Let |readyPromise| be |client|'s [=environment settings object/global object=]'s {{ServiceWorkerContainer}} object's [=ServiceWorkerContainer/ready promise=]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to queue a task on each client to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the subsequent step, I queued a task to resolve the ready promise. But I think your point is we should queue a task to get the ready promise itself for each client in this step, right? I made a change to address that point. Please take a look.
Before this change, .ready method was waiting for the change of the
state of the regisration's active worker without being synchronized with
the steps that set the corresponding ServiceWorkerRegistration object's
active ServiceWorker object's state.
This changes both .ready and Activate such that .ready tries to resolve
the ready promise, if pending, and just returns. And Activate resolves
pending ready promises after updating active ServiceWorker object's
state.
Fixes #1011
Preview | Diff