Make BrowserMessageFilter support dispatching messages on different threads.

Make BrowserMessageFilter support dispatching messages on different threads.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68870

[jam, 2010-12-03 22:46:01]

btw this is also being renamed since there's no corresponding
DOMStorageDispatcher in the renderer process.

On Fri, Dec 3, 2010 at 2:43 PM, <jam@chromium.org> wrote:

> Reviewers: jorlow,
>
> Description:
> Make DOMStorageDispatcherHost be a filter (and rename it in the process) so
> that
> ResourceMessageFilter doesn't have to know about it.
>
> Please review this at http://codereview.chromium.org/5541005/
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/in_process_webkit/browser_webkitclient_impl.cc
>  M     chrome/browser/in_process_webkit/dom_storage_area.h
>  M     chrome/browser/in_process_webkit/dom_storage_area.cc
>  M     chrome/browser/in_process_webkit/dom_storage_context.h
>  M     chrome/browser/in_process_webkit/dom_storage_context.cc
>  M     chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h
>  M     chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc
>  A  +  chrome/browser/in_process_webkit/dom_storage_message_filter.h
>  A  +  chrome/browser/in_process_webkit/dom_storage_message_filter.cc
>  M     chrome/browser/in_process_webkit/dom_storage_namespace.cc
>  M     chrome/browser/renderer_host/browser_render_process_host.cc
>  M     chrome/browser/renderer_host/resource_message_filter.h
>  M     chrome/browser/renderer_host/resource_message_filter.cc
>  M     chrome/chrome_browser.gypi
>
>
>

[jam, 2010-12-04 01:55:14]

actually, hold-off on reviewing this.  I'm going to make the base filter
class support dispatching to other threads now, since I'm seeing a lot of
classes that use it and I might as well do it together.

On Fri, Dec 3, 2010 at 2:45 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> btw this is also being renamed since there's no corresponding
> DOMStorageDispatcher in the renderer process.
>
>
> On Fri, Dec 3, 2010 at 2:43 PM, <jam@chromium.org> wrote:
>
>> Reviewers: jorlow,
>>
>> Description:
>> Make DOMStorageDispatcherHost be a filter (and rename it in the process)
>> so that
>> ResourceMessageFilter doesn't have to know about it.
>>
>> Please review this at http://codereview.chromium.org/5541005/
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>
>> Affected files:
>>  M     chrome/browser/in_process_webkit/browser_webkitclient_impl.cc
>>  M     chrome/browser/in_process_webkit/dom_storage_area.h
>>  M     chrome/browser/in_process_webkit/dom_storage_area.cc
>>  M     chrome/browser/in_process_webkit/dom_storage_context.h
>>  M     chrome/browser/in_process_webkit/dom_storage_context.cc
>>  M     chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h
>>  M     chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc
>>  A  +  chrome/browser/in_process_webkit/dom_storage_message_filter.h
>>  A  +  chrome/browser/in_process_webkit/dom_storage_message_filter.cc
>>  M     chrome/browser/in_process_webkit/dom_storage_namespace.cc
>>  M     chrome/browser/renderer_host/browser_render_process_host.cc
>>  M     chrome/browser/renderer_host/resource_message_filter.h
>>  M     chrome/browser/renderer_host/resource_message_filter.cc
>>  M     chrome/chrome_browser.gypi
>>
>>
>>
>

[jam, 2010-12-04 05:35:48]

ok it's ready now

Darin: can you review browser_message_filter.cc?  It accomplishes what we've
been discussing in a lightweight way I think.

[darin (slow to review), 2010-12-06 05:14:58]

some quick feedback... didn't read the entire patch...

http://codereview.chromium.org/5541005/diff/107001/chrome/browser/browser_mes...
File chrome/browser/browser_message_filter.cc (right):

http://codereview.chromium.org/5541005/diff/107001/chrome/browser/browser_mes...
chrome/browser/browser_message_filter.cc:71: this,
&BrowserMessageFilter::DispatchMessage, message));
This has the side effect of generating a PostTask for each BrowserMessageFilter,
even though only 1 instance really cares about each particular message.  It
seems like HandleMessageOnThread should provide the caller with a way to say
that they are not interested in a message.

http://codereview.chromium.org/5541005/diff/107001/chrome/browser/in_process_...
File chrome/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/5541005/diff/107001/chrome/browser/in_process_...
chrome/browser/in_process_webkit/dom_storage_message_filter.cc:122: return
BrowserMessageFilter::HandleMessageOnThread(message);
A better return value here might be some kind of "NONE" identifier.

