Add IPC stubs between browser and ServiceWorker's worker context in the child process

Add IPC stubs between browser and ServiceWorker's worker context in the child process

- Add ServiceWorkerHostMsg_WorkerStarted and ServiceWorkerHostMsg_WorkerStopped
  that are sent from embedded worker to the browser process
- Add ServiceWorkerContextMsg_FetchEvent for messages from embedded worker
  to the browser process

FetchEvent/FetchRequest code is a bit rough as it's meant to be
a placeholder.  I can remove the part from this CL if it doesn't
look reay yet.

The new code uses new IPC class (ServiceWorkerContextMsg) for dispatching
messages specifically sent to the embedded workers's worker context,
which may feel a bit noisy. (Alternative suggestions welcome)

BUG=313530
R=alecflett@chromium.org, jochen@chromium.org, tsepez@chromium.org

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

[kinuko, 2013-12-18 09:18:42]

A bit rough (again), but I think it's ready for review. PTL

[alecflett, 2013-12-19 00:59:10]

lgtm with various suggestions both for here and the future. Really glad to see
the stubs of Fetch!

https://codereview.chromium.org/118103002/diff/1/content/browser/service_work...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/118103002/diff/1/content/browser/service_work...
content/browser/service_worker/embedded_worker_instance.cc:50: // patch if this
part looks too rough)
I say leave it - this gives us something to build on - I'd keep the comment too,
turn it into a TODO

https://codereview.chromium.org/118103002/diff/1/content/common/service_worke...
File content/common/service_worker_interfaces.h (right):

https://codereview.chromium.org/118103002/diff/1/content/common/service_worke...
content/common/service_worker_interfaces.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Quite minor: the name of this file doesn't feel right to me. I wonder if this
could actually be service_worker.h or something? Maybe that's too general. Going
the other way, could this be service_worker_fetch.h ? maybe that's too specific.
I'm not sure.

https://codereview.chromium.org/118103002/diff/1/content/common/service_worke...
content/common/service_worker_interfaces.h:20: std::string referrer;
What is referrer? Does it overlap with the "Referrer" header?
(I think that's ok, because I assume there are cases like https where there is
no actual referrer header but we still need it on onfetch? I did something very
similar in the prototype, as I was basically just following service_worker.ts)

We should just document that with a comment - explain if the 'referrer' header
is also present or not.

https://codereview.chromium.org/118103002/diff/1/content/common/service_worke...
content/common/service_worker_interfaces.h:24: ServiceWorkerFetchRequest();
If you're going to put constructors on this, is it worth putting a constructor
that just sets all the fields in one go?

https://codereview.chromium.org/118103002/diff/1/content/common/service_worke...
File content/common/service_worker_messages.h (right):

https://codereview.chromium.org/118103002/diff/1/content/common/service_worke...
content/common/service_worker_messages.h:78:
IPC_MESSAGE_CONTROL2(ServiceWorkerHostMsg_WorkerStarted,
Should the embedded worker messages be called EmbeddedWorkerHostMsg and
whatnot?)

https://codereview.chromium.org/118103002/diff/1/content/renderer/service_wor...
File content/renderer/service_worker/service_worker_context_message_filter.h
(right):

https://codereview.chromium.org/118103002/diff/1/content/renderer/service_wor...
content/renderer/service_worker/service_worker_context_message_filter.h:12:
class ServiceWorkerContextMessageFilter : public ChildMessageFilter {
This class (and the rest of the patch) seems like this is the first place where
we're starting to bridge the gap between EmbeddedWorkers and ServiceWorkers -
meaning that EmbeddedWorkers as an abstraction have been mostly clean and
ignorant of ServiceWorker specifics. I had been previously wondering if we
should break out embedded worker stuff into its own directory (i.e.
content/browser/embedded_worker etc)

I'm fine with what you're doing here, but maybe we can have a refactoring down
the line, and in the mean time we can start naming things (like messages) with
the EmbeddedWorker name, so that refactoring is more obvious for whoever ends up
doing it.

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

I wasn't fully sure where we should split the two related modules,
embeddedworker and serviceworker, as EW's context handles SW's events etc. But
since having two MessageFilters etc that have too similar names could be
confusing, so I renamed most of them to EW now.  (I'll consider cleaner
separation later)

https://codereview.chromium.org/118103002/diff/1/content/browser/service_work...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/118103002/diff/1/content/browser/service_work...
content/browser/service_worker/embedded_worker_instance.cc:50: // patch if this
part looks too rough)
On 2013/12/19 00:59:11, alecflett wrote:
> I say leave it - this gives us something to build on - I'd keep the comment
too,
> turn it into a TODO

Ok, changed it to a TODO comment

https://codereview.chromium.org/118103002/diff/1/content/common/service_worke...
File content/common/service_worker_interfaces.h (right):

