Get rid of RenderViewHostDelegate::Resource and dispatch the IPC messages dir...

Get rid of RenderViewHostDelegate::Resource and dispatch the IPC messages directly in TabContents.


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

[brettw, 2010-12-29 20:20:34]

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.h:542: static void
FilterURL(ChildProcessSecurityPolicy* policy,
Now that this is a public function, can you provide documentation about what it
does and when it needs to be called?

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host_delegate.h:627: // This is used to
give the delegate a chance to filter IPC messages.
Can you also say something like "this will get called for each IPC message
received by the RenderViewHost, and if the delegate returns true, further
processing will not occur."

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/resource_dispatcher_host.cc:1609: type,
NotificationService::AllSources(), Details<T>(detail));
The documentation on the Source for the notifications you changed here is now
incorrect 9still references NavigationController.

It kind of sucks that the listeners for these notifications now need to check
the source themselves, since this is the whole purpose of the notification
source. Is there something we can use instead, like maybe a RVHDelegate pointer,
that would allow us to use the source again?

[jam, 2010-12-29 21:52:47]

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.h:542: static void
FilterURL(ChildProcessSecurityPolicy* policy,
On 2010/12/29 20:20:34, brettw wrote:
> Now that this is a public function, can you provide documentation about what
it
> does and when it needs to be called?

I had avoided adding a comment because these weren't quite clear from the ways
that this function is used, and didn't want to add something incorrect.  added a
brief comment for now

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host_delegate.h:627: // This is used to
give the delegate a chance to filter IPC messages.
On 2010/12/29 20:20:34, brettw wrote:
> Can you also say something like "this will get called for each IPC message
> received by the RenderViewHost, and if the delegate returns true, further
> processing will not occur."

isn't the concept of message filtering spread all over the code?  I feel if we
explicitly say here that returning true from OnMessageHandled will mean that the
message isn't dispatched then we'll have to repeat this comment in lot of other
places, and it'll be kind of redundant

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/resource_dispatcher_host.cc:1609: type,
NotificationService::AllSources(), Details<T>(detail));
On 2010/12/29 20:20:34, brettw wrote:
> The documentation on the Source for the notifications you changed here is now
> incorrect 9still references NavigationController.
> 
> It kind of sucks that the listeners for these notifications now need to check
> the source themselves, since this is the whole purpose of the notification
> source. Is there something we can use instead, like maybe a RVHDelegate
pointer,
> that would allow us to use the source again?

i was on the fence about this, but you convinced me that we should keep it the
same.  i made ResourceDispatcherHost get the navigation controller from the
render process/view id.

[brettw, 2010-12-29 22:19:19]

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/resource_dispatcher_host.cc:1609: type,
NotificationService::AllSources(), Details<T>(detail));
The problem with this solution is that renderer_host shouldn't depend on
TabContents (you should get a checkdeps error for this change now). Also,
GetAsTabContents should be deleted in the future anyway. This is why I suggested
using the delegate pointer.

[jam, 2010-12-29 23:39:46]

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6025009/diff/25002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/resource_dispatcher_host.cc:1609: type,
NotificationService::AllSources(), Details<T>(detail));
On 2010/12/29 22:19:19, brettw wrote:
> The problem with this solution is that renderer_host shouldn't depend on
> TabContents (you should get a checkdeps error for this change now). Also,
> GetAsTabContents should be deleted in the future anyway. This is why I
suggested
> using the delegate pointer.

ah, got it.  done

[brettw, 2010-12-30 00:17:52]

LGTM