Extract minimal RenderViewHost interface for embedders, leaving

Extract minimal RenderViewHost interface for embedders, leaving
RenderViewHostImpl for content.

TBR=darin@chromium.org
BUG=98716
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124941

[Jói, 2012-02-28 23:13:26]

Hi guys,

I'm signing off for the night and this is more or less ready for review, so I
thought I'd send it out.  I will remove one of the TODOs that I've addressed (on
RenderWidgetHost, about grouping and ordering interface implementations on
subclasses) before committing, and if desired address the other TODOs about
moving some or most of the class documentation to the interfaces (alternatively
this can be a follow-up change).

John:  Main reviewer.  Let me know if you want me to tackle some of the
remaining TODOs (introduced in this change) and send again for review before
committing.  Obviously the more mechanical stuff like moving to separate files,
renaming the existing files, and moving to content namespace will be in separate
relatively simple changes.

Darin:  John requested that I ask you to take a quick look at the diamond
hierarchy and the RenderWidgetHost::AsRWHImpl() method since you handled a
similar case with the diamond hierarchy that starts with WebWidgetClient, and no
downcasting method was needed.  I'm guessing this was only because you never did
a downcast from WebWidgetClient to derived types?  A static_cast downcast is no
longer possible with virtual inheritance since the object layout depends on the
concrete base class, and dynamic_cast isn't an option since we don't have RTTI,
so I think this is the cleanest approach.  Chrome won't be allowed to #include
the definition of RenderWidgetHostImpl, so even though it can see the
AsRWHImpl() method, it won't be able to use it.

The big changes are in content/browser/renderer_host/render_(widget|view)_host.h
and .cc.  The rest should just be cleanup to adjust usage.

The usage of content::Source<RenderWidgetHostImpl> and
content::Source<RenderViewHostImpl> was a bug, corrected in this change.

Cheers,
Jói

[jam, 2012-02-29 01:40:47]

lgtm after Darin oks the AsImpl. I'd really prefer we don't have it
(RenderViewHostDelegate::GetAsWebContents() still bothers me after all these
years, it was supposed to be temporary!) and perhaps this is extra motivation
for us to get rid of RenderWidgetHost completely from chrome.

feel free to do the TODOs and nits in a followup, since I feel your pain about
carrying a change this big locally

http://codereview.chromium.org/9473001/diff/13002/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_browsertest.cc (right):

http://codereview.chromium.org/9473001/diff/13002/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_browsertest.cc:693:
render_view_host()->OnMessageReceivedForTesting(
instead of adding that ForTesting method just for this test, can you keep things
all in chrome (since the receiver of this message is in chrome anyways). i.e.
you can get the TabContentsWrapper from browser()->GetSelectedWebContents() and
then call its translate_tab_helper() to get at the object that dispatches its
method. you'll have to make the BrowserMessageFilter methods on it public, but
that seems fine.

http://codereview.chromium.org/9473001/diff/13002/chrome/browser/memory_detai...
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/9473001/diff/13002/chrome/browser/memory_detai...
chrome/browser/memory_details.cc:210:
const_cast<RenderWidgetHost*>(widget)->AsRWHImpl());
nit: i guess this (and other usage in chrome) is a todo? (no need to add it now,
just want to make sure for my own knowledge)

we have similar loops in other places, perhaps what we really need is just to
have RenderProcessHost have a RenderViewHost iterator on it, and inside content
we do this casting and also the FromIPCChannelListener stuff (of course not in
this change)

http://codereview.chromium.org/9473001/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/9473001/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host.h:148: virtual BackingStore*
GetBackingStore(bool force_create) = 0;
sorry my change conflicted with this!

http://codereview.chromium.org/9473001/diff/13002/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.h (left):

http://codereview.chromium.org/9473001/diff/13002/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.h:112: RenderViewHost*
current_host() const {
nit: this and below should return the Impl, since they're internal methods of
content

[Jói, 2012-02-29 16:07:35]

Darin: I just need your stamp of approval for the
RenderWidgetHost::AsRWHImpl() method.  Alternatively, a suggestion for
how to handle downcasting in the diamond pattern better would be great
:)

Cheers,
Jói


