Move message handlers that don't belong in RenderProcessHostImpl to their own files.

Move RevealFolderInOS to file utilities.

BUG=117452
TEST=no change

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

[jam, 2012-05-02 18:14:59]

http://codereview.chromium.org/10323002/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/file_utilities_message_filter.cc (right):

http://codereview.chromium.org/10323002/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/file_utilities_message_filter.cc:11: #include
"content/public/common/content_client.h"
nit: not needed per convention

http://codereview.chromium.org/10323002/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/mhtml_generation_message_filter.cc (right):

http://codereview.chromium.org/10323002/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/mhtml_generation_message_filter.cc:27:
IPC_MESSAGE_HANDLER(ViewHostMsg_SavedPageAsMHTML, OnSavedPageAsMHTML)
a new filter just for one message? that seems a bit excessive. i'd be hesitant
if new messages get one filter each.

http://codereview.chromium.org/10323002/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/10323002/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_process_host_impl.cc:906: // message filter
and add it in CreateMessageFilters().
I agree if there are a few messages for a feature. but if there's just one, then
I think it's fine here.

[Avi (use Gerrit), 2012-05-02 20:22:23]

Why not a filter for one message? We have plenty of filters for two messages,
and there is one for just one (ResolveProxyMsgHelper).

The idea behind this is to get rid of the responsibility in
RenderProcessHostImpl. It's 1300 lines long, already too big. It should do just
what it needs to do and no more. Being a host for singleton message handlers
feels wrong to me.

http://codereview.chromium.org/10323002/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/file_utilities_message_filter.cc (right):

http://codereview.chromium.org/10323002/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/file_utilities_message_filter.cc:11: #include
"content/public/common/content_client.h"
On 2012/05/02 18:15:00, John Abd-El-Malek wrote:
> nit: not needed per convention

Done.

[jam, 2012-05-03 17:03:55]

On 2012/05/02 20:22:23, Avi wrote:
> Why not a filter for one message? We have plenty of filters for two messages,

I think the expectation there is that there might be more for this feature, if
there's already two.

> and there is one for just one (ResolveProxyMsgHelper).

ResolveProxyMsgHelper was split off because it was originally used by renderer
and plugin process. but then we stopped using it in the plugin after some dead
code cleanup. I'd be fine moving this stuff back to RenderMessageFilter since
it's only used by it.

> 
> The idea behind this is to get rid of the responsibility in
> RenderProcessHostImpl. It's 1300 lines long, already too big. It should do
just
> what it needs to do and no more. Being a host for singleton message handlers
> feels wrong to me.
> 

I feel that trying to save a few lines in render_process_host_impl.cc and adding
~70 elsewhere doesn't help readability. Note that the current filter isn't
correct because it moves dispatching from UI thread to IO thread, so you'd need
to add even more code to have an OverrideThreadForMessage

[Avi (use Gerrit), 2012-05-16 21:28:29]

I put the MHTML generator back.

I'm not sure what issue you refer to re threading. Is the existing filter wrong?

[jam, 2012-05-17 01:20:23]

On 2012/05/16 21:28:29, Avi wrote:
> I put the MHTML generator back.
> 
> I'm not sure what issue you refer to re threading. Is the existing filter
wrong?

RenderProcessHostImpl dispatches IPCs on the UI thread. filters, by default,
dispatch on the IO thread. For the message that you removed, looks like it has
to be dispatched on the UI thread because of the platform_util function that's
called inside CCBC::OpenItem. so
FileUtilitiesMessageFilter::OverrideThreadForMessage can have an if statement in
the beginning checking for this msg and setting the target loop to UI

[Avi (use Gerrit), 2012-05-17 04:23:10]

Oooh. This is new to me; I didn't know that. Is this documented? If not I'll add
a comment.

[Avi (use Gerrit), 2012-05-17 14:43:45]

ptal

[jam, 2012-05-17 20:37:39]

On 2012/05/17 04:23:10, Avi wrote:
> Oooh. This is new to me; I didn't know that. Is this documented? If not I'll
add
> a comment.

it's by definition since they're IPC::MessageFilters

[jam, 2012-05-17 20:39:06]

lgtm with nits

http://codereview.chromium.org/10323002/diff/18001/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/10323002/diff/18001/content/public/browser/con...
content/public/browser/content_browser_client.h:259: // This is called on the UI
thread.
no need for these two comments, see the comment at the top:

The methods are assumed to be called on the UI thread unless otherwise
specified. Use this "escape hatch" sparingly, to thread unless otherwise
specified.

[Avi (use Gerrit), 2012-05-17 21:04:36]

http://codereview.chromium.org/10323002/diff/18001/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/10323002/diff/18001/content/public/browser/con...
content/public/browser/content_browser_client.h:259: // This is called on the UI
thread.
Ah, thanks.

[commit-bot: I haz the power, 2012-05-17 21:06:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/10323002/18002

[commit-bot: I haz the power, 2012-05-17 22:46:39]

Change committed as 137756