Handle the ViewHostMsg_RunFileChooser IPC message which is sent by content(renderer) in RVH and

Handle the ViewHostMsg_RunFileChooser IPC message which is sent by content(renderer) in RVH and 
invoke the corresponding delegate.

ExtensionHosts and TabContents which are RenderViewHost delegates no longer dispatch the 
ViewHostMsg_RunFileChooser message. Instead this message is handled by overriding the
RunFileChooser method in the RenderViewHostDelegate interface.

We eventually want to get rid of the pattern of sending IPC messages from content and handling
them in chrome and vice versa.

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

[jam, 2011-08-24 20:30:29]

I'm confused. If this is only handled by extensions, then why is it sent from
content?

http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:725: 
why add this blank line? and also above

http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:728: // This does not apply to
messages sent by content.
if you add this line, please explain why that's the case (i.e. because content
messages need to be dispatched in content)

[ananta, 2011-08-24 20:37:58]

On 2011/08/24 20:30:29, John Abd-El-Malek wrote:
> I'm confused. If this is only handled by extensions, then why is it sent from
> content?

content renderer does send this message when the file selection dialog is to be
displayed. For e.g. the Choose file button in gmail. We handle that in
TabContents and invoke the delegate. ExtensionHosts dispatching this message was
missed in that CL.

> 
>
http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
> File content/browser/renderer_host/render_view_host.cc (right):
> 
>
http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
> content/browser/renderer_host/render_view_host.cc:725: 
> why add this blank line? and also above
> 
>
http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
> content/browser/renderer_host/render_view_host.cc:728: // This does not apply
to
> messages sent by content.
> if you add this line, please explain why that's the case (i.e. because content
> messages need to be dispatched in content)

[ananta, 2011-08-24 20:38:05]

http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:725: 
On 2011/08/24 20:30:29, John Abd-El-Malek wrote:
> why add this blank line? and also above

Removed both lines

http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:728: // This does not apply to
messages sent by content.
On 2011/08/24 20:30:29, John Abd-El-Malek wrote:
> if you add this line, please explain why that's the case (i.e. because content
> messages need to be dispatched in content)

I removed the whole comment as delegates can live in chrome. For e.g.
ExtensionHost, in which case we don't want them to dispatch content messages.

[jam, 2011-08-24 21:01:19]

On 2011/08/24 20:37:58, ananta wrote:
> On 2011/08/24 20:30:29, John Abd-El-Malek wrote:
> > I'm confused. If this is only handled by extensions, then why is it sent
from
> > content?
> 
> content renderer does send this message when the file selection dialog is to
be
> displayed. For e.g. the Choose file button in gmail. We handle that in
> TabContents and invoke the delegate. ExtensionHosts dispatching this message
was
> missed in that CL.

I think that was done to avoid calling to delegates, hence why it was dispatched
in two places.

Now that RenderViewHost is dispatching it, no need to dispatch it in TabContents
as well right? and TabContents' handler should be made virtual and moved with
the rest of the RenderViewHostDelegate methods
> 
> > 
> >
>
http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
> > File content/browser/renderer_host/render_view_host.cc (right):
> > 
> >
>
http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
> > content/browser/renderer_host/render_view_host.cc:725: 
> > why add this blank line? and also above
> > 
> >
>
http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
> > content/browser/renderer_host/render_view_host.cc:728: // This does not
apply
> to
> > messages sent by content.
> > if you add this line, please explain why that's the case (i.e. because
content
> > messages need to be dispatched in content)

[ananta, 2011-08-24 21:13:16]

On 2011/08/24 21:01:19, John Abd-El-Malek wrote:
> On 2011/08/24 20:37:58, ananta wrote:
> > On 2011/08/24 20:30:29, John Abd-El-Malek wrote:
> > > I'm confused. If this is only handled by extensions, then why is it sent
> from
> > > content?
> > 
> > content renderer does send this message when the file selection dialog is to
> be
> > displayed. For e.g. the Choose file button in gmail. We handle that in
> > TabContents and invoke the delegate. ExtensionHosts dispatching this message
> was
> > missed in that CL.
> 
> I think that was done to avoid calling to delegates, hence why it was
dispatched
> in two places.
> 
> Now that RenderViewHost is dispatching it, no need to dispatch it in
TabContents
> as well right? and TabContents' handler should be made virtual and moved with
> the rest of the RenderViewHostDelegate methods

Done. I also updated the CL description to reflect this. PTAL

> > 
> > > 
> > >
> >
>
http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
> > > File content/browser/renderer_host/render_view_host.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
> > > content/browser/renderer_host/render_view_host.cc:725: 
> > > why add this blank line? and also above
> > > 
> > >
> >
>
http://codereview.chromium.org/7729002/diff/4002/content/browser/renderer_hos...
> > > content/browser/renderer_host/render_view_host.cc:728: // This does not
> apply
> > to
> > > messages sent by content.
> > > if you add this line, please explain why that's the case (i.e. because
> content
> > > messages need to be dispatched in content)

[jam, 2011-08-24 22:42:57]

lgtm

http://codereview.chromium.org/7729002/diff/7002/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7729002/diff/7002/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.h:512: virtual void
RunFileChooser(RenderViewHost* render_view_host,
nit: please put this in order at the bottom of the file with the other
RenderViewHostDelegate methods (I'm not sure why OnmMessageReceived is public,
but this doesnt need to be)

[ananta, 2011-08-24 22:58:39]

http://codereview.chromium.org/7729002/diff/7002/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7729002/diff/7002/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.h:512: virtual void
RunFileChooser(RenderViewHost* render_view_host,
On 2011/08/24 22:42:57, John Abd-El-Malek wrote:
> nit: please put this in order at the bottom of the file with the other
> RenderViewHostDelegate methods (I'm not sure why OnmMessageReceived is public,
> but this doesnt need to be)

Moved both methods to the bottom with other RVHD overrides.