On Wed, Feb 29, 2012 at 1:40 AM,  <jam@chromium.org> wrote:
> lgtm after Darin oks the AsImpl. I'd really prefer we don't have it
> (RenderViewHostDelegate::GetAsWebContents() still bothers me after all these
> years, it was supposed to be temporary!) and perhaps this is extra
> motivation
> for us to get rid of RenderWidgetHost completely from chrome.
>
> feel free to do the TODOs and nits in a followup, since I feel your pain
> about
> carrying a change this big locally
>
>
>
>
http://codereview.chromium.org/9473001/diff/13002/chrome/browser/autofill/aut...
> File chrome/browser/autofill/autofill_browsertest.cc (right):
>
>
http://codereview.chromium.org/9473001/diff/13002/chrome/browser/autofill/aut...
> chrome/browser/autofill/autofill_browsertest.cc:693:
> render_view_host()->OnMessageReceivedForTesting(
> instead of adding that ForTesting method just for this test, can you
> keep things all in chrome (since the receiver of this message is in
> chrome anyways). i.e. you can get the TabContentsWrapper from
> browser()->GetSelectedWebContents() and then call its
> translate_tab_helper() to get at the object that dispatches its method.
> you'll have to make the BrowserMessageFilter methods on it public, but
> that seems fine.
>
>
http://codereview.chromium.org/9473001/diff/13002/chrome/browser/memory_detai...
> File chrome/browser/memory_details.cc (right):
>
>
http://codereview.chromium.org/9473001/diff/13002/chrome/browser/memory_detai...
> chrome/browser/memory_details.cc:210:
> const_cast<RenderWidgetHost*>(widget)->AsRWHImpl());
> nit: i guess this (and other usage in chrome) is a todo? (no need to add
> it now, just want to make sure for my own knowledge)
>
> we have similar loops in other places, perhaps what we really need is
> just to have RenderProcessHost have a RenderViewHost iterator on it, and
> inside content we do this casting and also the FromIPCChannelListener
> stuff (of course not in this change)
>
>
http://codereview.chromium.org/9473001/diff/13002/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host.h (right):
>
>
http://codereview.chromium.org/9473001/diff/13002/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host.h:148: virtual
> BackingStore* GetBackingStore(bool force_create) = 0;
> sorry my change conflicted with this!
>
>
http://codereview.chromium.org/9473001/diff/13002/content/browser/tab_content...
> File content/browser/tab_contents/render_view_host_manager.h (left):
>
>
http://codereview.chromium.org/9473001/diff/13002/content/browser/tab_content...
> content/browser/tab_contents/render_view_host_manager.h:112:
> RenderViewHost* current_host() const {
> nit: this and below should return the Impl, since they're internal
> methods of content
>
> http://codereview.chromium.org/9473001/

[darin (slow to review), 2012-03-01 23:15:39]

http://codereview.chromium.org/9473001/diff/29001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/9473001/diff/29001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.h:301: #if defined(OS_WIN)
nit: maybe this should be #if defined(COMPILER_MSVC) ?

http://codereview.chromium.org/9473001/diff/29001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/9473001/diff/29001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host.h:245: virtual
RenderWidgetHostImpl* AsRWHImpl() = 0;
It is unfortunate that we need this, but I completely get why it exists.

A couple things that are unfortunate:
1-  RenderWidgetHost is abbreviated.  I realize AsRenderWidgetHostImpl() is a
mouthful.
2-  It might be nice if the public entry point to this down-casting
functionality were somehow defined on RenderWidgetHostImpl instead.  It could be
an inline helper that still calls some virtual method defined on
RenderWidgetHost, but the virtual method could be made private.  Not sure if
this helps.

RenderWidgetHostImpl* impl = RenderWidgetHostImpl::From(render_widget_host);

This is somewhat nice because it hides the function from public consumers of
RenderWidgetHost.

You could also have a globally defined function named toRenderWidgetHostImpl
that works just like the From method shown above.

[Jói, 2012-03-02 10:40:36]

Darin, I took your feedback as "OK to commit with a better schema than
AsRWHImpl()" so I switched to the schema you suggested and will commit once
trybots are happy.  If you have any further feedback, I will address it in a
follow-up change, there will already be several so lots of opportunities to fix
things.

Cheers,
Jói

http://codereview.chromium.org/9473001/diff/13002/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_browsertest.cc (right):

http://codereview.chromium.org/9473001/diff/13002/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_browsertest.cc:693:
render_view_host()->OnMessageReceivedForTesting(
On 2012/02/29 01:40:47, John Abd-El-Malek wrote:
> instead of adding that ForTesting method just for this test, can you keep
things
> all in chrome (since the receiver of this message is in chrome anyways). i.e.
> you can get the TabContentsWrapper from browser()->GetSelectedWebContents()
and
> then call its translate_tab_helper() to get at the object that dispatches its
> method. you'll have to make the BrowserMessageFilter methods on it public, but
> that seems fine.

Added a TODO, will do this as a follow-up.

http://codereview.chromium.org/9473001/diff/13002/chrome/browser/memory_detai...
File chrome/browser/memory_details.cc (right):

http://codereview.chromium.org/9473001/diff/13002/chrome/browser/memory_detai...
chrome/browser/memory_details.cc:210:
const_cast<RenderWidgetHost*>(widget)->AsRWHImpl());
On 2012/02/29 01:40:47, John Abd-El-Malek wrote:
> nit: i guess this (and other usage in chrome) is a todo? (no need to add it
now,
> just want to make sure for my own knowledge)
> 
> we have similar loops in other places, perhaps what we really need is just to
> have RenderProcessHost have a RenderViewHost iterator on it, and inside
content
> we do this casting and also the FromIPCChannelListener stuff (of course not in
> this change)

Ah, this was an oversight.  Once I move RenderWidgetHost and RenderViewHost to
their own files and allow Chrome only to #include those, this cast won't work;
I'll figure out a solution, possibly converting the iterator as you suggested.

http://codereview.chromium.org/9473001/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/9473001/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host.h:148: virtual BackingStore*
GetBackingStore(bool force_create) = 0;
On 2012/02/29 01:40:47, John Abd-El-Malek wrote:
> sorry my change conflicted with this! 

Hehe, no worries :)

