Do some refactoring of renderer_host....

Do some refactoring of renderer_host.

This removes the last dependency on tab_contents from the renderer_host code
and into the RenderViewHostDelegate. Some of the tests depended on tab_contents,
so I moved to a new directory with the tab_contents include allowed via DEPS.
Now DEPS can enforce that no additional tab_contents includes are added to
renderer_host.

RenderViewHost delegate is now pure virtual. After spending a while *again*
figuring out why my code didn't work, only to find it was because the default
implementation of a function was getting called instead of the real one, I
decided to make this pure virtual. It is implemented by TabContents, which
implements basically everything, and two other places that implement less. Only
two lists of duplicate functions seems not too bad, although long-term it would
be nice if this delegate was somehow more succinct.

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

[Aaron Boodman, 2009-07-04 16:42:25]

http://codereview.chromium.org/155071/diff/1025/124
File chrome/browser/renderer_host/render_view_host_delegate.h (left):

http://codereview.chromium.org/155071/diff/1025/124#oldcode169
Line 169: virtual void RenderViewCreated(RenderViewHost* render_view_host) { }
Why change this to require an implementation of every method from subclasses? A
lot of the methods are no-ops and create a lot of noise for implementations that
don't use them (eg ExtensionHost).

[brettw, 2009-07-04 17:14:26]

RenderViewHostDelegate is an interface, and I just don't think interfaces should
provide default implementations "for convenience." It's common design, WebKit
uses pure virtual classes for the "Client" interfaces even for some functions
that not all clients care about.

I spent half a day finding a UI test failure in my patch earlier this week that
would not have happened without the default implementation in RVHDelegate. The
"all animated GIFs are broken" bug that I fixed a week ago was also caused by a
default implementation of an overridded function.

RVHDelegate is clearly out of control and I agree it kind of sucks. But there
are only two cases where it sucks: ExtensionHost and InterstitialPage. And I
think it already sucked, this change just makes the suckage more obvious. And at
least this way, if people add a new function to RVHDelegate, they do need to
consider if it might affect any of the other implementor's behaviors.

[brettw, 2009-07-04 18:14:15]

In general you'll only copy and paste the empty implementation, which won't have
any bugs.

In the future, I'd like to split off more sub delegates like I did for "Save"
and "View" so that ExtensionHost and InterstitialPage need to implement only the
more relevant functions.

[Ben Goodger (Google), 2009-07-06 02:59:09]

OK

[brettw, 2009-07-06 16:10:19]

I chatted with the people in my office who convinced me Aaron is right. I'll
make them empty again.

[darin (slow to review), 2009-07-06 18:33:46]

On 2009/07/06 16:10:19, brettw wrote:
> I chatted with the people in my office who convinced me Aaron is right. I'll
> make them empty again.

I've been struggling with this issue myself as far as the WebKit API is
concerned.  I have in the past similarly lost a lot of time debugging an issue
caused by a subclass failing to override a base class method (slight typo,
etc.).  However, in the case of WebKit API, default implementations can help
maintain backwards compatibility as the API evolves.  That benefit doesn't
really apply within Chromium code.  What was the argument that convinced you?