ServiceWorker: Implement navigate() method in WindowClient (blink side).

ServiceWorker: Implement navigate() method in WindowClient (blink side).

Implement navigate() method in WindowClient (blink side).

Spec:
  http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#client-navigate

Intent to implement:
  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/KETlHSSc878

Chromium side:
  https://codereview.chromium.org/1202453002/

Layout test:
  https://codereview.chromium.org/1211253007/

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

[zino, 2015-06-22 14:57:28]

jinho.bang@samsung.com changed reviewers:
+ mkwst@chromium.org, nhiroki@chromium.org

[zino, 2015-06-22 15:01:44]

Dear reviewers,

I uploaded the patch to implement the navigate() method in
ServiceWorkerWindowClient.

I'm not familiar with service worker part and this is my first patch about it.
(But I'm interested in the part).

So, please review the code and correct me.

Thank you.

[zino, 2015-06-22 16:28:08]

jinho.bang@samsung.com changed reviewers:
+ falken@chromium.org

[nhiroki, 2015-06-23 01:18:13]

Thank you for working on this! Could you make the 3rd CL to add layout tests?

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
File Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right):

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:83: // TODO
Why don't you implement this in this CL?

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:86: // settings
object's global object's service worker, return a promise rejected with a
TypeError.
We should check this in the browser-side.

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:87: // TODO
nit: TODO comment should be formatted as "TODO(name): ..." 
http://www.chromium.org/blink/coding-style#TOC-Comments

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
File Source/modules/serviceworkers/WindowClient.idl (right):

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/WindowClient.idl:25:
[RuntimeEnabled=ServiceWorkerClientAttributes, CallWith=ScriptState]
Promise<WindowClient> navigate(USVString url);
"ServiceWorkerClientAttributes" has already been enabled by default. Could you
add another flag for navigate()?

[zino1, 2015-06-23 02:04:44]

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

[zino, 2015-06-23 03:42:15]

Thank you for review!

I addressed all of comments and I'll upload a layout test for this soon.

Could you please review the chromium side patch together?

Thank you.

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
File Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right):

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:83: // TODO
On 2015/06/23 01:18:13, nhiroki wrote:
> Why don't you implement this in this CL?

Done.

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:86: // settings
object's global object's service worker, return a promise rejected with a
TypeError.
On 2015/06/23 01:18:13, nhiroki wrote:
> We should check this in the browser-side.

I got it. Could you please review chromium side patch as well?

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:87: // TODO
On 2015/06/23 01:18:13, nhiroki wrote:
> nit: TODO comment should be formatted as "TODO(name): ..." 
> http://www.chromium.org/blink/coding-style#TOC-Comments

It's my mistake. I've just removed all the comments :)

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
File Source/modules/serviceworkers/WindowClient.idl (right):

https://codereview.chromium.org/1196193003/diff/1/Source/modules/serviceworke...
Source/modules/serviceworkers/WindowClient.idl:25:
[RuntimeEnabled=ServiceWorkerClientAttributes, CallWith=ScriptState]
Promise<WindowClient> navigate(USVString url);
On 2015/06/23 01:18:13, nhiroki wrote:
> "ServiceWorkerClientAttributes" has already been enabled by default. Could you
> add another flag for navigate()?

Done.

[nhiroki, 2015-06-24 06:58:30]

Looks good overall.

Could you add a link to the 3rd CL and TEST= line in the CL description? (eg.
"TEST=will be added in the 3rd CL")

https://codereview.chromium.org/1196193003/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right):

https://codereview.chromium.org/1196193003/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:79: 
Could you check |url| by "SecurityOrigin::canDisplay" to avoid loading a local
file etc?

https://codereview.chromium.org/1196193003/diff/20001/Source/web/ServiceWorke...
File Source/web/ServiceWorkerGlobalScopeClientImpl.h (right):

https://codereview.chromium.org/1196193003/diff/20001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeClientImpl.h:71: virtual void navigate(const
WebString& clientUUIDi, const WebURL&, WebServiceWorkerClientCallbacks*)
override;
nit: cliendUUIDi -> clientUUID

[nhiroki, 2015-06-24 08:21:22]

On 2015/06/24 06:58:30, nhiroki wrote:
> Could you add a link to the 3rd CL and TEST= line in the CL description? (eg.
> "TEST=will be added in the 3rd CL")

