Add a test for cross process service worker messaging.

Add a test for cross process service worker messaging.

This tests the fix from https://codereview.chromium.org/733703002/

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

[Marijn Kruisselbrink, 2014-11-17 23:32:08]

mek@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2014-11-18 00:24:14]

lgtm...

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/postmessage-cross-process.html
(right):

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:7: if
(window.testRunner) {
Since this is validating chrome-specific behavior (cross-process communication)
and references content_shell helpers (testRunner), the test should probably be
in the chromium/ subdir

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:24: var a =
document.createElement('a');
Maybe factor this out into a helper function?

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:26:
a.setAttribute('rel', 'noreferrer');
I assume this is the magic that effectively forces a new process?

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:32:
worker.postMessage({resultport: messageChannel.port2}, [messageChannel.port2]);
wrap at 80 columns

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:44: 'Worker
should post back expected values.');
Can you explain more than the assertion already does, e.g. "Worker should ack
values posted by new window in order" ?

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html
(right):

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html:5:
// waitUntilDone here is necesary to make sure content_shell doesn't exist as
typo: "exist" -> "exit"

[Marijn Kruisselbrink, 2014-11-18 01:16:14]

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/postmessage-cross-process.html
(right):

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:7: if
(window.testRunner) {
On 2014/11/18 00:24:14, jsbell wrote:
> Since this is validating chrome-specific behavior (cross-process
communication)
> and references content_shell helpers (testRunner), the test should probably be
> in the chromium/ subdir
Yeah, I guess this is kind of on the edge between generic and chrome specific
behavior. Nothing it tests is really chrome specific. Being able to send
messages to a port that is transferred to a service worker is a perfectly
generic thing, and the fact that it does some trickery to run this code in a
separate process in chrome (and the need for testRunner.setCanOpenWindows()
because of that) shouldn't effect how well this runs in any other browser.
It's unfortunate that chrome's testing infrastructure requires chrome specific
code to do non chrome specific things like opening windows.
The waitUntilDone in the helper html file can be easily eliminated by instead
including testharnessreport.js, so the window thing is really the only part
where chrome specific code has to be called.

So on the one hand it would be nice to have a non-chrome-specific test that
makes sure messages can be sent to ports sent to service workers,  but I agree
that the magic that is necessary to hit this particular bug in chrome might mean
that this test should not be that non chrome specific test.

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:24: var a =
document.createElement('a');
On 2014/11/18 00:24:14, jsbell wrote:
> Maybe factor this out into a helper function?

Done.

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:26:
a.setAttribute('rel', 'noreferrer');
On 2014/11/18 00:24:14, jsbell wrote:
> I assume this is the magic that effectively forces a new process?
Yes.

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:32:
worker.postMessage({resultport: messageChannel.port2}, [messageChannel.port2]);
On 2014/11/18 00:24:14, jsbell wrote:
> wrap at 80 columns

Done.

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:44: 'Worker
should post back expected values.');
On 2014/11/18 00:24:14, jsbell wrote:
> Can you explain more than the assertion already does, e.g. "Worker should ack
> values posted by new window in order" ?

Done.

[falken, 2014-11-20 02:29:12]

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

[falken, 2014-11-20 02:29:13]

lgtm with some nits/suggestions

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/postmessage-cross-process.html
(right):

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:11: function
openUrlInNewWindow(url) {
In Service Worker test style
<http://www.chromium.org/blink/serviceworker/testing>
function names are snake_case.

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:30: return
get_newest_worker(registration);
You can just use registration.active directly. No need for this helper (this
helper is unnecessarily promisified anyway... see crbug.com/425900)

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html
(right):

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html:5:
// waitUntilDone here is necesary to make sure content_shell doesn't exist as
some typos here: "necesary"->"necessary", "exist"->"exit"

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html:14:
return get_newest_worker(registration);
Since we open this file after the worker activated, you can just use
registration.active here too.

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html:28:
function onMessage(e) {
on_message

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html:35:
};
no semi-colon needed here

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-worker.js
(right):

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-worker.js:19:
}
The checks for resultport and result are a tad confusing to me, I guess they are
needed because you don't know whether 'done' or 'resultport' comes first? Would
it make sense to use a helper function like send_result_if_ready which does
postMessage if resultport and result are populated?

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-worker.js:23:
function onMessage(e) {
on_message

[Marijn Kruisselbrink, 2014-11-20 20:41:13]

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/postmessage-cross-process.html
(right):

https://codereview.chromium.org/730543005/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:7: if
(window.testRunner) {
And I've convinced myself that even though the test doesn't test anything chrome
specific, it probably makes sense to at least for now just move the test to the
chromium directory, since it seems some things would need to be cleaned up to be
a proper w3c test (even though the test as is would run fine on any browser).

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/postmessage-cross-process.html
(right):

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:11: function
openUrlInNewWindow(url) {
On 2014/11/20 02:29:12, falken wrote:
> In Service Worker test style
> <http://www.chromium.org/blink/serviceworker/testing>
> function names are snake_case.

Done.

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:11: function
openUrlInNewWindow(url) {
On 2014/11/20 02:29:12, falken wrote:
> In Service Worker test style
> <http://www.chromium.org/blink/serviceworker/testing>
> function names are snake_case.

Done.

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-cross-process.html:30: return
get_newest_worker(registration);
On 2014/11/20 02:29:12, falken wrote:
> You can just use registration.active directly. No need for this helper (this
> helper is unnecessarily promisified anyway... see crbug.com/425900)

Done.

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html
(right):

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html:5:
// waitUntilDone here is necesary to make sure content_shell doesn't exist as
On 2014/11/20 02:29:13, falken wrote:
> some typos here: "necesary"->"necessary", "exist"->"exit"

Done.

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html:14:
return get_newest_worker(registration);
On 2014/11/20 02:29:12, falken wrote:
> Since we open this file after the worker activated, you can just use
> registration.active here too.

Done.

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html:28:
function onMessage(e) {
On 2014/11/20 02:29:13, falken wrote:
> on_message

Done.

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-helper.html:35:
};
On 2014/11/20 02:29:12, falken wrote:
> no semi-colon needed here

Done.

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-worker.js
(right):

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-worker.js:19:
}
On 2014/11/20 02:29:13, falken wrote:
> The checks for resultport and result are a tad confusing to me, I guess they
are
> needed because you don't know whether 'done' or 'resultport' comes first?
Would
> it make sense to use a helper function like send_result_if_ready which does
> postMessage if resultport and result are populated?

Done.

https://codereview.chromium.org/730543005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/postmessage-cross-process-worker.js:23:
function onMessage(e) {
On 2014/11/20 02:29:13, falken wrote:
> on_message

Done.

[Marijn Kruisselbrink, 2014-11-20 21:44:07]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-11-20 21:44:55]

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

[commit-bot: I haz the power, 2014-11-20 23:39:47]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185723