Implement ServiceWorker::postMessage() [Chromium]

Implement ServiceWorker::postMessage()

This allows the registering page, or pages loaded via the worker,
to send arbitrary messages to the worker script.

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

[jsbell, 2014-03-10 21:52:05]

This is ready for an initial look. Layout test over on the Blink side
demonstrates two-way communication.

Blink side is https://codereview.chromium.org/185643009 - I haven't looked at
how to split it up for landing, yet.

[michaeln, 2014-03-11 00:59:35]

can u reup this patch?

[jsbell, 2014-03-11 17:40:02]

On 2014/03/11 00:59:35, michaeln wrote:
> can u reup this patch?

Re-upped.

[jsbell, 2014-03-11 22:01:49]

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:201: //
TODO(jsbell): Need version_id passed along to disambiguate?
This is probably the biggest issue with this CL. The message ends up routed to
the "active" instance, regardless of which one is actually referenced. From
chatting w/ alec, it seems as if the version id needs to be passed through to
the renderer on registration so it can be passed back through here, but I'm not
positive.

(That could be a follow-up patch, if we acknowledge that the behavior here is
incorrect until then.)

[michaeln, 2014-03-11 23:17:33]

looking pretty good

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:168:
base::string16 message,
const ref here

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:169:
std::vector<int> sent_message_port_ids) {
ditto here and in the next method sig too

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:195:
MessagePortService::GetInstance()->UpdateMessagePort(
i have no clue about msgport voodoo, but tests pass so it must be right :)

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:201: //
TODO(jsbell): Need version_id passed along to disambiguate?
This was my main question too. I think there are two different cases, not sure
if the answer is any different depending?

1) Case where the page is invoking navigator.serviceWorker.active.postMessage()
to send it to the sw associated with the page.

2) Case where postMessage is called on the return value of a register call
(which may be different than ^^^). A page that has called this method may
outlive update cycles of the sw that return value refers to.

I think what you have coded here might be good for both those cases?

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.h:86:
MessagePortMessageFilter* const message_port_message_filter_;
is this really safe?

https://codereview.chromium.org/189253002/diff/110001/content/child/webmessag...
File content/child/webmessageportchannel_impl.cc (right):

https://codereview.chromium.org/189253002/diff/110001/content/child/webmessag...
content/child/webmessageportchannel_impl.cc:67:
message_port_ids->resize(channels ? channels->size() : 0);
looks like this could be in the if block and w/o for channels ? check

https://codereview.chromium.org/189253002/diff/110001/content/child/webmessag...
content/child/webmessageportchannel_impl.cc:74: webchannel->QueueMessages();
voodoo :)

https://codereview.chromium.org/189253002/diff/110001/content/renderer/servic...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/189253002/diff/110001/content/renderer/servic...
content/renderer/service_worker/service_worker_script_context.cc:73:
std::vector<int> new_routing_ids) {
const refs

[michaeln, 2014-03-11 23:19:19]

> TODO(jsbell): Need version_id passed along to disambiguate?

This is a spec question really.

[kinuko, 2014-03-12 08:19:04]

Looking good to me too

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:201: //
TODO(jsbell): Need version_id passed along to disambiguate?
On 2014/03/11 23:17:34, michaeln wrote:
> This was my main question too. I think there are two different cases, not sure
> if the answer is any different depending?
> 
> 1) Case where the page is invoking
navigator.serviceWorker.active.postMessage()
> to send it to the sw associated with the page.
> 
> 2) Case where postMessage is called on the return value of a register call
> (which may be different than ^^^). A page that has called this method may
> outlive update cycles of the sw that return value refers to.
> 
> I think what you have coded here might be good for both those cases?

In the current code it'd work for both cases, though when we flesh out
registration flow this (talking to active_version) could be talking to a
different version for case 2). So... I think at least we'll need to be able to
distinguish case 1) and 2), or will need version_id. (Leaving TODO for now and
iterate on it while spec evolves more sounds good to me too)