Sorry, adding TEST= line seems not necessary (see
https://codereview.chromium.org/1187623006/#msg20 for details). Please ignore
this comment.

[zino, 2015-06-28 06:53:43]

Thank you for review.

I'm sorry for delay.

I'll upload a new patch set soon.

[zino, 2015-07-07 15:39:43]

@nhiroki

Sorry for delay because of car accident.

Please review again.

I also uploaded layout test for this CL.

https://codereview.chromium.org/1211253007/.

Thank you.

https://codereview.chromium.org/1196193003/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right):

https://codereview.chromium.org/1196193003/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:79: 
On 2015/06/24 06:58:29, nhiroki wrote:
> Could you check |url| by "SecurityOrigin::canDisplay" to avoid loading a local
> file etc?

Done.

https://codereview.chromium.org/1196193003/diff/20001/Source/web/ServiceWorke...
File Source/web/ServiceWorkerGlobalScopeClientImpl.h (right):

https://codereview.chromium.org/1196193003/diff/20001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeClientImpl.h:71: virtual void navigate(const
WebString& clientUUIDi, const WebURL&, WebServiceWorkerClientCallbacks*)
override;
On 2015/06/24 06:58:30, nhiroki wrote:
> nit: cliendUUIDi -> clientUUID

Done.

[nhiroki, 2015-07-10 04:50:59]

> Sorry for delay because of car accident.

Oh..., I'm sorry to hear that. Please take care.

https://codereview.chromium.org/1196193003/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right):

https://codereview.chromium.org/1196193003/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:70: if
(!parsedUrl.isValid() || parsedUrl.protocolIsAbout()) {
FYI: As you might know, the way to handle "about:" protocol could be changed
(https://github.com/slightlyoff/ServiceWorker/issues/699).

I think we can keep this as is until the spec gets stable because this feature
is behind a flag.

https://codereview.chromium.org/1196193003/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:71:
resolver->reject(DOMException::create(SyntaxError, "'" + url + "' is not a valid
URL."));
Could you reject this by TypeError?

From the spec:
"2. If url is failure, return a promise rejected with a TypeError."
"3. If url is about:blank, return a promise rejected with a TypeError."

https://codereview.chromium.org/1196193003/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:78: if
(!context->isWindowInteractionAllowed()) {
Maybe we don't have to check this because this API does not create a window or
focus an existing window:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1196193003/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/WindowClient.idl (right):

https://codereview.chromium.org/1196193003/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/WindowClient.idl:24:
[RuntimeEnabled=ServiceWorkerWindowClientNavigate, CallWith=ScriptState]
Promise<WindowClient> navigate(USVString url);
Could you update
"http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt"?
That fails a test.

[nhiroki, 2015-07-10 06:19:01]

> I also uploaded layout test for this CL.
>
> https://codereview.chromium.org/1211253007/.

Thank you for making tests! Could you add a link to the CL in this and
chromium-side's CL description?

[zino, 2015-07-13 07:13:21]

Thank you for review.

I uploaded a new patch.

https://codereview.chromium.org/1196193003/diff/40001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp (right):

https://codereview.chromium.org/1196193003/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:70: if
(!parsedUrl.isValid() || parsedUrl.protocolIsAbout()) {
On 2015/07/10 04:50:59, nhiroki wrote:
> FYI: As you might know, the way to handle "about:" protocol could be changed
> (https://github.com/slightlyoff/ServiceWorker/issues/699).
> 
> I think we can keep this as is until the spec gets stable because this feature
> is behind a flag.

Thank you for the information.

https://codereview.chromium.org/1196193003/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:71:
resolver->reject(DOMException::create(SyntaxError, "'" + url + "' is not a valid
URL."));
On 2015/07/10 04:50:59, nhiroki wrote:
> Could you reject this by TypeError?
> 
> From the spec:
> "2. If url is failure, return a promise rejected with a TypeError."
> "3. If url is about:blank, return a promise rejected with a TypeError."

Done.

https://codereview.chromium.org/1196193003/diff/40001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:78: if
(!context->isWindowInteractionAllowed()) {
On 2015/07/10 04:50:59, nhiroki wrote:
> Maybe we don't have to check this because this API does not create a window or
> focus an existing window:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Done.

[nhiroki, 2015-07-13 07:22:32]

LGTM!

[zino, 2015-07-14 03:08:28]

jinho.bang@samsung.com changed reviewers:
+ tkent@chromium.org

[zino, 2015-07-14 03:08:50]

+ tkent@ for Source web/* and  Source/platform/*

Could you please review this CL?

[tkent, 2015-07-14 04:13:55]

The CQ bit was checked by tkent@chromium.org

[tkent, 2015-07-14 04:13:56]

lgtm

[commit-bot: I haz the power, 2015-07-14 04:14:05]

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

[commit-bot: I haz the power, 2015-07-14 05:32:03]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198838