Add blink-side binding code and tests for ServiceWorker -> Document postMessage (3/3)

Add blink-side binding code and tests for ServiceWorker -> Document postMessage

This is the last patch for postMessage plumbing:
1/3: https://codereview.chromium.org/263143004/ (blink)
2/3: https://codereview.chromium.org/246023007/ (chromium)
3/3: THIS PATCH

BUG=366063
TEST=http/tests/serviceworker/postmessage-to-client.html
R=jsbell@chromium.org, tkent@chromium.org

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

[kinuko, 2014-05-06 07:51:02]

This is to be the last patch for postMessage, to add tests and remaining binding
code. PTL

[Inactive, 2014-05-06 12:26:13]

https://codereview.chromium.org/264233003/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/Client.idl (right):

https://codereview.chromium.org/264233003/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Client.idl:10: [Custom, RaisesException] void
postMessage(SerializedScriptValue message, optional MessagePort[] messagePorts);
Looks like the spec says:
void postMessage(any message, DOMString targetOrigin, optional
sequence<Transferable> transfer);

Aren't you missing the second mandatory argument? Or is my spec version
(https://slightlyoff.github.io/ServiceWorker/spec/service_worker/) outdated?

[kinuko, 2014-05-06 13:57:36]

https://codereview.chromium.org/264233003/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/Client.idl (right):

https://codereview.chromium.org/264233003/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Client.idl:10: [Custom, RaisesException] void
postMessage(SerializedScriptValue message, optional MessagePort[] messagePorts);
On 2014/05/06 12:26:13, Chris Dumez wrote:
> Looks like the spec says:
> void postMessage(any message, DOMString targetOrigin, optional
> sequence<Transferable> transfer);
> 
> Aren't you missing the second mandatory argument? Or is my spec version
> (https://slightlyoff.github.io/ServiceWorker/spec/service_worker/) outdated?

There's an ongoing discussion about if we really want to have targetOrigin
argument [1], and we suspect we do.  I added a comment and link to the issue so
that we can follow-up once the issue's closed.
[1] https://github.com/slightlyoff/ServiceWorker/issues/190

[marja, 2014-05-06 16:29:15]

https://codereview.chromium.org/264233003/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/postmessage-to-client.html (right):

https://codereview.chromium.org/264233003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-to-client.html:35:
w.navigator.serviceWorker.current.postMessage('ping');
Is it also possible to post a message port?

[jsbell, 2014-05-06 17:43:50]

lgtm - but I agree with marja that we should test posting another port through,
which will inflate the test.

[jsbell, 2014-05-06 22:15:24]

https://codereview.chromium.org/264233003/diff/40001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8ClientCustom.cpp (right):

https://codereview.chromium.org/264233003/diff/40001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8ClientCustom.cpp:35: client->postMessage(context,
message.release(), &ports, exceptionState);
Actually... what's this patch based on besides 263143004 (1/3) and 265943003
(.current bindings)? I'm not finding Client::postMessage defined anywhere.

Did a file go missing?

[kinuko, 2014-05-07 09:12:15]

Added missing code and test for MessagePort.  The test worked, but it's crashing
at shutdown time in MessagePort::close() (in
ContextLifecycleNotifier::notifyStoppingActiveDOMObjects()) so probably I'm
doing something wrong.  Investigating..

https://codereview.chromium.org/264233003/diff/40001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8ClientCustom.cpp (right):

https://codereview.chromium.org/264233003/diff/40001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8ClientCustom.cpp:35: client->postMessage(context,
message.release(), &ports, exceptionState);
On 2014/05/06 22:15:24, jsbell wrote:
> Actually... what's this patch based on besides 263143004 (1/3) and 265943003
> (.current bindings)? I'm not finding Client::postMessage defined anywhere.
> 
> Did a file go missing?

Rebase failure.. added missing code.

[kinuko, 2014-05-07 19:26:09]

On 2014/05/07 09:12:15, kinuko wrote:
> Added missing code and test for MessagePort.  The test worked, but it's
crashing
> at shutdown time in MessagePort::close() (in
> ContextLifecycleNotifier::notifyStoppingActiveDOMObjects()) so probably I'm
> doing something wrong.  Investigating..

Fixed the issue (in the other patch), and added test expectations for the new
MessagePort test too.
PTAL

[jsbell, 2014-05-07 23:46:32]

lgtm

https://codereview.chromium.org/264233003/diff/80001/Source/web/ServiceWorker...
File Source/web/ServiceWorkerGlobalScopeClientImpl.cpp (right):

