Make DOMStorageDispatcherHost be a message filter (and rename it accordingly)...

Make DOMStorageDispatcherHost be a message filter (and rename it accordingly).  It now derives from BrowserMessageFilter and hence gets message dispatching to different threads for free.

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

[jorlow, 2010-12-13 10:36:15]

What's going on here?  I'm pretty sure I reviewed this already (with a thread
darin was on too).  And there's another requested review for this in my inbox. 
Which one am I supposed to be reviewing?  Why the duplication?

http://codereview.chromium.org/5722003/diff/47001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/5722003/diff/47001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/dom_storage_context.h:22: // shared by all the
DOMStorageMessageFilterss that share the same profile.  The
no 2 s's.

[jam, 2010-12-13 16:30:00]

On Mon, Dec 13, 2010 at 2:36 AM, <jorlow@chromium.org> wrote:

> What's going on here?  I'm pretty sure I reviewed this already (with a
> thread
> darin was on too).


Hey Jeremy, you had reviewed most of this.  But then it got bigger when I
moved the dom storage messages into their own file and renamed them.  This
made the filter not duplicate the message names for both its
OnMessageReceived and OverrideThreadForMessage functions.  So I split off
the change.

 And there's another requested review for this in my inbox.
>

The old change was trimmed down just to BrowserMessageFilter, so you can
ignore that.

There's also another change that I just sent you, that does the same for
IndexedDb.

> Which one am I supposed to be reviewing?  Why the duplication?
>
>
>
>
http://codereview.chromium.org/5722003/diff/47001/chrome/browser/in_process_w...
> File chrome/browser/in_process_webkit/dom_storage_context.h (right):
>
>
>
http://codereview.chromium.org/5722003/diff/47001/chrome/browser/in_process_w...
> chrome/browser/in_process_webkit/dom_storage_context.h:22: // shared by
> all the DOMStorageMessageFilterss that share the same profile.  The
> no 2 s's.


will do

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

[jorlow, 2010-12-13 17:56:52]

LGTM

http://codereview.chromium.org/5722003/diff/68001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/5722003/diff/68001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/dom_storage_message_filter.cc:125:
make_scoped_refptr(url_request_context->host_content_settings_map())));
I think you can just get the map from the profile upon construction and then
pass that in.  No need for this stuff.

http://codereview.chromium.org/5722003/diff/68001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/dom_storage_message_filter.cc:269:
ChromeURLRequestContext* DOMStorageMessageFilter::GetRequestContextForURL(
I think we can get rid of this.

http://codereview.chromium.org/5722003/diff/68001/chrome/browser/in_process_w...
File chrome/browser/in_process_webkit/dom_storage_message_filter.h (right):

http://codereview.chromium.org/5722003/diff/68001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/dom_storage_message_filter.h:80:
ChromeURLRequestContext* GetRequestContextForURL(const GURL& url);
probably delete

http://codereview.chromium.org/5722003/diff/68001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/dom_storage_message_filter.h:102:
scoped_refptr<URLRequestContextGetter> request_context_;
i think these can be deleted

http://codereview.chromium.org/5722003/diff/68001/chrome/browser/in_process_w...
chrome/browser/in_process_webkit/dom_storage_message_filter.h:105:
scoped_refptr<URLRequestContextGetter> extensions_request_context_;
i think these can be deleted

http://codereview.chromium.org/5722003/diff/68001/chrome/common/dom_storage_c...
File chrome/common/dom_storage_common.h (right):

http://codereview.chromium.org/5722003/diff/68001/chrome/common/dom_storage_c...
chrome/common/dom_storage_common.h:9: #include "build/build_config.h"
Why is this necessary?

[jam, 2010-12-13 19:07:00]

i've taken the hostcontentsettingsmap directly from the profile per your
suggestion, and also made all the IPC message handler macros not use the
_DELAY_REPLY versions

http://codereview.chromium.org/5722003/diff/68001/chrome/common/dom_storage_c...
File chrome/common/dom_storage_common.h (right):

http://codereview.chromium.org/5722003/diff/68001/chrome/common/dom_storage_c...
chrome/common/dom_storage_common.h:9: #include "build/build_config.h"
On 2010/12/13 17:56:52, jorlow wrote:
> Why is this necessary?

this needs to be done before including basictypes.h I believe, since the latter
depends on OS_XXX being defined which the former does

[jorlow, 2010-12-13 19:25:11]

LGTM minus my 1 comment

http://codereview.chromium.org/5722003/diff/8003/ipc/ipc_message_impl_macros.h
File ipc/ipc_message_impl_macros.h (right):

http://codereview.chromium.org/5722003/diff/8003/ipc/ipc_message_impl_macros....
ipc/ipc_message_impl_macros.h:333: #define
IPC_SYNC_MESSAGE_CONTROL4_3_EXTRA(msg_class, type1_in, type2_in, \
Hmm.....in theory, this probably means we should be switching over that method
to using params.  If someone like Darin or Brett are OK with this, I'm fine
though.

[jam, 2010-12-13 19:37:59]

http://codereview.chromium.org/5722003/diff/8003/ipc/ipc_message_impl_macros.h
File ipc/ipc_message_impl_macros.h (right):

http://codereview.chromium.org/5722003/diff/8003/ipc/ipc_message_impl_macros....
ipc/ipc_message_impl_macros.h:333: #define
IPC_SYNC_MESSAGE_CONTROL4_3_EXTRA(msg_class, type1_in, type2_in, \
On 2010/12/13 19:25:11, jorlow wrote:
> Hmm.....in theory, this probably means we should be switching over that method
> to using params.  If someone like Darin or Brett are OK with this, I'm fine
> though.

async routed/control and sync routed had messages with 5 inputs, so i decided to
make this match (it wasn't done before out of laziness)

[jorlow, 2010-12-13 20:35:34]

LGTM

On Mon, Dec 13, 2010 at 7:37 PM, <jam@chromium.org> wrote:

>
>
> http://codereview.chromium.org/5722003/diff/8003/ipc/ipc_message_impl_macros.h
> File ipc/ipc_message_impl_macros.h (right):
>
>
>
http://codereview.chromium.org/5722003/diff/8003/ipc/ipc_message_impl_macros....
> ipc/ipc_message_impl_macros.h:333: #define
> IPC_SYNC_MESSAGE_CONTROL4_3_EXTRA(msg_class, type1_in, type2_in, \
> On 2010/12/13 19:25:11, jorlow wrote:
>
>> Hmm.....in theory, this probably means we should be switching over
>>
> that method
>
>> to using params.  If someone like Darin or Brett are OK with this, I'm
>>
> fine
>
>> though.
>>
>
> async routed/control and sync routed had messages with 5 inputs, so i
> decided to make this match (it wasn't done before out of laziness)
>
>
> http://codereview.chromium.org/5722003/
>