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

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

- Add content_security_policy and security_policy_type to ViewHostMsg_CreateWorker_Params and WorkerProcessMsg_CreateWorker_Params.
 It is needed because WorkerMsg_StartWorkerContext will be removed.
- Implement workerScriptLoaded, workerScriptLoadFailed and selectAppChacheID.
- Queue WebMessagePortChannel in WebSharedWorkerStub::OnConnect
  When we will add the script loading code in WebSharedWorkerImpl, the worker process can't respond to the connection request which is sent soon after the SharedWorker is created.
  So we have to create WebMessagePortChannelImpl in WebSharedWorkerStub::OnConnect.

This is step 2 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=243864

[horo, 2013-12-19 06:55:52]

kinuko@
Could you please review this?
This cl depends on https://codereview.chromium.org/102243010/

[kinuko, 2013-12-20 05:29:38]

looking good

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
File content/worker/websharedworker_stub.h (right):

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
content/worker/websharedworker_stub.h:33: blink::WebContentSecurityPolicyType
security_policy_type_,
nit: trailing '_' not needed

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
content/worker/websharedworker_stub.h:45: void workerScriptLoadFailed();
CamelCase if they're not for blink overrides

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
File content/worker/websharedworkerclient_proxy.cc (right):

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
content/worker/websharedworkerclient_proxy.cc:74: if (app_cache_host_) {
app_cache_host_ could become stale as it's owned by blink's DocumentLoader, but
here we assume this should be called while it's valid. Maybe add a brief comment
about it?

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
content/worker/websharedworkerclient_proxy.cc:90:
blink::WebApplicationCacheHostClient* client) {
DCHECK(!app_cache_host_) ?

[horo, 2013-12-20 07:31:42]

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
File content/worker/websharedworker_stub.h (right):

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
content/worker/websharedworker_stub.h:33: blink::WebContentSecurityPolicyType
security_policy_type_,
On 2013/12/20 05:29:39, kinuko wrote:
> nit: trailing '_' not needed

Done.

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
content/worker/websharedworker_stub.h:45: void workerScriptLoadFailed();
On 2013/12/20 05:29:39, kinuko wrote:
> CamelCase if they're not for blink overrides

Done.

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
File content/worker/websharedworkerclient_proxy.cc (right):

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
content/worker/websharedworkerclient_proxy.cc:74: if (app_cache_host_) {
On 2013/12/20 05:29:39, kinuko wrote:
> app_cache_host_ could become stale as it's owned by blink's DocumentLoader,
but
> here we assume this should be called while it's valid. Maybe add a brief
comment
> about it?

Done.

https://codereview.chromium.org/115713004/diff/1/content/worker/websharedwork...
content/worker/websharedworkerclient_proxy.cc:90:
blink::WebApplicationCacheHostClient* client) {
On 2013/12/20 05:29:39, kinuko wrote:
> DCHECK(!app_cache_host_) ?

Done.

[kinuko, 2013-12-20 07:43:49]

Looks like the upload's broken? (rietveld appengine issue..)

[horo, 2013-12-20 08:06:20]

On 2013/12/20 07:43:49, kinuko wrote:
> Looks like the upload's broken? (rietveld appengine issue..)

Uploaded again.

[kinuko, 2013-12-20 08:23:20]

lgtm

https://codereview.chromium.org/115713004/diff/20002/content/worker/webshared...
File content/worker/websharedworkerclient_proxy.cc (right):

https://codereview.chromium.org/115713004/diff/20002/content/worker/webshared...
content/worker/websharedworkerclient_proxy.cc:76: // DocumentLoader. So we have
to call selectAppCacheID while it's valid.
nit: 'So we have to call selectAppCacheID' -> 'This method is assumed to be
called' as it's called by blink code?

[horo, 2013-12-20 08:48:15]

https://codereview.chromium.org/115713004/diff/20002/content/worker/webshared...
File content/worker/websharedworkerclient_proxy.cc (right):

https://codereview.chromium.org/115713004/diff/20002/content/worker/webshared...
content/worker/websharedworkerclient_proxy.cc:76: // DocumentLoader. So we have
to call selectAppCacheID while it's valid.
On 2013/12/20 08:23:21, kinuko wrote:
> nit: 'So we have to call selectAppCacheID' -> 'This method is assumed to be
> called' as it's called by blink code?

Done.
It will be called from WebSharedWorkerImpl in didReceiveResponse of the script
loading.

[horo, 2014-01-08 13:23:29]

jam@
Could you please review this CL?

[horo, 2014-01-08 13:24:30]

nasko@

Could you please review view_messages.h and worker_messages.h?

[nasko, 2014-01-08 15:05:01]

https://codereview.chromium.org/115713004/diff/280001/content/common/worker_m...
File content/common/worker_messages.h (left):

https://codereview.chromium.org/115713004/diff/280001/content/common/worker_m...
content/common/worker_messages.h:48:
IPC_ENUM_TRAITS(blink::WebContentSecurityPolicyType)
Why are you removing this? The more the IPC system knows about types, the
better. Even using IPC_ENUM_TRAITS_MAX_VALUE, though that enum doesn't have a
max value defined.

[horo, 2014-01-08 16:01:18]

https://codereview.chromium.org/115713004/diff/280001/content/common/worker_m...
File content/common/worker_messages.h (left):

https://codereview.chromium.org/115713004/diff/280001/content/common/worker_m...
content/common/worker_messages.h:48:
IPC_ENUM_TRAITS(blink::WebContentSecurityPolicyType)
On 2014/01/08 15:05:01, nasko wrote:
> Why are you removing this? The more the IPC system knows about types, the
> better. Even using IPC_ENUM_TRAITS_MAX_VALUE, though that enum doesn't have a
> max value defined.

It is moved to view_messages.h.

[horo, 2014-01-08 16:41:16]

https://codereview.chromium.org/115713004/diff/280001/content/common/worker_m...
File content/common/worker_messages.h (left):

https://codereview.chromium.org/115713004/diff/280001/content/common/worker_m...
content/common/worker_messages.h:48:
IPC_ENUM_TRAITS(blink::WebContentSecurityPolicyType)
On 2014/01/08 16:01:19, horo wrote:
> On 2014/01/08 15:05:01, nasko wrote:
> > Why are you removing this? The more the IPC system knows about types, the
> > better. Even using IPC_ENUM_TRAITS_MAX_VALUE, though that enum doesn't have
a
> > max value defined.
> 
> It is moved to view_messages.h.

I think it should be moved to content_param_traits_macros.h.
So I uploaded patch set 7.

[nasko, 2014-01-08 16:58:40]

IPC messages LGTM

Please add me to the subsequent CL that actually uses those parameters.

[jam, 2014-01-08 19:38:45]

lgtm, as long as there are no security concerns about trusting the
WebContentSecurityPolicyType value from the renderer

[nasko, 2014-01-08 20:34:49]

On 2014/01/08 19:38:45, jam wrote:
> lgtm, as long as there are no security concerns about trusting the
> WebContentSecurityPolicyType value from the renderer

The usage of the new parameters is not in this CL, that's why I want to review
the CL that will start using those.

[commit-bot: I haz the power, 2014-01-08 22:30:56]

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

[commit-bot: I haz the power, 2014-01-08 23:03:34]

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

[horo, 2014-01-08 23:22:55]

jochen@
Could you please review this DEPS change?

Chromium presubmit says
> Missing LGTM from OWNERS of dependencies added to DEPS:
>    '+third_party/WebKit/public/web/WebContentSecurityPolicy.h',

[jam, 2014-01-09 02:12:31]

On 2014/01/08 23:22:55, horo wrote:
> jochen@
> Could you please review this DEPS change?
> 
> Chromium presubmit says
> > Missing LGTM from OWNERS of dependencies added to DEPS:
> >    '+third_party/WebKit/public/web/WebContentSecurityPolicy.h',

lgtm

[commit-bot: I haz the power, 2014-01-09 02:18:07]

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

[commit-bot: I haz the power, 2014-01-09 02:48:17]

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

[jochen (gone - plz use gerrit), 2014-01-09 08:00:28]

lgtm

[commit-bot: I haz the power, 2014-01-09 08:05:05]

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

[commit-bot: I haz the power, 2014-01-09 12:23:47]

Change committed as 243864