Introduce abstractions to allow embedders to test RenderViewHost

Introduce abstractions to allow embedders to test RenderViewHost
without knowing about content's internals.

From the embedders' point of view, this is done by splitting the
"tester" interface away from the RenderViewHost interface, rather than
extending the parallel interface/implementation hierarchy; see
comments on TestRenderViewHost for the rationale.

TBR=ben@chromium.org
BUG=98716
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126200

[Jói, 2012-03-08 19:19:00]

Hi John,

This takes care of TestRenderViewHost (and TestRenderWidgetHostView, which
wasn't being used in Chrome).

I haven't started looking very much at TestTabContents, but will tomorrow.

Cheers,
Jói

[jam, 2012-03-08 19:36:07]

qq before I look into this in detail: I'm curious why add an interface around
RenderViewHostTest, instead of exposing that class as is and instead make it
contain a RenderViewHostImpl instead of deriving from it?

the reasons I ask are because I feel an interface adds a cost.  in this case, we
won't have multiple implementations and the rest of the test classes in
content/test are concrete classes. another thing that i worry a little about is
that this makes writing a test which uses this a bit different for content vs
chrome, which might add to developer confusion.

[Jói, 2012-03-08 20:46:05]

Hi John,

After fairly quickly abandoning the idea of extending the parallel
inheritance hierarchy, I started down the path of using composition.
At the point where I had mostly implemented the composited
TestRenderViewHost object and was about to update usages, I saw some
issues and abandoned the approach in favour of the current
split-interface approach.  My thinking on using composition is not
completely worked through, but I think I took it far enough to be sure
it wasn't the right approach.

The problem as far as I see it is that when testing RVH, we put in
place a new factory to create RVH objects.  For content, these need to
actually be RenderViewHostImpl objects; it's insufficient for them to
be an object inheriting from RenderViewHost (e.g. a TestRenderViewHost
that would be suitable for consumption by Chrome, one that inherits
RenderViewHost but delegates all calls to an instance of
RenderViewHostImpl).  So if we were to use composition to provide some
kind of test version of RenderViewHost to Chrome, it seems this would
need to be a wrapper object on top of an underlying (created by the
factory) RenderViewHostImpl instance.  Therefore creation and
instantiation on the Chrome side would seem to get more complex, you
would need to be wrapping RenderViewHost objects in test objects.

Another problem is that because we would still need a test object for
content that exposes not just RenderViewHost but all of
RenderViewHostImpl plus some additional functionality for testing, we
would either need a completely separate test object implementation for
content, e.g. TestRenderViewHostImpl that does _not_ inherit from the
test RenderViewHost object we create for Chrome, or alternatively if
we _do_ inherit from it, we are back to a similar type of parallel
inheritance hierarchy and the complexity (in production code, alas)
that comes with it.

To address your concern re the difference between writing RVH tests in
Chrome vs. content, it would be possible (over time at least) to
convert more of the tests on the content side to use the
RenderViewHostTester interface (and possibly to expand it to
accommodate this) and make the TestRenderViewHost object more of an
implementation detail even within content.

Cheers,
Jói



On Thu, Mar 8, 2012 at 7:36 PM,  <jam@chromium.org> wrote:
> qq before I look into this in detail: I'm curious why add an interface
> around
> RenderViewHostTest, instead of exposing that class as is and instead make it
> contain a RenderViewHostImpl instead of deriving from it?
>
> the reasons I ask are because I feel an interface adds a cost.  in this
> case, we
> won't have multiple implementations and the rest of the test classes in
> content/test are concrete classes. another thing that i worry a little about
> is
> that this makes writing a test which uses this a bit different for content
> vs
> chrome, which might add to developer confusion.
>
> http://codereview.chromium.org/9645003/

[jam, 2012-03-09 06:09:39]

Thanks for the explanation, I understand now. lgtm

[commit-bot: I haz the power, 2012-03-09 15:59:59]

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

[commit-bot: I haz the power, 2012-03-09 16:00:11]

Presubmit check for 9645003-9002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui
    chrome/browser/ui/webui
    chrome/browser/ui/content_settings
    chrome/browser/tabs
    chrome/browser/visitedlink

Presubmit checks took 2.8s to calculate.

[commit-bot: I haz the power, 2012-03-09 16:28:39]

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

[commit-bot: I haz the power, 2012-03-09 18:23:13]

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

[commit-bot: I haz the power, 2012-03-12 15:22:25]

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

[commit-bot: I haz the power, 2012-03-12 16:34:28]

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

[commit-bot: I haz the power, 2012-03-12 16:34:34]

Can't apply patch for file chrome/browser/DEPS.
While running patch -p1 --forward --force;
patching file chrome/browser/DEPS
Hunk #1 FAILED at 47.
1 out of 1 hunk FAILED -- saving rejects to file chrome/browser/DEPS.rej

[commit-bot: I haz the power, 2012-03-12 16:39:24]

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

[commit-bot: I haz the power, 2012-03-12 19:22:11]

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