Move the worker script loading code to the worker process (phase:4/5)

Move the worker script loading code to the worker process (phase:4/5)

- Stop sending the worker script from the renderer.
- In this change we can remove the pending instances in WorkerServiceImpl.

This is step 4 of moving the worker script loading code from the renderer process to the worker process.
See: http://crbug.com/329786

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

[horo, 2014-01-29 10:51:21]

jam@ nasko@ kinuko@
Could you please review this CL?

Thank you!

[kinuko, 2014-01-30 05:21:06]

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
File content/browser/worker_host/worker_process_host.h (right):

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
content/browser/worker_host/worker_process_host.h:78: void
set_message_port_id(int id) { message_port_id_ = id; }
nit: please put one empty line here

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
content/browser/worker_host/worker_service_impl.cc:302:
params.render_frame_route_id);
nit: indent

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
content/browser/worker_host/worker_service_impl.cc:303: filter->Send(new
ViewMsg_WorkerCreated(route_id));
I'm slightly unsure when this is sent-- it's sent after the renderer gets the
response from the browser for sync CreateWorker message, is it right?

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
content/browser/worker_host/worker_service_impl.cc:378:
DCHECK(iter->NumFilters() == 1);
DCHECK_EQ(instance.url(), iter->url) ?

https://codereview.chromium.org/133093003/diff/550001/content/renderer/websha...
File content/renderer/websharedworker_proxy.cc (right):

https://codereview.chromium.org/133093003/diff/550001/content/renderer/websha...
content/renderer/websharedworker_proxy.cc:106: // The worker is created - now
send off the CreateWorkerContext message and
We no longer send Create(Start)WorkerContext, please update the comment

https://codereview.chromium.org/133093003/diff/550001/content/worker/webshare...
File content/worker/websharedworker_stub.cc (right):

https://codereview.chromium.org/133093003/diff/550001/content/worker/webshare...
content/worker/websharedworker_stub.cc:31: worker_script_loaded_(false) {
I have a feeling that started_ and worker_script_loaded_ can be combined and
probably named like running_ or something.

[kinuko, 2014-01-30 05:35:26]

Some more thoughts/comments.

https://codereview.chromium.org/133093003/diff/550001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/133093003/diff/550001/content/common/view_mes...
content/common/view_messages.h:1222: // Response message to
ViewHostMsg_CreateShared/DedicatedWorker.
ViewHostMsg_CreateWorker

https://codereview.chromium.org/133093003/diff/550001/content/common/view_mes...
content/common/view_messages.h:1227:
IPC_MESSAGE_ROUTED0(ViewMsg_WorkerScriptLoadFailed)
The timing of when each of these messages are sent is not very clear /
symmetric, is there a way we could cleanup this a bit more, or maybe add a note
in the comment?

1. Either WorkerCreated or WorkerScriptLoadFailed could be sent immediately
after CreateWorker is receive by the browser
2. Or both of WorkerCreated and WorkerScriptLoadFailed could be sent if worker
is created by then failed to load the script
3. WorkerConnected is sent only if the worker creation succeeds

It might be slightly cleaner to return more info (e.g. URL_MISMATCH, PENDING,
LOAD_FAILED, CREATED) as out values of sync CreateWorker msg, then:
- WorkerCreated will be always sent if PENDING
- Either WorkerScriptLoaded or WorkerConnected will be sent if CREATED

[horo, 2014-01-30 11:09:13]

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
File content/browser/worker_host/worker_process_host.h (right):

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
content/browser/worker_host/worker_process_host.h:78: void
set_message_port_id(int id) { message_port_id_ = id; }
On 2014/01/30 05:21:07, kinuko wrote:
> nit: please put one empty line here

Done.

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
content/browser/worker_host/worker_service_impl.cc:302:
params.render_frame_route_id);
On 2014/01/30 05:21:07, kinuko wrote:
> nit: indent

Done.

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
content/browser/worker_host/worker_service_impl.cc:303: filter->Send(new
ViewMsg_WorkerCreated(route_id));
On 2014/01/30 05:21:07, kinuko wrote:
> I'm slightly unsure when this is sent-- it's sent after the renderer gets the
> response from the browser for sync CreateWorker message, is it right?

Yes.
This message is sent after the second client of the shared worker send
ViewHostMsg_CreateWorker.
This is [ViewMsg_WorkerCreated] which is received by <<Renderer 2>> in 
https://code.google.com/p/chromium/issues/detail?id=329786#c1