It is also kind of unfortunate to have to repeat message IDs here and again in
OnMessageReceived.  This makes me want some kind of "group ID" that can be used
to select all DOMStorage* messages.

Maybe that's the answer.  Instead of using ViewHostMsg_, we should introduce
DOMStorageHostMsg_?

[jam, 2010-12-06 07:24:43]

http://codereview.chromium.org/5541005/diff/107001/chrome/browser/browser_mes...
File chrome/browser/browser_message_filter.cc (right):

http://codereview.chromium.org/5541005/diff/107001/chrome/browser/browser_mes...
chrome/browser/browser_message_filter.cc:71: this,
&BrowserMessageFilter::DispatchMessage, message));
On 2010/12/06 05:14:58, darin wrote:
> This has the side effect of generating a PostTask for each
BrowserMessageFilter,
> even though only 1 instance really cares about each particular message.  It
> seems like HandleMessageOnThread should provide the caller with a way to say
> that they are not interested in a message.

Each message filter's HandleMessageOnThread should only reply with a non-IO
thread if it wants to handle that specific message, so this shouldn't happen.

http://codereview.chromium.org/5541005/diff/107001/chrome/browser/in_process_...
File chrome/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/5541005/diff/107001/chrome/browser/in_process_...
chrome/browser/in_process_webkit/dom_storage_message_filter.cc:122: return
BrowserMessageFilter::HandleMessageOnThread(message);
On 2010/12/06 05:14:58, darin wrote:
> A better return value here might be some kind of "NONE" identifier.

yeah I wasn't sure how best to structure this, I also considered having a bool
rv that says whether this filter wants to handle the message or not, and if it's
true, then another output parameter would specify the thread.  I don't feel
strongly either way.
> 
> It is also kind of unfortunate to have to repeat message IDs here and again in
> OnMessageReceived.  This makes me want some kind of "group ID" that can be
used
> to select all DOMStorage* messages.
> 
> Maybe that's the answer.  Instead of using ViewHostMsg_, we should introduce
> DOMStorageHostMsg_?

This breaks down when a filter wants to handle messages on different threads. 
For example, DatabaseDispatcherHost and DOMStorageMessageFilter (and maybe
others) want to process most of their messages on the FILE/WEBKIT threads, but
they each want to process one message on the IO thread.  That message is related
to the other messages.  If the grouping decides which thread a message is
dispatched on, then it seems awkward to have create multiple groupings for all
of a feature's messages.  Same with messages that are dispatched on different
threads depending on the platform.

[jam, 2010-12-06 16:42:09]

On Sun, Dec 5, 2010 at 11:24 PM, <jam@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/5541005/diff/107001/chrome/browser/browser_mes...
> File chrome/browser/browser_message_filter.cc (right):
>
>
>
http://codereview.chromium.org/5541005/diff/107001/chrome/browser/browser_mes...
> chrome/browser/browser_message_filter.cc:71: this,
> &BrowserMessageFilter::DispatchMessage, message));
> On 2010/12/06 05:14:58, darin wrote:
>
>> This has the side effect of generating a PostTask for each
>>
> BrowserMessageFilter,
>
>> even though only 1 instance really cares about each particular
>>
> message.  It
>
>> seems like HandleMessageOnThread should provide the caller with a way
>>
> to say
>
>> that they are not interested in a message.
>>
>
> Each message filter's HandleMessageOnThread should only reply with a
> non-IO thread if it wants to handle that specific message, so this
> shouldn't happen.
>
>
>
>
http://codereview.chromium.org/5541005/diff/107001/chrome/browser/in_process_...
> File chrome/browser/in_process_webkit/dom_storage_message_filter.cc
> (right):
>
>
>
http://codereview.chromium.org/5541005/diff/107001/chrome/browser/in_process_...
> chrome/browser/in_process_webkit/dom_storage_message_filter.cc:122:
> return BrowserMessageFilter::HandleMessageOnThread(message);
> On 2010/12/06 05:14:58, darin wrote:
>
>> A better return value here might be some kind of "NONE" identifier.
>>
>
> yeah I wasn't sure how best to structure this, I also considered having
> a bool rv that says whether this filter wants to handle the message or
> not, and if it's true, then another output parameter would specify the
> thread.  I don't feel strongly either way.
>
>
>  It is also kind of unfortunate to have to repeat message IDs here and
>>
> again in
>
>> OnMessageReceived.  This makes me want some kind of "group ID" that
>>
> can be used
>
>> to select all DOMStorage* messages.
>>
>
>  Maybe that's the answer.  Instead of using ViewHostMsg_, we should
>>
> introduce
>
>> DOMStorageHostMsg_?
>>
>
> This breaks down when a filter wants to handle messages on different
> threads.  For example, DatabaseDispatcherHost and
> DOMStorageMessageFilter (and maybe others) want to process most of their
> messages on the FILE/WEBKIT threads, but they each want to process one
> message on the IO thread.  That message is related to the other
> messages.  If the grouping decides which thread a message is dispatched
> on, then it seems awkward to have create multiple groupings for all of a
> feature's messages.  Same with messages that are dispatched on different
> threads depending on the platform.