https://codereview.chromium.org/118103002/diff/1/content/common/service_worke...
content/common/service_worker_interfaces.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/12/19 00:59:11, alecflett wrote:
> Quite minor: the name of this file doesn't feel right to me. I wonder if this
> could actually be service_worker.h or something? Maybe that's too general.
Going
> the other way, could this be service_worker_fetch.h ? maybe that's too
specific.
> I'm not sure.

I was thinking about having 'common' types / definitions here that are to be
shared by browser and child process, though currently it only has FetchRequest. 
service_worker_types.h maybe?

https://codereview.chromium.org/118103002/diff/1/content/common/service_worke...
content/common/service_worker_interfaces.h:20: std::string referrer;
On 2013/12/19 00:59:11, alecflett wrote:
> What is referrer? Does it overlap with the "Referrer" header?
> (I think that's ok, because I assume there are cases like https where there is
> no actual referrer header but we still need it on onfetch? I did something
very
> similar in the prototype, as I was basically just following service_worker.ts)
> 
> We should just document that with a comment - explain if the 'referrer' header
> is also present or not.

Some of them are taken from old prototype code, but for now I removed most of
them other than 'url', 'method' and 'headers'.
(Again, this struct is basically for placeholder)

https://codereview.chromium.org/118103002/diff/1/content/common/service_worke...
content/common/service_worker_interfaces.h:24: ServiceWorkerFetchRequest();
On 2013/12/19 00:59:11, alecflett wrote:
> If you're going to put constructors on this, is it worth putting a constructor
> that just sets all the fields in one go?

Yup. Will do when this struct/class's become more stable and it's actually used.

[kinuko, 2013-12-20 08:06:35]

tsepez@: mind reviewing content/common/service_worker_messages.h?

jochen@: mind doing the owner review for content/* changes? (content/common,
content/*.gypi, content/renderer/render_thread_impl.cc)

[jochen (gone - plz use gerrit), 2013-12-20 09:15:57]

https://codereview.chromium.org/118103002/diff/20001/content/common/service_w...
File content/common/service_worker_messages.h (right):

https://codereview.chromium.org/118103002/diff/20001/content/common/service_w...
content/common/service_worker_messages.h:12: #include
"third_party/WebKit/public/web/WebContentSecurityPolicy.h"
why do you need this?

https://codereview.chromium.org/118103002/diff/20001/content/common/service_w...
File content/common/service_worker_types.h (right):

https://codereview.chromium.org/118103002/diff/20001/content/common/service_w...
content/common/service_worker_types.h:26: std::map<std::string, std::string>
headers;
should this be net::HttpRequestHeaders?

https://codereview.chromium.org/118103002/diff/20001/content/common/service_w...
content/common/service_worker_types.h:28: ServiceWorkerFetchRequest();
can you move the ctor/dtor before the members.

can you add parameters to the ctor to initialize the fields (so it's not
possible to create the struct without initializing it)

[kinuko, 2013-12-20 10:12:42]

Thanks for reviewing,

https://codereview.chromium.org/118103002/diff/20001/content/common/service_w...
File content/common/service_worker_messages.h (right):

https://codereview.chromium.org/118103002/diff/20001/content/common/service_w...
content/common/service_worker_messages.h:12: #include
"third_party/WebKit/public/web/WebContentSecurityPolicy.h"
On 2013/12/20 09:15:58, jochen wrote:
> why do you need this?

Nope, was a remnant of old change. Removed.

https://codereview.chromium.org/118103002/diff/20001/content/common/service_w...
File content/common/service_worker_types.h (right):

https://codereview.chromium.org/118103002/diff/20001/content/common/service_w...
content/common/service_worker_types.h:26: std::map<std::string, std::string>
headers;
On 2013/12/20 09:15:58, jochen wrote:
> should this be net::HttpRequestHeaders?

This struct's going to be sent over IPC, while HttpRequestHeaders looks too rich
(non-PODS)

https://codereview.chromium.org/118103002/diff/20001/content/common/service_w...
content/common/service_worker_types.h:28: ServiceWorkerFetchRequest();
On 2013/12/20 09:15:58, jochen wrote:
> can you move the ctor/dtor before the members.

Done.

> can you add parameters to the ctor to initialize the fields (so it's not
> possible to create the struct without initializing it)

Done, but also left empty ctor since it's required to make the IPC code work.

[jochen (gone - plz use gerrit), 2013-12-20 10:19:06]

lgtm

https://codereview.chromium.org/118103002/diff/40001/content/common/service_w...
File content/common/service_worker_types.cc (right):

https://codereview.chromium.org/118103002/diff/40001/content/common/service_w...
content/common/service_worker_types.cc:20:
ServiceWorkerFetchRequest::~ServiceWorkerFetchRequest() {}
nit. for POD structs, it's ok to have the empty dtor/only initializing ctor in
the header

[Tom Sepez, 2013-12-20 18:05:15]

Messages LGTM.

[commit-bot: I haz the power, 2013-12-20 21:54:10]

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

[commit-bot: I haz the power, 2013-12-21 00:37:31]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-06 03:32:16]

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

[kinuko, 2014-01-06 11:00:10]

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