https://codereview.chromium.org/133093003/diff/550001/content/browser/worker_...
content/browser/worker_host/worker_service_impl.cc:378:
DCHECK(iter->NumFilters() == 1);
On 2014/01/30 05:21:07, kinuko wrote:
> DCHECK_EQ(instance.url(), iter->url) ?

Done.

https://codereview.chromium.org/133093003/diff/550001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/133093003/diff/550001/content/common/view_mes...
content/common/view_messages.h:1222: // Response message to
ViewHostMsg_CreateShared/DedicatedWorker.
On 2014/01/30 05:35:27, kinuko wrote:
> ViewHostMsg_CreateWorker

Done.

https://codereview.chromium.org/133093003/diff/550001/content/common/view_mes...
content/common/view_messages.h:1227:
IPC_MESSAGE_ROUTED0(ViewMsg_WorkerScriptLoadFailed)
On 2014/01/30 05:35:27, kinuko wrote:
> The timing of when each of these messages are sent is not very clear /
> symmetric, is there a way we could cleanup this a bit more, or maybe add a
note
> in the comment?
> 
> 1. Either WorkerCreated or WorkerScriptLoadFailed could be sent immediately
> after CreateWorker is receive by the browser
> 2. Or both of WorkerCreated and WorkerScriptLoadFailed could be sent if worker
> is created by then failed to load the script
> 3. WorkerConnected is sent only if the worker creation succeeds
> 
> It might be slightly cleaner to return more info (e.g. URL_MISMATCH, PENDING,
> LOAD_FAILED, CREATED) as out values of sync CreateWorker msg, then:
> - WorkerCreated will be always sent if PENDING
> - Either WorkerScriptLoaded or WorkerConnected will be sent if CREATED

Added comments.

https://codereview.chromium.org/133093003/diff/550001/content/renderer/websha...
File content/renderer/websharedworker_proxy.cc (right):

https://codereview.chromium.org/133093003/diff/550001/content/renderer/websha...
content/renderer/websharedworker_proxy.cc:106: // The worker is created - now
send off the CreateWorkerContext message and
On 2014/01/30 05:21:07, kinuko wrote:
> We no longer send Create(Start)WorkerContext, please update the comment

Done.

https://codereview.chromium.org/133093003/diff/550001/content/worker/webshare...
File content/worker/websharedworker_stub.cc (right):

https://codereview.chromium.org/133093003/diff/550001/content/worker/webshare...
content/worker/websharedworker_stub.cc:31: worker_script_loaded_(false) {
On 2014/01/30 05:21:07, kinuko wrote:
> I have a feeling that started_ and worker_script_loaded_ can be combined and
> probably named like running_ or something.

Done.

[kinuko, 2014-01-30 13:25:48]

lgtm /w some more nits

Adding michaeln@ in case he wants to say something on AppCache related changes
in this patch

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
File content/browser/worker_host/worker_message_filter.cc (right):

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_message_filter.cc:64: *route_id =
MSG_ROUTING_NONE;
nit: indent

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
File content/browser/worker_host/worker_process_host.cc (right):

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:433: }
nit: nest is a bit deep, does it help if we move else clause before and write
like:

for (...) {
  if (i->worker_route_id != worker_route_id) {
    shutdown = false;
    continue;
  }
  // rest of the body
}

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:437:
process_->GetData().handle, RESULT_CODE_NORMAL_EXIT, false);
Please surround 2+ lines body with { }

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:453: }
ditto.  does having 'if (i->worker_route_id() != worker_route_id) continue;'
before the inner loop make it easier to read?

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:453: }
Also we seem to have a lot of places where we loop over instances_ while
comparing worker_route_id(), it might make sense to have a helper method for it 
(could be done in a separate cleanup)

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:726: 
nit: extra empty line

[horo, 2014-01-30 14:12:54]

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
File content/browser/worker_host/worker_message_filter.cc (right):

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_message_filter.cc:64: *route_id =
MSG_ROUTING_NONE;
On 2014/01/30 13:25:48, kinuko wrote:
> nit: indent

Done.

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
File content/browser/worker_host/worker_process_host.cc (right):

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:433: }
On 2014/01/30 13:25:48, kinuko wrote:
> nit: nest is a bit deep, does it help if we move else clause before and write
> like:
> 
> for (...) {
>   if (i->worker_route_id != worker_route_id) {
>     shutdown = false;
>     continue;
>   }
>   // rest of the body
> }

Done.

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:437:
process_->GetData().handle, RESULT_CODE_NORMAL_EXIT, false);
On 2014/01/30 13:25:48, kinuko wrote:
> Please surround 2+ lines body with { }