https://codereview.chromium.org/264233003/diff/80001/Source/web/ServiceWorker...
Source/web/ServiceWorkerGlobalScopeClientImpl.cpp:94: blink::WebString
messageString = message->toWireString();
Just curious - is there a reason to defer the toWireString and
toWebMessagePortChannelArray from Client.cpp to here?

It's done directly in ServiceWorker::postMessage() so I'm wondering if the
difference is intentional and we should change it there too.

[haraken, 2014-05-08 00:35:42]

bindings/ LGTM

[kinuko, 2014-05-08 02:28:22]

https://codereview.chromium.org/264233003/diff/80001/Source/web/ServiceWorker...
File Source/web/ServiceWorkerGlobalScopeClientImpl.cpp (right):

https://codereview.chromium.org/264233003/diff/80001/Source/web/ServiceWorker...
Source/web/ServiceWorkerGlobalScopeClientImpl.cpp:94: blink::WebString
messageString = message->toWireString();
On 2014/05/07 23:46:33, jsbell wrote:
> Just curious - is there a reason to defer the toWireString and
> toWebMessagePortChannelArray from Client.cpp to here?
> 
> It's done directly in ServiceWorker::postMessage() so I'm wondering if the
> difference is intentional and we should change it there too.

No strong reason, did so just to avoid some header churn. I moved them to
Client.cpp as it'd be more readable & to make them look more alike.

[kinuko, 2014-05-08 02:29:12]

Kent-san, could you do owner review for Source/web changes?

[tkent, 2014-05-08 02:40:01]

lgtm

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

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-09 02:31:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/264233003/100001

[commit-bot: I haz the power, 2014-05-09 02:31:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 02:31:55]

Failed to apply patch for
LayoutTests/http/tests/serviceworker/postmessage-to-client-expected.txt:
While running svn copy
LayoutTests/http/tests/serviceworker/current-on-load-expected.txt
LayoutTests/http/tests/serviceworker/postmessage-to-client-expected.txt
--config-dir /b/infra_internal/commit_queue/subversion_config --non-interactive
--username commit-bot@chromium.org;
  svn: Path 'LayoutTests/http/tests/serviceworker/current-on-load-expected.txt'
does not exist

Patch:   NR 
LayoutTests/http/tests/serviceworker/current-on-load-expected.txt->LayoutTests/http/tests/serviceworker/postmessage-to-client-expected.txt
Index: LayoutTests/http/tests/serviceworker/postmessage-to-client-expected.txt
diff --git a/LayoutTests/http/tests/serviceworker/current-on-load-expected.txt
b/LayoutTests/http/tests/serviceworker/postmessage-to-client-expected.txt
similarity index 53%
copy from LayoutTests/http/tests/serviceworker/current-on-load-expected.txt
copy to LayoutTests/http/tests/serviceworker/postmessage-to-client-expected.txt
index
961b30caf4dd5dcffb214161cbc793e29819e692..12abf632640d181a8d7243603288a817a327c909
100644
--- a/LayoutTests/http/tests/serviceworker/current-on-load-expected.txt
+++ b/LayoutTests/http/tests/serviceworker/postmessage-to-client-expected.txt
@@ -1,3 +1,3 @@
 
-FAIL current is set for a controlled document assert_unreached: Unregister
should not fail: NotSupportedError Reached unreachable code
+FAIL postMessage from ServiceWorker to Client assert_unreached: Unregister
should not fail: NotSupportedError Reached unreachable code

[kinuko, 2014-05-09 04:32:44]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-09 04:33:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/264233003/100001

[kinuko, 2014-05-09 05:57:57]

Ok I found 'new MessageChannel' suddenly stopped working in ServiceWorker due to
http://crbug.com/369115 work. I added Exposed=ServiceWorker in
Source/core/dom/MessageChannel.idl to make it work for now.  (Chris, please let
me know if this is a wrong fix, if so I could revisit this)

[kinuko, 2014-05-09 05:59:17]

The CQ bit was checked by kinuko@chromium.org

[haraken, 2014-05-09 05:59:20]

On 2014/05/09 05:57:57, kinuko wrote:
> Ok I found 'new MessageChannel' suddenly stopped working in ServiceWorker due
to
> http://crbug.com/369115 work. I added Exposed=ServiceWorker in
> Source/core/dom/MessageChannel.idl to make it work for now.  (Chris, please
let
> me know if this is a wrong fix, if so I could revisit this)

The change makes sense to me.

[commit-bot: I haz the power, 2014-05-09 06:00:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/264233003/140001

[commit-bot: I haz the power, 2014-05-09 07:14:00]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/7044)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6788)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/6763)

[kinuko, 2014-05-09 08:34:30]

Committed patchset #7 manually as r173736 (presubmit successful).