actually I think I misunderstood your comment, I was thinking you meant
automatic dispatching based on the group id. I understand what you mean now,
and yep I agree it'll be cleaner, since we can just special case the
different messages in HandleMessageOnThread.  I'll update htis change to
reflect that.

>
>
> http://codereview.chromium.org/5541005/
>

[darin (slow to review), 2010-12-06 16:52:37]

On Sun, Dec 5, 2010 at 11:24 PM, <jam@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/5541005/diff/107001/chrome/browser/browser_mes...
> File chrome/browser/browser_message_filter.cc (right):
>
>
>
http://codereview.chromium.org/5541005/diff/107001/chrome/browser/browser_mes...
> chrome/browser/browser_message_filter.cc:71: this,
> &BrowserMessageFilter::DispatchMessage, message));
> On 2010/12/06 05:14:58, darin wrote:
>
>> This has the side effect of generating a PostTask for each
>>
> BrowserMessageFilter,
>
>> even though only 1 instance really cares about each particular
>>
> message.  It
>
>> seems like HandleMessageOnThread should provide the caller with a way
>>
> to say
>
>> that they are not interested in a message.
>>
>
> Each message filter's HandleMessageOnThread should only reply with a
> non-IO thread if it wants to handle that specific message, so this
> shouldn't happen.


Oh, right.



>
>
>
>
http://codereview.chromium.org/5541005/diff/107001/chrome/browser/in_process_...
> File chrome/browser/in_process_webkit/dom_storage_message_filter.cc
> (right):
>
>
>
http://codereview.chromium.org/5541005/diff/107001/chrome/browser/in_process_...
> chrome/browser/in_process_webkit/dom_storage_message_filter.cc:122:
> return BrowserMessageFilter::HandleMessageOnThread(message);
> On 2010/12/06 05:14:58, darin wrote:
>
>> A better return value here might be some kind of "NONE" identifier.
>>
>
> yeah I wasn't sure how best to structure this, I also considered having
> a bool rv that says whether this filter wants to handle the message or
> not, and if it's true, then another output parameter would specify the
> thread.  I don't feel strongly either way.
>
>
Yeah, I kind of think the method would be clearer if it returned a bool
to indicate if the filter wants to handle the message with an out param
that is assigned the target thread ID.



>
>  It is also kind of unfortunate to have to repeat message IDs here and
>>
> again in
>
>> OnMessageReceived.  This makes me want some kind of "group ID" that
>>
> can be used
>
>> to select all DOMStorage* messages.
>>
>
>  Maybe that's the answer.  Instead of using ViewHostMsg_, we should
>>
> introduce
>
>> DOMStorageHostMsg_?
>>
>
> This breaks down when a filter wants to handle messages on different
> threads.  For example, DatabaseDispatcherHost and
> DOMStorageMessageFilter (and maybe others) want to process most of their
> messages on the FILE/WEBKIT threads, but they each want to process one
> message on the IO thread.  That message is related to the other
> messages.  If the grouping decides which thread a message is dispatched
> on, then it seems awkward to have create multiple groupings for all of a
> feature's messages.  Same with messages that are dispatched on different
> threads depending on the platform.
>
>
> actually I think I misunderstood your comment, I was thinking you meant
automatic dispatching based on the
> group id. I understand what you mean now, and yep I agree it'll be
cleaner, since we can just special case the
> different messages in HandleMessageOnThread.  I'll update htis change to
reflect that.

Cool.



>
> http://codereview.chromium.org/5541005/
>

[jam, 2010-12-09 17:44:41]

I've taken out the DOM storage changes since they'll involve renaming messages
and adding new files for the messages.  This is just the filter part with the
change that Darin suggested.

