ServiceWorker: Support SWRegistration.unregister() in SWGlobalScope [2/2]

ServiceWorker: Support SWRegistration.unregister() in SWGlobalScope [2/2]

SWRegistration.unregister() depends on WebSWProvider, but it's not available
in SWGlobalScope because the provider isn't supplied on the worker startup
sequence. This series of CLs make it available and support unregister() in
the service worker context.

This CL defines createServiceWorkerProvider() interface and actually provides
ServiceWorkerContainerClient object.

[1] Chromium: https://codereview.chromium.org/893363003/
[2] Blink: THIS PATCH


BUG=452910
TEST=run-webkit-tests http/tests/serviceworker/ServiceWorkerGlobalScope

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189616

[kinuko, 2015-02-04 15:39:29]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2015-02-04 15:39:30]

https://codereview.chromium.org/900793002/diff/20001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/900793002/diff/20001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:58: // are called on the worker
thread.
Please update this comment too.

[nhiroki, 2015-02-05 08:32:05]

Patchset #3 (id:40001) has been deleted

[nhiroki, 2015-02-05 08:36:38]

Can you take another look? Thanks!

https://codereview.chromium.org/900793002/diff/20001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/900793002/diff/20001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:58: // are called on the worker
thread.
On 2015/02/04 15:39:30, kinuko wrote:
> Please update this comment too.

Done.

[kinuko, 2015-02-05 09:09:29]

lgtm

I'm not terribly good at reviewing registration tests, you may also want falken@
or someone to have a look.

https://codereview.chromium.org/900793002/diff/60001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/900793002/diff/60001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:154: virtual
WebServiceWorkerProvider* createServiceWorkerProvider() { return 0; }
nit: I think we can now return nullptr (back then we weren't able to do so)

[nhiroki, 2015-02-05 09:12:17]

New patchsets have been uploaded after l-g-t-m from kinuko@chromium.org

[nhiroki, 2015-02-05 09:12:42]

Updated. Thank you!

https://codereview.chromium.org/900793002/diff/60001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/900793002/diff/60001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:154: virtual
WebServiceWorkerProvider* createServiceWorkerProvider() { return 0; }
On 2015/02/05 09:09:29, kinuko wrote:
> nit: I think we can now return nullptr (back then we weren't able to do so)

Done.

[nhiroki, 2015-02-05 09:14:10]

nhiroki@chromium.org changed reviewers:
+ falken@chromium.org, mkwst@chromium.org

[nhiroki, 2015-02-05 09:15:06]

Hi, can you review this?

+falken for LayoutTests/
+mkwst for Source/web and public/web

Thanks.

[falken, 2015-02-05 12:07:57]

lgtm with comments

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/unregister-controlling-worker-iframe.html
(right):

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/unregister-controlling-worker-iframe.html:3:
function fetch_url(url) {
Hopefully we can remove this.

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/unregister-worker.js
(right):

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/unregister-worker.js:28:
});
I think we shouldn't need this fetch event handler for these tests.

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/unregister.html
(right):

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/unregister.html:91:
'Service worker should respond to fetch from iframe under control');
Our old tests did this because we didn't yet have .controller (or didn't really
"trust" it yet). We don't really need to do an XHR to see if the SW really
controls page anymore, just check .controller.

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/unregister.html:93:
// Wait for the completion of unregistrer() on the worker.
nit: unregister()

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/unregister.html:113:
return frame1.contentWindow.fetch_url(text_url);
just check controller here and below

https://codereview.chromium.org/900793002/diff/80001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/900793002/diff/80001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:58: // workerContextFailedToStart(),
are called on the worker thread.
My comment on the chromium patch also applies here.

[Mike West, 2015-02-05 12:44:37]

*/web LGTM.

[nhiroki, 2015-02-06 02:04:06]

New patchsets have been uploaded after l-g-t-m from
falken@chromium.org,mkwst@chromium.org

[nhiroki, 2015-02-06 03:35:26]

Patchset #6 (id:120001) has been deleted

[nhiroki, 2015-02-06 03:42:50]

Patchset #6 (id:140001) has been deleted

[nhiroki, 2015-02-06 03:49:30]

Thank you!

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/unregister-controlling-worker-iframe.html
(right):

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/unregister-controlling-worker-iframe.html:3:
function fetch_url(url) {
On 2015/02/05 12:07:57, falken wrote:
> Hopefully we can remove this.

Removed this file, yay!

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/unregister-worker.js
(right):

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/unregister-worker.js:28:
});
On 2015/02/05 12:07:57, falken wrote:
> I think we shouldn't need this fetch event handler for these tests.

Removed!

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/unregister.html
(right):

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/unregister.html:91:
'Service worker should respond to fetch from iframe under control');
On 2015/02/05 12:07:57, falken wrote:
> Our old tests did this because we didn't yet have .controller (or didn't
really
> "trust" it yet). We don't really need to do an XHR to see if the SW really
> controls page anymore, just check .controller.

That's a neat solution! Updated here and elsewhere in the way.

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/unregister.html:93:
// Wait for the completion of unregistrer() on the worker.
On 2015/02/05 12:07:57, falken wrote:
> nit: unregister()

Done.

https://codereview.chromium.org/900793002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/unregister.html:113:
return frame1.contentWindow.fetch_url(text_url);
On 2015/02/05 12:07:57, falken wrote:
> just check controller here and below

Done.

https://codereview.chromium.org/900793002/diff/80001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/900793002/diff/80001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:58: // workerContextFailedToStart(),
are called on the worker thread.
On 2015/02/05 12:07:57, falken wrote:
> My comment on the chromium patch also applies here.

Done.

[nhiroki, 2015-02-06 04:58:05]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-02-06 04:58:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/900793002/160001

[commit-bot: I haz the power, 2015-02-06 06:24:08]

Committed patchset #6 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189616