workers: Introduce a WorkerMessageFilter to ease routing/processing worker messages.

workers: Introduce a WorkerThreadMessageFilter to ease routing/processing worker messages.

Move the task of managing the task-runners for the worker-thread/main-thread in
a common implementation in WorkerThreadMessageFilter, and use that from the rest
of the message filters.

BUG=none
R=jochen@chromium.org, kinuko@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/869b89123602a51ac523d97054262bc4ab6a6933

[sadrul, 2015-01-30 07:36:25]

sadrul@chromium.org changed reviewers:
+ jochen@chromium.org

[sadrul, 2015-01-30 07:36:26]

Hi! Let me know if I should also get reviews from owners of each component (e.g.
bluetooth, geofencing etc.) Thanks

[kinuko, 2015-01-30 08:38:44]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2015-01-30 08:38:45]

non-owner lgtm, with two nits

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
File content/child/worker_message_filter.cc (right):

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
content/child/worker_message_filter.cc:24: return
PickleIterator(msg).ReadInt(ipc_thread_id);
It's true that many module does the same, but I prefer keeping this as a pure
virtual, as this is part of each subclass's IPC logic.

Also from what other subclasses may do this method could be named in a bit more
general way, say, 'GetWorkerThreadIdForMessage()'?

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
File content/child/worker_message_filter.h (right):

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
content/child/worker_message_filter.h:19: class WorkerMessageFilter : public
ChildMessageFilter {
nit: could this be called WorkerThreadMessageFilter rather than
WorkerMessageFilter, as it assumes an existence of strong tie to a single worker
thread (while most other chrome code tries not to use the notion of 'thread'
directly in favor of more generic task runners)?

[kinuko, 2015-01-30 09:13:17]

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
File content/child/worker_message_filter.h (right):

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
content/child/worker_message_filter.h:48: bool OnMessageReceived(const
IPC::Message& msg) override;
Um... if this method is overridden this class wouldn't work.  Since having
default impl for these methods are the whole point of this class, I'd mark these
'final'.

(Or this class could have an inner class that does filtering as
ChildMessageFilter does, which needs more code but will make the interface
cleaner)

[sadrul, 2015-01-30 14:22:37]

Patchset #5 (id:80001) has been deleted

[sadrul, 2015-01-30 14:24:45]

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
File content/child/worker_message_filter.cc (right):

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
content/child/worker_message_filter.cc:24: return
PickleIterator(msg).ReadInt(ipc_thread_id);
On 2015/01/30 08:38:45, kinuko wrote:
> It's true that many module does the same, but I prefer keeping this as a pure
> virtual, as this is part of each subclass's IPC logic.

Instead of repeating this code in the overrides, I have added a
|first_field_in_msg_is_thread_id_| field in WorkerThreadMessageFilter which must
be set upon construction if this default implementation is to be used. Does that
look better?

> 
> Also from what other subclasses may do this method could be named in a bit
more
> general way, say, 'GetWorkerThreadIdForMessage()'?

Done.

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
File content/child/worker_message_filter.h (right):

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
content/child/worker_message_filter.h:19: class WorkerMessageFilter : public
ChildMessageFilter {
On 2015/01/30 08:38:45, kinuko wrote:
> nit: could this be called WorkerThreadMessageFilter rather than
> WorkerMessageFilter, as it assumes an existence of strong tie to a single
worker
> thread (while most other chrome code tries not to use the notion of 'thread'
> directly in favor of more generic task runners)?

Done.

https://codereview.chromium.org/892543002/diff/60001/content/child/worker_mes...
content/child/worker_message_filter.h:48: bool OnMessageReceived(const
IPC::Message& msg) override;
On 2015/01/30 09:13:17, kinuko wrote:
> Um... if this method is overridden this class wouldn't work.  Since having
> default impl for these methods are the whole point of this class, I'd mark
these
> 'final'.

Made these final.

> 
> (Or this class could have an inner class that does filtering as
> ChildMessageFilter does, which needs more code but will make the interface
> cleaner)

[kinuko, 2015-01-30 16:34:17]

https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
File content/child/worker_thread_message_filter.cc (right):

https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
content/child/worker_thread_message_filter.cc:19:
first_field_in_msg_is_thread_id_(first_field_in_msg_is_thread_id) {
I don't think having this in the constructor is a good idea.  Let me try to
clarify why:

1. Boolean parameter is not preferred in general, as it looks often cryptic at
the callsite without comment, which makes the code less readable.
2. This parameter does nothing (is never used) but just adds extra cognitive
load to implementors when a subclass chooses to implement
GetWorkerThreadIdForMessage, which seems a bit awkward design.
3. Whether using the first field in the msg as thread ID or not is a part of
each subclass's logic and not all subclasses do this. I don't think reducing the
total # of lines is considered more important than having a clear separation of
logic.

https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
File content/child/worker_thread_message_filter.h (right):

https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
content/child/worker_thread_message_filter.h:44: // set to true if the default
implementation is used.
Please add a comment about when this returns true or false.

[sadrul, 2015-01-30 17:02:45]

https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
File content/child/worker_thread_message_filter.cc (right):

https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
content/child/worker_thread_message_filter.cc:19:
first_field_in_msg_is_thread_id_(first_field_in_msg_is_thread_id) {
On 2015/01/30 16:34:17, kinuko wrote:
> I don't think having this in the constructor is a good idea.  Let me try to
> clarify why:
> 
> 1. Boolean parameter is not preferred in general, as it looks often cryptic at
> the callsite without comment, which makes the code less readable.
> 2. This parameter does nothing (is never used) but just adds extra cognitive
> load to implementors when a subclass chooses to implement
> GetWorkerThreadIdForMessage, which seems a bit awkward design.
> 3. Whether using the first field in the msg as thread ID or not is a part of
> each subclass's logic and not all subclasses do this. I don't think reducing
the
> total # of lines is considered more important than having a clear separation
of
> logic.

Reducing the number of lines is nice, but my main motivation here is to avoid
having to duplicate code that extracts info from the IPC message.

What if instead of a boolean variable, we add a FirstFieldInIPCIsThreadId()
interface to WorkerThreadMessageFilter instead? That would address both the
concerns you have.

I also want to remove the current overrides of GetWorkerThreadIdForMessage(),
since it looks like the overrides always use a request-id ==> thread-id mapping,
and move that logic into WorkerThreadMessageFilter instead. We can then have an
enum (e.g. IPC_MESSAGE_FIRST_FIELD_IS_THREAD_ID,
IPC_MESSAGE_FIRST_FIELD_IS_REQUEST_ID) that the subclasses set to dictate how
WorkerThreadMessageFilter gets the correct thread-id. I wanted to hold off on it
for the next CL, since NotificationDispatcher currently never removes a mapping,
whereas PushDispatcher and QuotaMessageFilte do, and I wanted to check first if
the behaviour of NotificationDispatcher is deliberate, or an oversight. I think
in this approach, we can remove more code duplication, and be clear/readable at
the same time. If this sounds like a good plan, then I can introduce the enum in
this CL, with just IPC_MESSAGE_FIRST_FIELD_IS_THREAD_ID enum, and introduce
IPC_MESSAGE_FIRST_FIELD_IS_REQUEST_ID in the next CL. Or I can investigate
NotificationDispatcher a bit more, and make all of these changes in this CL. Let
me know what you think.

[sadrul, 2015-01-31 05:54:38]

https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
File content/child/worker_thread_message_filter.cc (right):

https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
content/child/worker_thread_message_filter.cc:19:
first_field_in_msg_is_thread_id_(first_field_in_msg_is_thread_id) {
On 2015/01/30 17:02:45, sadrul wrote:
> On 2015/01/30 16:34:17, kinuko wrote:
> > I don't think having this in the constructor is a good idea.  Let me try to
> > clarify why:
> > 
> > 1. Boolean parameter is not preferred in general, as it looks often cryptic
at
> > the callsite without comment, which makes the code less readable.
> > 2. This parameter does nothing (is never used) but just adds extra cognitive
> > load to implementors when a subclass chooses to implement
> > GetWorkerThreadIdForMessage, which seems a bit awkward design.
> > 3. Whether using the first field in the msg as thread ID or not is a part of
> > each subclass's logic and not all subclasses do this. I don't think reducing
> the
> > total # of lines is considered more important than having a clear separation
> of
> > logic.
> 
> Reducing the number of lines is nice, but my main motivation here is to avoid
> having to duplicate code that extracts info from the IPC message.
> 
> What if instead of a boolean variable, we add a FirstFieldInIPCIsThreadId()
> interface to WorkerThreadMessageFilter instead? That would address both the
> concerns you have.
> 
> I also want to remove the current overrides of GetWorkerThreadIdForMessage(),
> since it looks like the overrides always use a request-id ==> thread-id
mapping,
> and move that logic into WorkerThreadMessageFilter instead. We can then have
an
> enum (e.g. IPC_MESSAGE_FIRST_FIELD_IS_THREAD_ID,
> IPC_MESSAGE_FIRST_FIELD_IS_REQUEST_ID) that the subclasses set to dictate how
> WorkerThreadMessageFilter gets the correct thread-id. I wanted to hold off on
it
> for the next CL, since NotificationDispatcher currently never removes a
mapping,
> whereas PushDispatcher and QuotaMessageFilte do, and I wanted to check first
if
> the behaviour of NotificationDispatcher is deliberate, or an oversight. I
think
> in this approach, we can remove more code duplication, and be clear/readable
at
> the same time. If this sounds like a good plan, then I can introduce the enum
in
> this CL, with just IPC_MESSAGE_FIRST_FIELD_IS_THREAD_ID enum, and introduce
> IPC_MESSAGE_FIRST_FIELD_IS_REQUEST_ID in the next CL. Or I can investigate
> NotificationDispatcher a bit more, and make all of these changes in this CL.
Let
> me know what you think.

Hrm, looks like this wouldn't be possible to do cleanly (the behaviour
difference in NotificationDispatcher from QuotaMessageFilter/PushDispatcher is
deliberate).

What do you think of the proposal about FirstFieldInIPCIsThreadId()? Although
the bool flag feels a bit cleaner than that to me. So maybe just make
GetWorkerThreadIdForMessage a pure virtual as you suggested?

[kinuko, 2015-02-02 06:02:10]

On 2015/01/31 05:54:38, sadrul wrote:
>
https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
> File content/child/worker_thread_message_filter.cc (right):
> 
>
https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
> content/child/worker_thread_message_filter.cc:19:
> first_field_in_msg_is_thread_id_(first_field_in_msg_is_thread_id) {
> On 2015/01/30 17:02:45, sadrul wrote:
> > On 2015/01/30 16:34:17, kinuko wrote:
> > > I don't think having this in the constructor is a good idea.  Let me try
to
> > > clarify why:
> > > 
> > > 1. Boolean parameter is not preferred in general, as it looks often
cryptic
> at
> > > the callsite without comment, which makes the code less readable.
> > > 2. This parameter does nothing (is never used) but just adds extra
cognitive
> > > load to implementors when a subclass chooses to implement
> > > GetWorkerThreadIdForMessage, which seems a bit awkward design.
> > > 3. Whether using the first field in the msg as thread ID or not is a part
of
> > > each subclass's logic and not all subclasses do this. I don't think
reducing
> > the
> > > total # of lines is considered more important than having a clear
separation
> > of
> > > logic.
> > 
> > Reducing the number of lines is nice, but my main motivation here is to
avoid
> > having to duplicate code that extracts info from the IPC message.
> > 
> > What if instead of a boolean variable, we add a FirstFieldInIPCIsThreadId()
> > interface to WorkerThreadMessageFilter instead? That would address both the
> > concerns you have.
> > 
> > I also want to remove the current overrides of
GetWorkerThreadIdForMessage(),
> > since it looks like the overrides always use a request-id ==> thread-id
> mapping,
> > and move that logic into WorkerThreadMessageFilter instead. We can then have
> an
> > enum (e.g. IPC_MESSAGE_FIRST_FIELD_IS_THREAD_ID,
> > IPC_MESSAGE_FIRST_FIELD_IS_REQUEST_ID) that the subclasses set to dictate
how
> > WorkerThreadMessageFilter gets the correct thread-id. I wanted to hold off
on
> it
> > for the next CL, since NotificationDispatcher currently never removes a
> mapping,
> > whereas PushDispatcher and QuotaMessageFilte do, and I wanted to check first
> if
> > the behaviour of NotificationDispatcher is deliberate, or an oversight. I
> think
> > in this approach, we can remove more code duplication, and be clear/readable
> at
> > the same time. If this sounds like a good plan, then I can introduce the
enum
> in
> > this CL, with just IPC_MESSAGE_FIRST_FIELD_IS_THREAD_ID enum, and introduce
> > IPC_MESSAGE_FIRST_FIELD_IS_REQUEST_ID in the next CL. Or I can investigate
> > NotificationDispatcher a bit more, and make all of these changes in this CL.
> Let
> > me know what you think.
> 
> Hrm, looks like this wouldn't be possible to do cleanly (the behaviour
> difference in NotificationDispatcher from QuotaMessageFilter/PushDispatcher is
> deliberate).
> 
> What do you think of the proposal about FirstFieldInIPCIsThreadId()? Although
> the bool flag feels a bit cleaner than that to me. So maybe just make
> GetWorkerThreadIdForMessage a pure virtual as you suggested?

I think I still prefer pure virtual GetWorkerThreadIdForMessage(), or I feel we
can start with the pure virtual one and exploring more dup reduction ideas in a
separate patch. Wdyt?
(Btw I can imagine how you're motivated to remove the dup'ed code, since I've
once tried to do the similar before)

[sadrul, 2015-02-03 04:10:55]

> > What do you think of the proposal about FirstFieldInIPCIsThreadId()?
Although
> > the bool flag feels a bit cleaner than that to me. So maybe just make
> > GetWorkerThreadIdForMessage a pure virtual as you suggested?
> 
> I think I still prefer pure virtual GetWorkerThreadIdForMessage(), or I feel
we
> can start with the pure virtual one and exploring more dup reduction ideas in
a
> separate patch. Wdyt?
> (Btw I can imagine how you're motivated to remove the dup'ed code, since I've
> once tried to do the similar before)

Fair enough. Done.

https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
File content/child/worker_thread_message_filter.h (right):

https://codereview.chromium.org/892543002/diff/100001/content/child/worker_th...
content/child/worker_thread_message_filter.h:44: // set to true if the default
implementation is used.
On 2015/01/30 16:34:17, kinuko wrote:
> Please add a comment about when this returns true or false.

Done.

[kinuko, 2015-02-03 06:28:11]

lgtm

[jochen (gone - plz use gerrit), 2015-02-04 12:48:31]

lgtm

[commit-bot: I haz the power, 2015-02-04 14:20:48]

Patchset 7 (id:??) landed as
https://crrev.com/869b89123602a51ac523d97054262bc4ab6a6933
Cr-Commit-Position: refs/heads/master@{#314556}

[sadrul, 2015-02-04 14:20:58]

Committed patchset #7 (id:140001) manually as
869b89123602a51ac523d97054262bc4ab6a6933 (presubmit successful).