https://codereview.chromium.org/189253002/diff/110001/content/renderer/servic...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/189253002/diff/110001/content/renderer/servic...
content/renderer/service_worker/service_worker_script_context.cc:80: ports[i] =
new WebMessagePortChannelImpl(
(It's a bit sad that WebMessagePortChannelImpl relays all methods to the main
thread... but it's not our issue for now)

[jsbell, 2014-03-12 18:16:16]

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:168:
base::string16 message,
On 2014/03/11 23:17:34, michaeln wrote:
> const ref here

Done.

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:169:
std::vector<int> sent_message_port_ids) {
On 2014/03/11 23:17:34, michaeln wrote:
> ditto here and in the next method sig too

Done.

https://codereview.chromium.org/189253002/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:201: //
TODO(jsbell): Need version_id passed along to disambiguate?
On 2014/03/12 08:19:05, kinuko wrote:
> On 2014/03/11 23:17:34, michaeln wrote:
> > This was my main question too. I think there are two different cases, not
sure
> > if the answer is any different depending?
> > 
> > 1) Case where the page is invoking
> navigator.serviceWorker.active.postMessage()
> > to send it to the sw associated with the page.
> > 
> > 2) Case where postMessage is called on the return value of a register call
> > (which may be different than ^^^). A page that has called this method may
> > outlive update cycles of the sw that return value refers to.
> > 
> > I think what you have coded here might be good for both those cases?
> 
> In the current code it'd work for both cases, though when we flesh out
> registration flow this (talking to active_version) could be talking to a
> different version for case 2).

Agreed, that's the failure case I was thinking of.

> So... I think at least we'll need to be able to
> distinguish case 1) and 2), or will need version_id. (Leaving TODO for now and
> iterate on it while spec evolves more sounds good to me too)

SGTM2.

I'll put in a tracking bug. crbug.com/351797

https://codereview.chromium.org/189253002/diff/110001/content/child/webmessag...
File content/child/webmessageportchannel_impl.cc (right):

https://codereview.chromium.org/189253002/diff/110001/content/child/webmessag...
content/child/webmessageportchannel_impl.cc:67:
message_port_ids->resize(channels ? channels->size() : 0);
On 2014/03/11 23:17:34, michaeln wrote:
> looks like this could be in the if block and w/o for channels ? check

Yep, DCHECK (a later addition) makes it optional. Done.

https://codereview.chromium.org/189253002/diff/110001/content/child/webmessag...
content/child/webmessageportchannel_impl.cc:74: webchannel->QueueMessages();
On 2014/03/11 23:17:34, michaeln wrote:
> voodoo :)

No kidding. I'll get a domain guru to review the voodoo too.