[Matt Perry, 2010-12-09 18:20:06]

mostly LGTM

http://codereview.chromium.org/5541005/diff/146001/chrome/browser/browser_mes...
File chrome/browser/browser_message_filter.cc (right):

http://codereview.chromium.org/5541005/diff/146001/chrome/browser/browser_mes...
chrome/browser/browser_message_filter.cc:66:
ShouldHandleMessageOnThread(message, &thread);
You don't actually use the return value here. Might as well change the signature
to clarify what really matters (setting thread_id).

http://codereview.chromium.org/5541005/diff/146001/chrome/browser/browser_mes...
File chrome/browser/browser_message_filter.h (right):

http://codereview.chromium.org/5541005/diff/146001/chrome/browser/browser_mes...
chrome/browser/browser_message_filter.h:27: bool OnMessageReceived(const
IPC::Message& message);
FYI I don't think the compiler actually prevents you from overriding this. I
think once it's virtual, it's always virtual. Maybe making it private will help.

[jam, 2010-12-09 22:44:51]

http://codereview.chromium.org/5541005/diff/146001/chrome/browser/browser_mes...
File chrome/browser/browser_message_filter.cc (right):

http://codereview.chromium.org/5541005/diff/146001/chrome/browser/browser_mes...
chrome/browser/browser_message_filter.cc:66:
ShouldHandleMessageOnThread(message, &thread);
On 2010/12/09 18:20:06, Matt Perry wrote:
> You don't actually use the return value here. Might as well change the
signature
> to clarify what really matters (setting thread_id).

true, updated

http://codereview.chromium.org/5541005/diff/146001/chrome/browser/browser_mes...
File chrome/browser/browser_message_filter.h (right):

http://codereview.chromium.org/5541005/diff/146001/chrome/browser/browser_mes...
chrome/browser/browser_message_filter.h:27: bool OnMessageReceived(const
IPC::Message& message);
On 2010/12/09 18:20:06, Matt Perry wrote:
> FYI I don't think the compiler actually prevents you from overriding this. I
> think once it's virtual, it's always virtual. Maybe making it private will
help.

you're right.  i tried it out with private and also changing virtual but there
was no way to prevent a derived class from calling it.  i updated the comment.

[brettw, 2010-12-09 23:44:12]

http://codereview.chromium.org/5541005/diff/154002/chrome/browser/browser_mes...
File chrome/browser/browser_message_filter.h (right):

http://codereview.chromium.org/5541005/diff/154002/chrome/browser/browser_mes...
chrome/browser/browser_message_filter.h:36: // change |thread| to the thread id.
 Otherwise leave it alone.
Can you give a little more detail here? I think what I'm reading is that when
this gets called, *thread will tell me what thread I'm on, and if I like that I
handle the message and leave it alone, and if I don't like that, I change
*thread and don't handle the message?

I'm now realizing I'm not supposed to process the message. It would be more
clear (if this works) to call this BrowserThread::ID
GetThreadForMessage(Message& message);

Of if you need it to "default to the current one" maybe it should be
OverrideThreadForMessage with the current signature?

http://codereview.chromium.org/5541005/diff/154002/chrome/browser/browser_mes...
chrome/browser/browser_message_filter.h:56: // Used for dispatching a message to
the derived class.
Google comment style: "Dispatches a message to the derived class."

[jam, 2010-12-10 01:03:52]

http://codereview.chromium.org/5541005/diff/154002/chrome/browser/browser_mes...
File chrome/browser/browser_message_filter.h (right):

http://codereview.chromium.org/5541005/diff/154002/chrome/browser/browser_mes...
chrome/browser/browser_message_filter.h:36: // change |thread| to the thread id.
 Otherwise leave it alone.
On 2010/12/09 23:44:12, brettw wrote:
> Can you give a little more detail here? I think what I'm reading is that when
> this gets called, *thread will tell me what thread I'm on, and if I like that
I
> handle the message and leave it alone, and if I don't like that, I change
> *thread and don't handle the message?
> 
> I'm now realizing I'm not supposed to process the message. It would be more
> clear (if this works) to call this BrowserThread::ID
> GetThreadForMessage(Message& message);
> 
> Of if you need it to "default to the current one" maybe it should be
> OverrideThreadForMessage with the current signature?

Thanks, OverrideThreadForMessage is clearer.  switched.

[brettw, 2010-12-10 17:35:02]

LGTM