Use the WebTestProxy for layout tests in content_shell

Use the WebTestProxy for layout tests in content_shell

- Add a hook to the content renderer client to allow embedders to override the
  creation of RenderViewImpls. To simplify this, I consolidate the parameters
  passed to RVI's constructor into a struct.

- Add a content_layouttest_support library that provides a method for creating
  a modified RenderViewImpl suitable for running layout tests.

- Use above library from the content_shell.

BUG=111316
TEST=almost all accessibility tests pass.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=167341

[jochen (gone - plz use gerrit), 2012-11-08 16:00:45]

plz review

This depends on https://bugs.webkit.org/show_bug.cgi?id=101452

[jam, 2012-11-08 17:18:08]

https://codereview.chromium.org/11362161/diff/1/content/layouttest_support/co...
File content/layouttest_support/content_layouttest_support.cc (right):

https://codereview.chromium.org/11362161/diff/1/content/layouttest_support/co...
content/layouttest_support/content_layouttest_support.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
why a top level dir under content for just one file, i.e. especially since this
is only used by tests.

seems like this (and the public file) should both be in content/public/test

https://codereview.chromium.org/11362161/diff/1/content/public/layouttest_sup...
File content/public/layouttest_support/content_layouttest_support.h (right):

https://codereview.chromium.org/11362161/diff/1/content/public/layouttest_sup...
content/public/layouttest_support/content_layouttest_support.h:22:
CONTENT_EXPORT RenderViewImpl* CreateWebTestProxy(
so webkit code now knows about RenderViewImpl? I naively thought it would just
be a WebViewClient that they'd care about?

also, anything in public has to use only public classes/structs. so if this is
public, RenderViewImplParams would have to be public. why is that struct needed?
can you point me to where this function is called so I can see an example?

[jochen (gone - plz use gerrit), 2012-11-08 19:27:48]

Here's a version that works without exposing RenderViewImpl or other internal
names in the public API, wdyt?

[jam, 2012-11-08 20:37:06]

https://codereview.chromium.org/11362161/diff/4001/content/content_renderer.gypi
File content/content_renderer.gypi (right):

https://codereview.chromium.org/11362161/diff/4001/content/content_renderer.g...
content/content_renderer.gypi:276: 'test/content_layouttest_support.cc',
why is this dependency there on shared builds?

https://codereview.chromium.org/11362161/diff/4001/content/public/test/DEPS
File content/public/test/DEPS (right):

https://codereview.chromium.org/11362161/diff/4001/content/public/test/DEPS#n...
content/public/test/DEPS:17: "+content/common/content_export.h",
this is fine for all files, so it should be like content/public/DEPS, i.e.: 

"+content/common/content_export.h",

in line 4

https://codereview.chromium.org/11362161/diff/4001/content/public/test/conten...
File content/public/test/content_layouttest_support.h (right):

https://codereview.chromium.org/11362161/diff/4001/content/public/test/conten...
content/public/test/content_layouttest_support.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: get rid of "content_" in the filename, it's a bit redundant

https://codereview.chromium.org/11362161/diff/4001/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11362161/diff/4001/content/renderer/render_vi...
content/renderer/render_view_impl.h:186: struct RenderViewImplParams {
nit: please move this to content/renderer/render_view_impl_params.{h,cc}

https://codereview.chromium.org/11362161/diff/4001/content/shell/shell_conten...
File content/shell/shell_content_renderer_client.cc (right):

https://codereview.chromium.org/11362161/diff/4001/content/shell/shell_conten...
content/shell/shell_content_renderer_client.cc:25: : latest_test_runner_(NULL) {
nit: tabbing

https://codereview.chromium.org/11362161/diff/4001/content/shell/shell_render...
File content/shell/shell_render_process_observer.h (right):

https://codereview.chromium.org/11362161/diff/4001/content/shell/shell_render...
content/shell/shell_render_process_observer.h:36:
WebTestRunner::WebTestInterfaces* test_interfaces() const {
nit: getters usually are at the end, so i.e. after 43

https://codereview.chromium.org/11362161/diff/4001/content/test/content_layou...
File content/test/content_layouttest_support.cc (right):

https://codereview.chromium.org/11362161/diff/4001/content/test/content_layou...
content/test/content_layouttest_support.cc:24:
reinterpret_cast<RenderViewImplParams*>(params));
nit: no cast needed

[jochen (gone - plz use gerrit), 2012-11-08 20:41:08]

https://codereview.chromium.org/11362161/diff/4001/content/content_renderer.gypi
File content/content_renderer.gypi (right):

https://codereview.chromium.org/11362161/diff/4001/content/content_renderer.g...
content/content_renderer.gypi:276: 'test/content_layouttest_support.cc',
On 2012/11/08 20:37:06, John Abd-El-Malek wrote:
> why is this dependency there on shared builds?

The alternative is to content_export RenderViewImpl and all of its base classes.
Given that shared builds are a development-only thing that seemed like the
better alternative.

In WebKit, we do the same with the webkit_test_support target. In a shared
library build, we just build the test stuff into the webkit.dll

[jam, 2012-11-08 20:49:24]

https://codereview.chromium.org/11362161/diff/4001/content/content_renderer.gypi
File content/content_renderer.gypi (right):

https://codereview.chromium.org/11362161/diff/4001/content/content_renderer.g...
content/content_renderer.gypi:276: 'test/content_layouttest_support.cc',
On 2012/11/08 20:41:08, jochen wrote:
> On 2012/11/08 20:37:06, John Abd-El-Malek wrote:
> > why is this dependency there on shared builds?
> 
> The alternative is to content_export RenderViewImpl and all of its base
classes.

would this only be RenderViewImpl and RenderWidget?

even if we have to export the other interfaces, there aren't that many

> Given that shared builds are a development-only thing that seemed like the
> better alternative.

we put in CONTENT_EXPORT all over the code already to support dev-only shared
builds, so not sure why this would be different? I ask because there's a price
for this complication of the gypi files as well.

> 
> In WebKit, we do the same with the webkit_test_support target. In a shared
> library build, we just build the test stuff into the webkit.dll

[jochen (gone - plz use gerrit), 2012-11-08 21:46:02]

Turns out content_exporting RenderViewImpl is enough.

Addressed all other comments

[jam, 2012-11-09 02:00:19]

lgtm with the one change below, very cool :)

https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi...
content/content_tests.gypi:790: 'target_name': 'content_layouttest_support',
why is this a separate target instead of putting it in test_support_content?

[jam, 2012-11-09 02:05:57]

https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi...
content/content_tests.gypi:790: 'target_name': 'content_layouttest_support',
On 2012/11/09 02:00:19, John Abd-El-Malek wrote:
> why is this a separate target instead of putting it in test_support_content?

(and to be clear, I think it should be in test_support_content)

[jochen (gone - plz use gerrit), 2012-11-09 09:41:00]

https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi...
content/content_tests.gypi:790: 'target_name': 'content_layouttest_support',
On 2012/11/09 02:05:58, John Abd-El-Malek wrote:
> On 2012/11/09 02:00:19, John Abd-El-Malek wrote:
> > why is this a separate target instead of putting it in test_support_content?
> 
> (and to be clear, I think it should be in test_support_content)

Done

I thought it would be nicer not to depend on all of test_support from
content_shell, but I guess it doesn't really matter

[jochen (gone - plz use gerrit), 2012-11-09 10:54:30]

On 2012/11/09 09:41:00, jochen wrote:
> https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi
> File content/content_tests.gypi (right):
> 
>
https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi...
> content/content_tests.gypi:790: 'target_name': 'content_layouttest_support',
> On 2012/11/09 02:05:58, John Abd-El-Malek wrote:
> > On 2012/11/09 02:00:19, John Abd-El-Malek wrote:
> > > why is this a separate target instead of putting it in
test_support_content?
> > 
> > (and to be clear, I think it should be in test_support_content)
> 
> Done
> 
> I thought it would be nicer not to depend on all of test_support from
> content_shell, but I guess it doesn't really matter

Actually, it doesn't work on windows since the additional dependency on WebKit
and TestRunner introduce symbols that collide with test-only stuff from the net
library :-/

[jam, 2012-11-09 17:02:31]

On 2012/11/09 10:54:30, jochen wrote:
> On 2012/11/09 09:41:00, jochen wrote:
> >
https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi
> > File content/content_tests.gypi (right):
> > 
> >
>
https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi...
> > content/content_tests.gypi:790: 'target_name': 'content_layouttest_support',
> > On 2012/11/09 02:05:58, John Abd-El-Malek wrote:
> > > On 2012/11/09 02:00:19, John Abd-El-Malek wrote:
> > > > why is this a separate target instead of putting it in
> test_support_content?
> > > 
> > > (and to be clear, I think it should be in test_support_content)
> > 
> > Done
> > 
> > I thought it would be nicer not to depend on all of test_support from
> > content_shell, but I guess it doesn't really matter
> 
> Actually, it doesn't work on windows since the additional dependency on WebKit
> and TestRunner introduce symbols that collide with test-only stuff from the
net
> library :-/

do you have a link to the try run or a copy of the build output? i.e. why does
it not fail when it's a separate target. I'd like to see if there's a workaround
we can do.

[jochen (gone - plz use gerrit), 2012-11-09 17:18:54]

http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

It's a collision on TestRunner
On Nov 9, 2012 6:02 PM, <jam@chromium.org> wrote:

> On 2012/11/09 10:54:30, jochen wrote:
>
>> On 2012/11/09 09:41:00, jochen wrote:
>> >
>>
> https://codereview.chromium.**org/11362161/diff/1007/**
>
content/content_tests.gypi<https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi>
>
>> > File content/content_tests.gypi (right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/11362161/diff/1007/**
>
content/content_tests.gypi#**newcode790<https://codereview.chromium.org/11362161/diff/1007/content/content_tests.gypi#newcode790>
>
>> > content/content_tests.gypi:**790: 'target_name':
>> 'content_layouttest_support',
>> > On 2012/11/09 02:05:58, John Abd-El-Malek wrote:
>> > > On 2012/11/09 02:00:19, John Abd-El-Malek wrote:
>> > > > why is this a separate target instead of putting it in
>> test_support_content?
>> > >
>> > > (and to be clear, I think it should be in test_support_content)
>> >
>> > Done
>> >
>> > I thought it would be nicer not to depend on all of test_support from
>> > content_shell, but I guess it doesn't really matter
>>
>
>  Actually, it doesn't work on windows since the additional dependency on
>> WebKit
>> and TestRunner introduce symbols that collide with test-only stuff from
>> the
>>
> net
>
>> library :-/
>>
>
> do you have a link to the try run or a copy of the build output? i.e. why
> does
> it not fail when it's a separate target. I'd like to see if there's a
> workaround
> we can do.
>
>
https://codereview.chromium.**org/11362161/<https://codereview.chromium.org/1...
>

[jam, 2012-11-09 17:26:05]

On 2012/11/09 17:18:54, jochen wrote:
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
> 
> It's a collision on TestRunner

Seems like that code should be in the net namespace per our convention.