https://codereview.chromium.org/189253002/diff/110001/content/renderer/servic...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/189253002/diff/110001/content/renderer/servic...
content/renderer/service_worker/service_worker_script_context.cc:73:
std::vector<int> new_routing_ids) {
On 2014/03/11 23:17:34, michaeln wrote:
> const refs

Done.

[michaeln, 2014-03-12 23:38:47]

couple minor things + the raw ptr question

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:189: if (status
== SERVICE_WORKER_ERROR_NOT_FOUND)
Maybe test for SUCCESS instead, there could be more than one way in which this
can fail. 

I guess we can DCHECK(result) if status == OK.

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:194:
new_routing_ids[i] = message_port_message_filter_->GetNextRoutingID();
I'm not so sure about the safety of this message_port_message_filter_ raw ptr.

We're not inside an IPC handler at this point. The bind closure around this
method call can potentially have kept 'this' object alive past our channel
having gone away (along with all the other filters).

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:202:
result->active_version()->SendMessage(
Any chance there may be no active version? If not DCHECK or is so, bail out
early in that case.

https://codereview.chromium.org/189253002/diff/130001/content/child/service_w...
File content/child/service_worker/web_service_worker_impl.h (right):

https://codereview.chromium.org/189253002/diff/130001/content/child/service_w...
content/child/service_worker/web_service_worker_impl.h:34: ThreadSafeSender*
thread_safe_sender_;
scoped_refptr here

[jsbell, 2014-03-13 17:53:26]

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:181: static
void NoOpStatusCallback(ServiceWorkerStatusCode status) {}
BTW, this is necessary since ServiceWorkerVersion::SendMessage() takes a ref not
pointer, although its comment says "|callback| can be null if the sender does
not need to know if the message is successfully sent or not"

I could rework SendMessage() to take a pointer, or fix the comment. I could also
move NoOpStatusCallback into ServiceWorkerVersion.h so it could be re-used.
Thoughts?

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:189: if (status
== SERVICE_WORKER_ERROR_NOT_FOUND)
On 2014/03/12 23:38:47, michaeln wrote:
> Maybe test for SUCCESS instead, there could be more than one way in which this
> can fail. 
> 
> I guess we can DCHECK(result) if status == OK.

Done.

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:194:
new_routing_ids[i] = message_port_message_filter_->GetNextRoutingID();
On 2014/03/12 23:38:47, michaeln wrote:
> I'm not so sure about the safety of this message_port_message_filter_ raw ptr.
> 
> We're not inside an IPC handler at this point. The bind closure around this
> method call can potentially have kept 'this' object alive past our channel
> having gone away (along with all the other filters).
> 

Very nice catch. This can be addressed by moving the routing ID allocation into
the OnPostMessage IPC handler, and pass new_routing_ids into the closure.

Which suddenly makes me wonder what happens to all this message port voodoo if
the message ends up not being delivered. I'll try and answer that or ask a guru.

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:202:
result->active_version()->SendMessage(
On 2014/03/12 23:38:47, michaeln wrote:
> Any chance there may be no active version? If not DCHECK or is so, bail out
> early in that case.

If window1 made a registration, window2 deletes the registration, then window2
registers again but it's pending, and window1 posts a message, there could be a
registration but not an active one? That'd be a different registration_id
though, wouldn't it?

Making it resilient to no active_version for now, but can change it to a DCHECK.

[michaeln, 2014-03-13 21:16:41]

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:181: static
void NoOpStatusCallback(ServiceWorkerStatusCode status) {}
> I could rework SendMessage() to take a pointer, or fix the comment. I could
also
> move NoOpStatusCallback into ServiceWorkerVersion.h so it could be re-used.
> Thoughts?

No strong opionion. Putting it in the _utils.h file might be nice. Starts to
matter if we notice a fire & forget pattern being used more.

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:202:
result->active_version()->SendMessage(
> Making it resilient to no active_version for now, but can change it to a
DCHECK.

Resilient sgtm if we're unsure but we need to nail down the semantics of our
growing pile of stuff.

What is the contract that FindRegistrationForXXX holds up, can it find things
that have no active registration? During initial installation, when we're first
firing up a script, seems like thats a time when callers might want to get a
hold of things w/o an active version? But in that case, we could probably use a
FindRegistrationForVersionId(xxxx) which can lookup by id of the
pending_version.

[jsbell, 2014-03-14 18:11:44]

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/189253002/diff/130001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:202:
result->active_version()->SendMessage(
On 2014/03/13 21:16:42, michaeln wrote:
> > Making it resilient to no active_version for now, but can change it to a
> DCHECK.
> 
> Resilient sgtm if we're unsure but we need to nail down the semantics of our
> growing pile of stuff.

+N for large N.
 
> What is the contract that FindRegistrationForXXX holds up, can it find things
> that have no active registration?

Yes, and per email discussion that's going to be valid for postMessage. This
code will definitely need to change.

There's a related case; if a registration is deleted then a postMessage comes
through, I believe FindRegistrationForXXXX would result in NOT_FOUND. This is
handled above, but that struck me as weird since the sender is holding onto what
it thinks is a registered SW. Lots of fun sequences to test.

[jsbell, 2014-03-14 18:13:14]

marja@ - could you please take a look at the "message port" voodoo here and on
the Blink side?

[marja, 2014-03-17 09:15:01]

I left some comments. Could you also add a test to make sure that passing
message ports works? (So, you should send a message port, and then communicate
via that port and verify that it worked.)

Probably something similar to this would work:
https://codereview.chromium.org/24733002/diff/55001/content/browser/renderer_...

https://codereview.chromium.org/189253002/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/189253002/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:191: static
void NoOpStatusCallback(ServiceWorkerStatusCode status) {}
Unnamed namespace instead of static?

(It seems that C++ has been going a bit back and forth about whether this style
should be deprecated or not, and Chromium is also not consistent. But at least
nobody should complain if you use unnamed namespace instead of static, right?)

https://codereview.chromium.org/189253002/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/189253002/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_storage.h:41: // Finds
registration for |document_url| or |pattern| or |registraion_id|.
Nit: typo

https://codereview.chromium.org/189253002/diff/170001/content/child/service_w...
File content/child/service_worker/web_service_worker_impl.cc (right):

https://codereview.chromium.org/189253002/diff/170001/content/child/service_w...
content/child/service_worker/web_service_worker_impl.cc:25: PostMessage(message,
channels);
Why this indirection?

https://codereview.chromium.org/189253002/diff/170001/content/child/webmessag...
File content/child/webmessageportchannel_impl.cc (right):

https://codereview.chromium.org/189253002/diff/170001/content/child/webmessag...
content/child/webmessageportchannel_impl.cc:67: if (channels) {
The same code is here and in WebMessagePortChannelImpl::PostMessage; can it be
moved to a common place?

[marja, 2014-03-17 09:16:24]

(Oops, I just noticed you have tests in the Blink CL, lemme have a look at them
first..)

[jsbell, 2014-03-17 16:49:05]

https://codereview.chromium.org/189253002/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/189253002/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:191: static
void NoOpStatusCallback(ServiceWorkerStatusCode status) {}
On 2014/03/17 09:15:02, marja wrote:
> Unnamed namespace instead of static?
> 
> (It seems that C++ has been going a bit back and forth about whether this
style
> should be deprecated or not, and Chromium is also not consistent. But at least
> nobody should complain if you use unnamed namespace instead of static, right?)

Done.

https://codereview.chromium.org/189253002/diff/170001/content/browser/service...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/189253002/diff/170001/content/browser/service...
content/browser/service_worker/service_worker_storage.h:41: // Finds
registration for |document_url| or |pattern| or |registraion_id|.
On 2014/03/17 09:15:02, marja wrote:
> Nit: typo

Done.

https://codereview.chromium.org/189253002/diff/170001/content/child/service_w...
File content/child/service_worker/web_service_worker_impl.cc (right):

https://codereview.chromium.org/189253002/diff/170001/content/child/service_w...
content/child/service_worker/web_service_worker_impl.cc:25: PostMessage(message,
channels);
On 2014/03/17 09:15:02, marja wrote:
> Why this indirection?

No need now. Squashed the methods.

(Before I factored ExtractMessagePortIDs out, I wanted to sequester the
copy/pasta into its own Chromium-specific method rather than putting all the
logic into the Blink interface impl.)

https://codereview.chromium.org/189253002/diff/170001/content/child/webmessag...
File content/child/webmessageportchannel_impl.cc (right):

https://codereview.chromium.org/189253002/diff/170001/content/child/webmessag...
content/child/webmessageportchannel_impl.cc:67: if (channels) {
On 2014/03/17 09:15:02, marja wrote:
> The same code is here and in WebMessagePortChannelImpl::PostMessage; can it be
> moved to a common place?

Uh... isn't that what I did by having WebMessagePortChannelImpl::PostMessage
call ExtractMessagePortIDs? Am I missing something?

[marja, 2014-03-17 17:04:05]

messageport parts lgtm; you still need a content/ owner.

https://codereview.chromium.org/189253002/diff/170001/content/child/webmessag...
File content/child/webmessageportchannel_impl.cc (right):

https://codereview.chromium.org/189253002/diff/170001/content/child/webmessag...
content/child/webmessageportchannel_impl.cc:67: if (channels) {
On 2014/03/17 16:49:06, jsbell wrote:
> On 2014/03/17 09:15:02, marja wrote:
> > The same code is here and in WebMessagePortChannelImpl::PostMessage; can it
be
> > moved to a common place?
> 
> Uh... isn't that what I did by having WebMessagePortChannelImpl::PostMessage
> call ExtractMessagePortIDs? Am I missing something?

Oops, sorry, missed that. (Wut, it's in the same file. Idk how I managed to miss
that :)

[michaeln, 2014-03-17 19:14:02]

lgtm

https://codereview.chromium.org/189253002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/189253002/diff/190001/content/renderer/servic...
content/renderer/service_worker/service_worker_script_context.cc:73: 
nit: blank line not needed

Also i was confused about how current_request_id_ is used in this class and why
it didn't apply to your new OnMessage. Not sure what do to about that,  maybe
something like...

// There is no corresponding message sent back in response..
DCHECK_EQ(kInvalidRequestId, current_request_id_)

[michaeln, 2014-03-17 19:36:45]

https://codereview.chromium.org/189253002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/189253002/diff/190001/content/renderer/servic...
content/renderer/service_worker/service_worker_script_context.cc:73: 
> // There is no corresponding message sent back in response..
> DCHECK_EQ(kInvalidRequestId, current_request_id_)

On second thought, better doc comments in the .h file about this data member
would have helped a whole lot. Maybe take a stab at adding a doc comment instead
of the code comment here.

// Used for incoming messages from the browser for which an outgoing response
// back to the browser is expected, the id must be sent back with the response.

[jsbell, 2014-03-18 17:20:13]

https://codereview.chromium.org/189253002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/189253002/diff/190001/content/renderer/servic...
content/renderer/service_worker/service_worker_script_context.cc:73: 
On 2014/03/17 19:36:46, michaeln wrote:
> > // There is no corresponding message sent back in response..
> > DCHECK_EQ(kInvalidRequestId, current_request_id_)
> 
> On second thought, better doc comments in the .h file about this data member
> would have helped a whole lot. Maybe take a stab at adding a doc comment
instead
> of the code comment here.
> 
> // Used for incoming messages from the browser for which an outgoing response
> // back to the browser is expected, the id must be sent back with the
response.

Good idea, done. (Might end up using that even for postMessage for blob acks.)

[jsbell, 2014-03-19 00:10:02]

jam@ - could you please review for the generic content/ bits?

[jam, 2014-03-19 21:08:48]

lgtm

https://codereview.chromium.org/189253002/diff/210001/content/child/service_w...
File content/child/service_worker/web_service_worker_impl.cc (right):

https://codereview.chromium.org/189253002/diff/210001/content/child/service_w...
content/child/service_worker/web_service_worker_impl.cc:10: #include
"ipc/ipc_message.h"
nit: don't need this, you get this automatically from the previous header

https://codereview.chromium.org/189253002/diff/210001/content/child/webmessag...
File content/child/webmessageportchannel_impl.h (right):

https://codereview.chromium.org/189253002/diff/210001/content/child/webmessag...
content/child/webmessageportchannel_impl.h:37: std::vector<int>*
message_port_ids);
nit: why not have the return value be the struct since you dcheck that it's
empty initially? also, please add a comment

[jsbell, 2014-03-19 23:52:39]

Thanks. Now waiting on the blink side to land + roll.

https://codereview.chromium.org/189253002/diff/210001/content/child/service_w...
File content/child/service_worker/web_service_worker_impl.cc (right):

https://codereview.chromium.org/189253002/diff/210001/content/child/service_w...
content/child/service_worker/web_service_worker_impl.cc:10: #include
"ipc/ipc_message.h"
On 2014/03/19 21:08:48, jam wrote:
> nit: don't need this, you get this automatically from the previous header

Done. (Left over from before ExtractMessagePortIDs was extracted.)

https://codereview.chromium.org/189253002/diff/210001/content/child/webmessag...
File content/child/webmessageportchannel_impl.h (right):

https://codereview.chromium.org/189253002/diff/210001/content/child/webmessag...
content/child/webmessageportchannel_impl.h:37: std::vector<int>*
message_port_ids);
On 2014/03/19 21:08:48, jam wrote:
> nit: why not have the return value be the struct since you dcheck that it's
> empty initially? also, please add a comment

Ah yes, much cleaner. Done and done.

[jsbell, 2014-03-21 16:24:18]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-03-21 16:24:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/189253002/230001

[commit-bot: I haz the power, 2014-03-21 16:48:30]

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

[commit-bot: I haz the power, 2014-03-21 16:48:31]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[jsbell, 2014-03-21 16:56:42]

tsepez@ - can you review the IPC message additions?

[Tom Sepez, 2014-03-21 17:47:49]

On 2014/03/21 16:56:42, jsbell wrote:
> tsepez@ - can you review the IPC message additions?

Messages LGTM.  My only comment is about the OnMessage() method: since there are
numerous kinds of messages flying around here, IPC, worker, etc., can we get a
name that indicates the specific sort of message in play here?

[jsbell, 2014-03-21 18:38:43]

On 2014/03/21 17:47:49, Tom Sepez wrote:
> On 2014/03/21 16:56:42, jsbell wrote:
> > tsepez@ - can you review the IPC message additions?
> 
> Messages LGTM.  My only comment is about the OnMessage() method: since there
are
> numerous kinds of messages flying around here, IPC, worker, etc., can we get a
> name that indicates the specific sort of message in play here?

Yeah, I can do that. The event delivered to script is just 'message' and I was
modeling this on the naming used around MessagePortMsg_Message. But
consistency/hobgoblins/etc. I'll stick in 'Post' as appropriate, which should
make it more meaningful in context.

[jsbell, 2014-03-21 18:52:53]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-03-21 18:52:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/189253002/250001

[commit-bot: I haz the power, 2014-03-21 18:54:49]

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

[commit-bot: I haz the power, 2014-03-21 18:54:50]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[jsbell, 2014-03-21 21:22:41]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-03-21 21:23:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/189253002/250001

[commit-bot: I haz the power, 2014-03-22 00:45:41]

Change committed as 258727