Done.

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:453: }
On 2014/01/30 13:25:48, kinuko wrote:
> ditto.  does having 'if (i->worker_route_id() != worker_route_id) continue;'
> before the inner loop make it easier to read?

Done.

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:453: }
On 2014/01/30 13:25:48, kinuko wrote:
> Also we seem to have a lot of places where we loop over instances_ while
> comparing worker_route_id(), it might make sense to have a helper method for
it 
> (could be done in a separate cleanup)

I will do so in the next cl.

https://codereview.chromium.org/133093003/diff/640001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:726: 
On 2014/01/30 13:25:48, kinuko wrote:
> nit: extra empty line

Done.

[jam, 2014-01-30 19:46:11]

lgtm

https://codereview.chromium.org/133093003/diff/660001/content/browser/worker_...
File content/browser/worker_host/worker_process_host.cc (right):

https://codereview.chromium.org/133093003/diff/660001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:431: CHECK(j->filter());
nit: this is a bit redundant since the next line crashing will be just as
informative

https://codereview.chromium.org/133093003/diff/660001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:450: CHECK(j->filter());
ditto

https://codereview.chromium.org/133093003/diff/660001/content/common/worker_m...
File content/common/worker_messages.h (right):

https://codereview.chromium.org/133093003/diff/660001/content/common/worker_m...
content/common/worker_messages.h:104:
IPC_MESSAGE_CONTROL1(WorkerHostMsg_WorkerScriptLoaded,
nit: the convention is a blank line between each IPC

[jam, 2014-01-30 19:47:53]

Also, it seems like we need more owners in content/worker and
content/browser/worker_host and for the worker files in content/renderer. Kinuko
has made a lot of changes to these files, and Horo you're making core changes
and obviously know these files. So you can add yourselves to these owners files
in this cl or in a followup.

[horo, 2014-01-31 03:19:14]

https://codereview.chromium.org/133093003/diff/660001/content/browser/worker_...
File content/browser/worker_host/worker_process_host.cc (right):

https://codereview.chromium.org/133093003/diff/660001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:431: CHECK(j->filter());
On 2014/01/30 19:46:11, jam wrote:
> nit: this is a bit redundant since the next line crashing will be just as
> informative

Done.

https://codereview.chromium.org/133093003/diff/660001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:450: CHECK(j->filter());
On 2014/01/30 19:46:11, jam wrote:
> ditto

Done.

https://codereview.chromium.org/133093003/diff/660001/content/common/worker_m...
File content/common/worker_messages.h (right):

https://codereview.chromium.org/133093003/diff/660001/content/common/worker_m...
content/common/worker_messages.h:104:
IPC_MESSAGE_CONTROL1(WorkerHostMsg_WorkerScriptLoaded,
On 2014/01/30 19:46:11, jam wrote:
> nit: the convention is a blank line between each IPC

Done.

[michaeln, 2014-01-31 23:37:44]

https://codereview.chromium.org/133093003/diff/700001/content/worker/worker_w...
File content/worker/worker_webapplicationcachehost_impl.h (right):

https://codereview.chromium.org/133093003/diff/700001/content/worker/worker_w...
content/worker/worker_webapplicationcachehost_impl.h:18: // loaded by the
creator of the worker rather than the worker itself.
Since main resource loading is changed for workers something needs to change so
that the appcache association is made appropiately (if the script was loaded
from a cache, thats the cache it should be associated with). Otherwise the main
script won't be able to load cached scripts via importScript.

[michaeln, 2014-01-31 23:41:46]

https://codereview.chromium.org/133093003/diff/700001/content/worker/worker_w...
File content/worker/worker_webapplicationcachehost_impl.cc (left):

https://codereview.chromium.org/133093003/diff/700001/content/worker/worker_w...
content/worker/worker_webapplicationcachehost_impl.cc:17:
backend()->SelectCacheForSharedWorker(host_id(),
This method probably should get called prior to the script running. We don't
know at ctor time what the appcache_id is, but we do know upon receipt of the
response head for the worker script. Can this call be made during the processing
of that response head?

[nasko, 2014-01-31 23:57:34]

No need to wait for me on this CL. I'm interested to see the usage of
content_security_policy, which is not in this round.

[horo, 2014-02-03 02:32:27]

https://codereview.chromium.org/133093003/diff/700001/content/worker/worker_w...
File content/worker/worker_webapplicationcachehost_impl.cc (left):

https://codereview.chromium.org/133093003/diff/700001/content/worker/worker_w...
content/worker/worker_webapplicationcachehost_impl.cc:17:
backend()->SelectCacheForSharedWorker(host_id(),
On 2014/01/31 23:41:47, michaeln wrote:
> This method probably should get called prior to the script running. We don't
> know at ctor time what the appcache_id is, but we do know upon receipt of the
> response head for the worker script. Can this call be made during the
processing
> of that response head?

"backend()->SelectCacheForSharedWorker" is called when WebSharedWorkerImpl
finishes loading the main worker script in
WebSharedWorkerImpl::didReceiveScriptLoaderResponse().
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
So the main worker script can load the cached script using importScript.

https://codereview.chromium.org/133093003/diff/700001/content/worker/worker_w...
File content/worker/worker_webapplicationcachehost_impl.h (right):

https://codereview.chromium.org/133093003/diff/700001/content/worker/worker_w...
content/worker/worker_webapplicationcachehost_impl.h:18: // loaded by the
creator of the worker rather than the worker itself.
On 2014/01/31 23:37:44, michaeln wrote:
> Since main resource loading is changed for workers something needs to change
so
> that the appcache association is made appropiately (if the script was loaded
> from a cache, thats the cache it should be associated with). Otherwise the
main
> script won't be able to load cached scripts via importScript.

Done.

[horo, 2014-02-03 02:33:20]

On 2014/01/31 23:57:34, nasko wrote:
> No need to wait for me on this CL. I'm interested to see the usage of
> content_security_policy, which is not in this round.

The way of sending content_security_policy from the renderer process to the
worker process is changed.
It was sent in WorkerMsg_StartWorkerContext message, but now it is sent in
WorkerProcessMsg_CreateWorker message.

But the usage of sending content_security_policy is not changed.
It is used to call WorkerThreadStartupData::create() in the worker process.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Could you please give me LGTM for content/common/view_messages.h and
content/common/worker_messages.h?

[nasko, 2014-02-03 15:06:54]

You technically don't need mine, since jam@ already has done it.
I have just one comment on the IPC, but overall LGTM.

https://codereview.chromium.org/133093003/diff/750001/content/common/worker_m...
File content/common/worker_messages.h (right):

https://codereview.chromium.org/133093003/diff/750001/content/common/worker_m...
content/common/worker_messages.h:106:
IPC_MESSAGE_CONTROL1(WorkerHostMsg_WorkerScriptLoaded,
nit: It looks like those messages fit better as routed messages instead of
control, but it looks like more churn that is not needed for this specific CL.

[nasko, 2014-02-03 15:06:59]

You technically don't need mine, since jam@ already has done it.
I have just one comment on the IPC, but overall LGTM.

[jam, 2014-02-03 16:15:23]

https://codereview.chromium.org/133093003/diff/750001/content/common/worker_m...
File content/common/worker_messages.h (right):

https://codereview.chromium.org/133093003/diff/750001/content/common/worker_m...
content/common/worker_messages.h:106:
IPC_MESSAGE_CONTROL1(WorkerHostMsg_WorkerScriptLoaded,
On 2014/02/03 15:06:55, nasko wrote:
> nit: It looks like those messages fit better as routed messages instead of
> control, but it looks like more churn that is not needed for this specific CL.

Sine these IPCs are dispatched by one object per process (WorkerProcessHost),
they should be control. They should be routed if they were dispatched by an
object where multiple instances are present for each process host, and where the
route determines which instance they go to. The class example of that would be
RenderViewHost for renderer IPCs.

[nasko, 2014-02-03 16:37:12]

On 2014/02/03 16:15:23, jam wrote:
>
https://codereview.chromium.org/133093003/diff/750001/content/common/worker_m...
> File content/common/worker_messages.h (right):
> 
>
https://codereview.chromium.org/133093003/diff/750001/content/common/worker_m...
> content/common/worker_messages.h:106:
> IPC_MESSAGE_CONTROL1(WorkerHostMsg_WorkerScriptLoaded,
> On 2014/02/03 15:06:55, nasko wrote:
> > nit: It looks like those messages fit better as routed messages instead of
> > control, but it looks like more churn that is not needed for this specific
CL.
> 
> Sine these IPCs are dispatched by one object per process (WorkerProcessHost),
> they should be control. They should be routed if they were dispatched by an
> object where multiple instances are present for each process host, and where
the
> route determines which instance they go to. The class example of that would be
> RenderViewHost for renderer IPCs.

Before this CL commits, there are two messages sent to the browser, both of
which are different. If there is only one WorkerProcessHost and no other
objects, should WorkerHostMsg_WorkerContextDestroyed then be control message and
not routed?

[michaeln, 2014-02-03 20:31:04]

great, thnx, lgtm

>
https://codereview.chromium.org/133093003/diff/700001/content/worker/worker_w...
> content/worker/worker_webapplicationcachehost_impl.h:18: // loaded by the
> creator of the worker rather than the worker itself.
> On 2014/01/31 23:37:44, michaeln wrote:
> > Since main resource loading is changed for workers something needs to change
> so
> > that the appcache association is made appropiately (if the script was loaded
> > from a cache, thats the cache it should be associated with). Otherwise the
> main
> > script won't be able to load cached scripts via importScript.
> 
> Done.

[horo, 2014-02-04 01:36:35]

On 2014/02/03 16:37:12, nasko wrote:
> On 2014/02/03 16:15:23, jam wrote:
> >
>
https://codereview.chromium.org/133093003/diff/750001/content/common/worker_m...
> > File content/common/worker_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/133093003/diff/750001/content/common/worker_m...
> > content/common/worker_messages.h:106:
> > IPC_MESSAGE_CONTROL1(WorkerHostMsg_WorkerScriptLoaded,
> > On 2014/02/03 15:06:55, nasko wrote:
> > > nit: It looks like those messages fit better as routed messages instead of
> > > control, but it looks like more churn that is not needed for this specific
> CL.
> > 
> > Sine these IPCs are dispatched by one object per process
(WorkerProcessHost),
> > they should be control. They should be routed if they were dispatched by an
> > object where multiple instances are present for each process host, and where
> the
> > route determines which instance they go to. The class example of that would
be
> > RenderViewHost for renderer IPCs.
> 
> Before this CL commits, there are two messages sent to the browser, both of
> which are different. If there is only one WorkerProcessHost and no other
> objects, should WorkerHostMsg_WorkerContextDestroyed then be control message
and
> not routed?

I think WorkerHostMsg_WorkerContextDestroyed should be control message.
I will create another CL.

[horo, 2014-02-04 09:14:58]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-02-04 09:16:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/133093003/750001

[commit-bot: I haz the power, 2014-02-04 10:24:55]

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

[commit-bot: I haz the power, 2014-02-04 10:24:56]

Retried try job too often on ios_dbg_simulator for step(s) url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[horo, 2014-02-04 10:59:51]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-02-04 10:59:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/133093003/750001

[commit-bot: I haz the power, 2014-02-04 12:21:28]

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

[commit-bot: I haz the power, 2014-02-04 12:21:29]

Retried try job too often on ios_dbg_simulator for step(s) url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[horo, 2014-02-04 14:06:33]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-02-04 14:06:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/133093003/750001

[commit-bot: I haz the power, 2014-02-04 15:10:06]

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

[commit-bot: I haz the power, 2014-02-04 15:10:06]

Retried try job too often on ios_dbg_simulator for step(s) url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[skia-commit-bot, 2014-02-04 15:10:15]

CQ bit was unchecked on CL. Ignoring.

[horo, 2014-02-04 20:52:46]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-02-04 20:55:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/133093003/750001

[jam, 2014-02-05 00:03:40]

On 2014/02/03 16:37:12, nasko wrote:
> On 2014/02/03 16:15:23, jam wrote:
> >
>
https://codereview.chromium.org/133093003/diff/750001/content/common/worker_m...
> > File content/common/worker_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/133093003/diff/750001/content/common/worker_m...
> > content/common/worker_messages.h:106:
> > IPC_MESSAGE_CONTROL1(WorkerHostMsg_WorkerScriptLoaded,
> > On 2014/02/03 15:06:55, nasko wrote:
> > > nit: It looks like those messages fit better as routed messages instead of
> > > control, but it looks like more churn that is not needed for this specific
> CL.
> > 
> > Sine these IPCs are dispatched by one object per process
(WorkerProcessHost),
> > they should be control. They should be routed if they were dispatched by an
> > object where multiple instances are present for each process host, and where
> the
> > route determines which instance they go to. The class example of that would
be
> > RenderViewHost for renderer IPCs.
> 
> Before this CL commits, there are two messages sent to the browser, both of
> which are different. If there is only one WorkerProcessHost and no other
> objects, should WorkerHostMsg_WorkerContextDestroyed then be control message
and
> not routed?

right, WorkerHostMsg_WorkerContextDestroyed should be control. i can see why
routing was misused here, because the way it's dispatched (not in a message
handler) makes it easy to read that value. but there are IPC macros for
unpacking IPCs

[commit-bot: I haz the power, 2014-02-05 00:55:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/133093003/750001

[horo, 2014-02-05 03:17:55]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-02-05 03:17:56]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-02-05 06:05:01]

Change committed as 248869