ServiceWorker: Implement navigator.serviceWorker.getRegistration [3/3]

ServiceWorker: Implement navigator.serviceWorker.getRegistration [3/3]

This completes the implementation of ServiceWorkerContainer#getRegistration().

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#navigator-service-worker-getRegistration

[1/3] https://codereview.chromium.org/553983010/
[2/3] https://codereview.chromium.org/535753002/

BUG=404951
TEST=LayoutTests/http/tests/serviceworker/getregistration.html,webkit_unit_tests

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

[Kunihiko Sakamoto, 2014-09-04 08:23:52]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2014-09-04 09:33:45]

Patchset #1 (id:20001) has been deleted

[Kunihiko Sakamoto, 2014-09-09 07:29:35]

ksakamoto@chromium.org changed reviewers:
+ horo@chromium.org, nhiroki@chromium.org

[nhiroki, 2014-09-10 07:45:04]

https://codereview.chromium.org/540823003/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/getregistration.html (right):

https://codereview.chromium.org/540823003/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:33: 
How about testing following cases?

- getRegistration() without a doc url argument
- getRegistration() with a cross origin url
- register() -> unregister() -> getRegistration() (should return 'undefined')

https://codereview.chromium.org/540823003/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/540823003/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:204: KURL completedURL
= executionContext->completeURL(documentURL);
(Just to check my understanding) When the document URL is not specified,
|completedURL| represents the caller's document URL. That is,
navigator.serviceWorker.getRegistration() without an argument returns a
registration object whose scope matches the caller's document URL. Okay, this
would be conformed with the spec.

https://codereview.chromium.org/540823003/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp (right):

https://codereview.chromium.org/540823003/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp:274: 
Can you test the CrossOriginDocument case?

[Kunihiko Sakamoto, 2014-09-10 09:34:51]

https://codereview.chromium.org/540823003/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/getregistration.html (right):

https://codereview.chromium.org/540823003/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:33: 
On 2014/09/10 07:45:04, nhiroki wrote:
> How about testing following cases?
> 
> - getRegistration() without a doc url argument
> - getRegistration() with a cross origin url
> - register() -> unregister() -> getRegistration() (should return 'undefined')

Thanks for the test cases suggestion! Done.

https://codereview.chromium.org/540823003/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/540823003/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:204: KURL completedURL
= executionContext->completeURL(documentURL);
On 2014/09/10 07:45:04, nhiroki wrote:
> (Just to check my understanding) When the document URL is not specified,
> |completedURL| represents the caller's document URL. That is,
> navigator.serviceWorker.getRegistration() without an argument returns a
> registration object whose scope matches the caller's document URL. Okay, this
> would be conformed with the spec.

Yes. I've added a test case to verify this in ServiceWorkerContainerTest.cpp.

https://codereview.chromium.org/540823003/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp (right):

https://codereview.chromium.org/540823003/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerContainerTest.cpp:274: 
On 2014/09/10 07:45:04, nhiroki wrote:
> Can you test the CrossOriginDocument case?

Done.

[nhiroki, 2014-09-11 11:30:34]

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/getregistration.html (right):

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:20:
navigator.serviceWorker.register('resources/empty-worker.js',
Can you use service_worker_unregister_and_register()? If the previous test run
failed, the registration for |scope| could be still remaining.

("multiple-register.html" is a good example)

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:29: t.done();
To unregister the registration before finishing the test, can you call
service_worker_unregister_and_done() instead?

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:38:
assert_unreached('getRegistration with an out of origin URL should fail');
nit: +2-space indents
nit: please wrap this at 80-column.

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:50: {scope:
location.href})
Why do you use |location.href|?

We'd prefer to avoid that the scope includes the test script itself because it
might cause an inexplicable bug in the next run.

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:58: t.done();
ditto (service_worker_unregister_and_done)

[Kunihiko Sakamoto, 2014-09-12 01:32:13]

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/getregistration.html (right):

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:20:
navigator.serviceWorker.register('resources/empty-worker.js',
On 2014/09/11 11:30:34, nhiroki wrote:
> Can you use service_worker_unregister_and_register()? If the previous test run
> failed, the registration for |scope| could be still remaining.
> 
> ("multiple-register.html" is a good example)

Done.

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:29: t.done();
On 2014/09/11 11:30:34, nhiroki wrote:
> To unregister the registration before finishing the test, can you call
> service_worker_unregister_and_done() instead?

Done.

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:38:
assert_unreached('getRegistration with an out of origin URL should fail');
On 2014/09/11 11:30:34, nhiroki wrote:
> nit: +2-space indents
> nit: please wrap this at 80-column.

Done.

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:50: {scope:
location.href})
On 2014/09/11 11:30:34, nhiroki wrote:
> Why do you use |location.href|?
> 
> We'd prefer to avoid that the scope includes the test script itself because it
> might cause an inexplicable bug in the next run.

Wanted to test that getRegistration() gets a registration for the document URL.

Since this case is covered by ServiceWorkerContainerTest, we might not need it.
Removed.

[nhiroki, 2014-09-12 02:23:26]

lgtm

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/getregistration.html (right):

https://codereview.chromium.org/540823003/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/getregistration.html:50: {scope:
location.href})
On 2014/09/12 01:32:13, Kunihiko Sakamoto wrote:
> On 2014/09/11 11:30:34, nhiroki wrote:
> > Why do you use |location.href|?
> > 
> > We'd prefer to avoid that the scope includes the test script itself because
it
> > might cause an inexplicable bug in the next run.
> 
> Wanted to test that getRegistration() gets a registration for the document
URL.
> 
> Since this case is covered by ServiceWorkerContainerTest, we might not need
it.
> Removed.

I see. Probably you can make an iframe for |scope| (fyi: with_iframe() and
unload_iframe() are helper functions to manipulate iframe) and call
|getRegistration()| in the iframe.

[nhiroki, 2014-09-12 07:07:54]

You may want to add a test for a long url case in
http/tests/serviceworker/chromium/url-limits.html.

[Kunihiko Sakamoto, 2014-09-12 07:21:05]

Patchset #7 (id:160001) has been deleted

[Kunihiko Sakamoto, 2014-09-12 07:22:53]

Patchset #7 (id:180001) has been deleted

[Kunihiko Sakamoto, 2014-09-12 07:23:18]

On 2014/09/12 07:07:54, nhiroki wrote:
> You may want to add a test for a long url case in
> http/tests/serviceworker/chromium/url-limits.html.

Done.

[Kunihiko Sakamoto, 2014-09-13 05:10:49]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2014-09-13 05:11:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/540823003/200001

[commit-bot: I haz the power, 2014-09-13 06:13:15]

Committed patchset #7 (id:200001) as 181952