Chromium-side plumbing for ServiceWorker -> Document postMessage (2/3)

Chromium-side plumbing for ServiceWorker -> Document postMessage (2/3)

This is one of multi-sided patches for postMessage plumbing:
1/3: https://codereview.chromium.org/263143004/ (blink)
2/3: THIS PATCH
3/3: https://codereview.chromium.org/264233003/ (blink, adds Layout test)

BUG=366063
TEST=to be added in the patch 3/3
R=jochen@chromium.org, jsbell@chromium.org, marja@chromium.org, tsepez@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269143

[kinuko, 2014-04-23 11:13:22]

This patch only covers half of the plumbing, though the part is basically ready
for review.

Please review, or pick up remaining parts (e.g. blink side, browser->document
side), or take over if someone wants to.

[kinuko, 2014-05-06 07:49:03]

Updated the code to include all chromium-side changes. PTL

[jochen (gone - plz use gerrit), 2014-05-06 16:09:08]

Marja, can you have a look at this?

[marja, 2014-05-06 16:34:30]

Looks like a plumbing CL is expected to look.

... did I understand it correctly, so these messages cannot contain message
ports, so no port entangling / disentangling is needed?

https://codereview.chromium.org/246023007/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/246023007/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:153: for
(size_t i = 0; i < message_port_ids.size(); ++i) {
This looks very similar to what WebContentsImpl::RouteMessageEvent does; can you
refactor so that the code is only in one place?

[jsbell, 2014-05-06 17:55:19]

lgtm...

re: entangling: this patch matches what we landed previously for
document->worker postMessage (crrev.com/189253002), which allows for posting
ports through. Are we missing something in that pathway?

https://codereview.chromium.org/246023007/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_message_filter.h (right):

https://codereview.chromium.org/246023007/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_message_filter.h:10: #include
"content/common/service_worker/service_worker_messages.h"
Why did this include move?

[kinuko, 2014-05-07 05:56:21]

Updated. marja@ PTAL?

https://codereview.chromium.org/246023007/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/246023007/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:153: for
(size_t i = 0; i < message_port_ids.size(); ++i) {
On 2014/05/06 16:34:30, marja wrote:
> This looks very similar to what WebContentsImpl::RouteMessageEvent does; can
you
> refactor so that the code is only in one place?

Done.

https://codereview.chromium.org/246023007/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_message_filter.h (right):

https://codereview.chromium.org/246023007/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_message_filter.h:10: #include
"content/common/service_worker/service_worker_messages.h"
On 2014/05/06 17:55:19, jsbell wrote:
> Why did this include move?

Nope, some remnants maybe... reverted this change.

[marja, 2014-05-07 06:45:34]

lgtm

[jochen (gone - plz use gerrit), 2014-05-07 06:56:37]

lgtm if you update the test in the third step to pass a message port across
processes and it works

[kinuko, 2014-05-07 07:00:17]

On 2014/05/07 06:56:37, jochen wrote:
> lgtm if you update the test in the third step to pass a message port across
> processes and it works

Yup, I'm working on it. Thanks!

[kinuko, 2014-05-07 15:25:27]

Tom: could you do security review for
content/common/service_worker/service_worker_messages.h ?

[Tom Sepez, 2014-05-07 16:55:23]

lgtm

[kinuko, 2014-05-07 19:02:01]

jsbell@: can you take one more look at the diff between PS3 and PS4?  There was
a subtle threading issue due to SW architecture and I fixed it (or added a
work-around)

[jsbell, 2014-05-07 23:35:48]

lgtm

[michaeln, 2014-05-08 00:00:26]

https://codereview.chromium.org/246023007/diff/60001/content/renderer/service...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/246023007/diff/60001/content/renderer/service...
content/renderer/service_worker/service_worker_script_context.cc:99: // to
overtake those messages.
ouch, that's a subtle ordering dependency

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

The CQ bit was checked by kinuko@chromium.org

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

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

[kinuko, 2014-05-09 01:48:30]

Committed patchset #4 manually as r269143 (presubmit successful).