http://codereview.chromium.org/9473001/diff/13002/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.h (left):

http://codereview.chromium.org/9473001/diff/13002/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.h:112: RenderViewHost*
current_host() const {
On 2012/02/29 01:40:47, John Abd-El-Malek wrote:
> nit: this and below should return the Impl, since they're internal methods of
> content

Done.

http://codereview.chromium.org/9473001/diff/29001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/9473001/diff/29001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.h:301: #if defined(OS_WIN)
On 2012/03/01 23:15:39, darin wrote:
> nit: maybe this should be #if defined(COMPILER_MSVC) ?

Done, changed this as well in render_view_impl.h where the same warning is
disabled.

http://codereview.chromium.org/9473001/diff/29001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/9473001/diff/29001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host.h:245: virtual
RenderWidgetHostImpl* AsRWHImpl() = 0;
On 2012/03/01 23:15:39, darin wrote:
> It is unfortunate that we need this, but I completely get why it exists.
> 
> A couple things that are unfortunate:
> 1-  RenderWidgetHost is abbreviated.  I realize AsRenderWidgetHostImpl() is a
> mouthful.
> 2-  It might be nice if the public entry point to this down-casting
> functionality were somehow defined on RenderWidgetHostImpl instead.  It could
be
> an inline helper that still calls some virtual method defined on
> RenderWidgetHost, but the virtual method could be made private.  Not sure if
> this helps.
> 
> RenderWidgetHostImpl* impl = RenderWidgetHostImpl::From(render_widget_host);
> 
> This is somewhat nice because it hides the function from public consumers of
> RenderWidgetHost.
> 
> You could also have a globally defined function named toRenderWidgetHostImpl
> that works just like the From method shown above.

I switched to a static RenderWidgetHostImpl::From method as suggested.  This
still calls a protected RenderWidgetHost::AsRenderWidgetHostImpl method, since
the cast can't be done statically (so we rely on a virtual method), but I agree
this hides things more nicely from embedders; before it was a public method and
I relied on our DEPS mechanism to make it unusable; now we also have a C++
mechanism to hide it.

[commit-bot: I haz the power, 2012-03-02 13:59:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/9473001/40205

[commit-bot: I haz the power, 2012-03-02 15:25:37]

Try job failure for 9473001-40205 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-02 15:41:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/9473001/40205

[commit-bot: I haz the power, 2012-03-02 17:18:38]

Can't apply patch for file content/browser/renderer_host/render_view_host.cc.
While running patch -p1 --forward --force;
patching file content/browser/renderer_host/render_view_host.cc
Hunk #36 FAILED at 1112.
Hunk #43 FAILED at 1409.
Hunk #44 succeeded at 1533 (offset 5 lines).
Hunk #45 succeeded at 1550 (offset 5 lines).
Hunk #46 succeeded at 1585 (offset 5 lines).
Hunk #47 succeeded at 1603 (offset 5 lines).
Hunk #48 succeeded at 1626 (offset 5 lines).
Hunk #49 succeeded at 1646 (offset 5 lines).
Hunk #50 succeeded at 1656 (offset 5 lines).
2 out of 50 hunks FAILED -- saving rejects to file
content/browser/renderer_host/render_view_host.cc.rej

[commit-bot: I haz the power, 2012-03-04 11:34:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/9473001/44001

[commit-bot: I haz the power, 2012-03-04 13:37:18]

Try job failure for 9473001-44001 (retry) on mac_rel for step "net_unittests".
It's a second try, previously, steps "net_unittests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-03-05 13:36:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/9473001/47002

[commit-bot: I haz the power, 2012-03-05 15:16:03]

Change committed as 124941