Implement WebKit::WebUnitTestSupport::createLayerTreeViewForTesting()

Implement WebKit::WebUnitTestSupport::createLayerTreeViewForTesting()

This provides an implementation WebKit::WebUnitTestSupport::createLayerTreeViewForTesting()
for webkit_unit_tests that want to instantiate compositing support and need initialization
to succeed but do not need to actually render anything. Critically, this sort of
view does not depend on any particular client or settings.

This appears to be a lot of code, but it's actually a bit of movement and very
little new code.  The primary movement is extracting most of the test class
cc/test/fake_web_graphics_context3d.h into cc/fake_web_graphics_context3d.h so
that it can be exported to other modules as a regular class. This is necessary
since webkit_unit_tests are in a different component and must use only the
normally exported parts of the cc library.  cc/test/test_web_graphics_context3d.h
has all of the extra bells and whistles and assertions for cc_unittests.

WebLayerTreeViewImplForTesting uses a very simple context and does not depend
on an external client at all.  This results in two subtle but very significant
properties (which are the point of this whole exercise) - the unit tests no longer
depend on WebLayerTreeViewClient or WebGraphicsContext3D at all.  The entire
notion of what the context type is now entirely hidden in the chromium side of
the codebase and can be swapped out for something else (like gpu::GLES2Interface)
without any further changes on the WebKit side.

After this patch, the only remaining caller of WebCompositorSupport::createLayerTreeView
in the WebKit tree is WebViewHost, which needs to instantiate a view that can
render pixels (using mesa) for DumpRenderTree.  I'll address that separately.

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

[jamesr, 2013-02-11 07:56:33]

This patch looks big but really isn't - see the description for a rough idea of
what's moving.  It's only touching unit test code and code used to support unit
tests so it shouldn't cause any merge or stability issues.

[danakj, 2013-02-11 18:35:16]

What if we left FakeWGC3D in cc/test and made TestWGC3D in cc/ instead? Then all
the cc tests would not need to change, and we'd have consistent "Fake" named
classes for them. The TestWGC3D in cc/ would then also be very similar to the
TestWGC3D in ui/compositor for added points.

[danakj, 2013-02-11 18:43:02]

https://codereview.chromium.org/12211110/diff/7001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.h (right):

https://codereview.chromium.org/12211110/diff/7001/webkit/compositor_bindings...
webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.h:23: class
WebLayerTreeViewImplForTesting : public WebKit::WebLayerTreeView,
Since there are different types of tests (pixel tests are tests too!) maybe this
could be named more obviously that this will not generate any output pixels.
ForUnitTests? NullOutputForTesting?

[piman, 2013-02-11 19:53:00]

LGTM+nit. I leave Dana in charge of the naming bikeshedding :)
+1 if we can share FakeWGC3D and ui::TestWGC3D, but I'm ok if it's a follow-up
CL.

https://codereview.chromium.org/12211110/diff/7001/cc/test/test_web_graphics_...
File cc/test/test_web_graphics_context_3d.h (right):

https://codereview.chromium.org/12211110/diff/7001/cc/test/test_web_graphics_...
cc/test/test_web_graphics_context_3d.h:20: public
NON_EXPORTED_BASE(FakeWebGraphicsContext3D) {
nit: don't think NON_EXPORTED_BASE is useful since this class isn't exported...

[jamesr, 2013-02-11 21:08:28]

On 2013/02/11 18:35:16, danakj wrote:
> What if we left FakeWGC3D in cc/test and made TestWGC3D in cc/ instead? Then
all
> the cc tests would not need to change, and we'd have consistent "Fake" named
> classes for them. The TestWGC3D in cc/ would then also be very similar to the
> TestWGC3D in ui/compositor for added points.

FakeWGC3D is a no-op implementation.  It doesn't do anything, really.

TestWGC3D has state tracking and makes gtest assertions about various things. 
It has to be in a test binary only (since it links in gtest).

So I can't flip these implementations, but perhaps I could change the naming
around if that's what you are suggesting.

[jamesr, 2013-02-11 21:09:43]

On 2013/02/11 18:43:02, danakj wrote:
>
https://codereview.chromium.org/12211110/diff/7001/webkit/compositor_bindings...
> File webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.h
(right):
> 
>
https://codereview.chromium.org/12211110/diff/7001/webkit/compositor_bindings...
> webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.h:23: class
> WebLayerTreeViewImplForTesting : public WebKit::WebLayerTreeView,
> Since there are different types of tests (pixel tests are tests too!) maybe
this
> could be named more obviously that this will not generate any output pixels.
> ForUnitTests? NullOutputForTesting?

I also need some way to provide a view for layout tests.  Since this is on
WebUnitTestSupport, I think the "for testing" bit is obvious but the intended
backend probably isn't.  What would you think about having a parameter of type
enum { ForUnitTests, ForLayoutTests } that determines the type of WGC3D created?

[jamesr, 2013-02-11 21:10:15]

On 2013/02/11 19:53:00, piman wrote:
> File cc/test/test_web_graphics_context_3d.h (right):
> 
>
https://codereview.chromium.org/12211110/diff/7001/cc/test/test_web_graphics_...
> cc/test/test_web_graphics_context_3d.h:20: public
> NON_EXPORTED_BASE(FakeWebGraphicsContext3D) {
> nit: don't think NON_EXPORTED_BASE is useful since this class isn't
exported...


Hmm, ok.  To be honest I still have no idea what NON_EXPORTED_BASE means.

[danakj, 2013-02-11 21:11:41]

On Mon, Feb 11, 2013 at 1:09 PM, <jamesr@chromium.org> wrote:

> On 2013/02/11 18:43:02, danakj wrote:
>
> https://codereview.chromium.**org/12211110/diff/7001/webkit/**
>
compositor_bindings/web_layer_**tree_view_impl_for_testing.h<https://codereview.chromium.org/12211110/diff/7001/webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.h>
>
>> File webkit/compositor_bindings/**web_layer_tree_view_impl_for_**
>> testing.h
>>
> (right):
>
>
> https://codereview.chromium.**org/12211110/diff/7001/webkit/**
>
compositor_bindings/web_layer_**tree_view_impl_for_testing.h#**newcode23<https://codereview.chromium.org/12211110/diff/7001/webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.h#newcode23>
>
>> webkit/compositor_bindings/**web_layer_tree_view_impl_for_**testing.h:23:
>> class
>> WebLayerTreeViewImplForTesting : public WebKit::WebLayerTreeView,
>> Since there are different types of tests (pixel tests are tests too!)
>> maybe
>>
> this
>
>> could be named more obviously that this will not generate any output
>> pixels.
>> ForUnitTests? NullOutputForTesting?
>>
>
> I also need some way to provide a view for layout tests.  Since this is on
> WebUnitTestSupport, I think the "for testing" bit is obvious but the
> intended
> backend probably isn't.  What would you think about having a parameter of
> type
> enum { ForUnitTests, ForLayoutTests } that determines the type of WGC3D
> created?
>

SGTM

[danakj, 2013-02-11 21:12:25]

On Mon, Feb 11, 2013 at 1:08 PM, <jamesr@chromium.org> wrote:

> On 2013/02/11 18:35:16, danakj wrote:
>
>> What if we left FakeWGC3D in cc/test and made TestWGC3D in cc/ instead?
>> Then
>>
> all
>
>> the cc tests would not need to change, and we'd have consistent "Fake"
>> named
>> classes for them. The TestWGC3D in cc/ would then also be very similar to
>> the
>> TestWGC3D in ui/compositor for added points.
>>
>
> FakeWGC3D is a no-op implementation.  It doesn't do anything, really.
>
> TestWGC3D has state tracking and makes gtest assertions about various
> things.
> It has to be in a test binary only (since it links in gtest).
>
> So I can't flip these implementations, but perhaps I could change the
> naming
> around if that's what you are suggesting.
>

Ya that's what I meant. cc/ code doesn't need to be renamed at all, just a
new class added. And then other code could use cc::TestWGC3D instead of
ui::TestWGC3D and also need minimal renaming.

[jamesr1, 2013-02-11 21:14:16]

On Mon, Feb 11, 2013 at 1:11 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Mon, Feb 11, 2013 at 1:08 PM, <jamesr@chromium.org> wrote:
>
>> On 2013/02/11 18:35:16, danakj wrote:
>>
>>> What if we left FakeWGC3D in cc/test and made TestWGC3D in cc/ instead?
>>> Then
>>>
>> all
>>
>>> the cc tests would not need to change, and we'd have consistent "Fake"
>>> named
>>> classes for them. The TestWGC3D in cc/ would then also be very similar
>>> to the
>>> TestWGC3D in ui/compositor for added points.
>>>
>>
>> FakeWGC3D is a no-op implementation.  It doesn't do anything, really.
>>
>> TestWGC3D has state tracking and makes gtest assertions about various
>> things.
>> It has to be in a test binary only (since it links in gtest).
>>
>> So I can't flip these implementations, but perhaps I could change the
>> naming
>> around if that's what you are suggesting.
>>
>
> Ya that's what I meant. cc/ code doesn't need to be renamed at all, just a
> new class added. And then other code could use cc::TestWGC3D instead of
> ui::TestWGC3D and also need minimal renaming.
>

Gotcha.  Mind if I tackle the ui:: bits in a follow-up?  This patch has
already gone a bit epic.

[danakj, 2013-02-11 21:15:23]

On Mon, Feb 11, 2013 at 1:14 PM, James Robinson <jamesr@google.com> wrote:

>
>
> On Mon, Feb 11, 2013 at 1:11 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Mon, Feb 11, 2013 at 1:08 PM, <jamesr@chromium.org> wrote:
>>
>>> On 2013/02/11 18:35:16, danakj wrote:
>>>
>>>> What if we left FakeWGC3D in cc/test and made TestWGC3D in cc/ instead?
>>>> Then
>>>>
>>> all
>>>
>>>> the cc tests would not need to change, and we'd have consistent "Fake"
>>>> named
>>>> classes for them. The TestWGC3D in cc/ would then also be very similar
>>>> to the
>>>> TestWGC3D in ui/compositor for added points.
>>>>
>>>
>>> FakeWGC3D is a no-op implementation.  It doesn't do anything, really.
>>>
>>> TestWGC3D has state tracking and makes gtest assertions about various
>>> things.
>>> It has to be in a test binary only (since it links in gtest).
>>>
>>> So I can't flip these implementations, but perhaps I could change the
>>> naming
>>> around if that's what you are suggesting.
>>>
>>
>> Ya that's what I meant. cc/ code doesn't need to be renamed at all, just
>> a new class added. And then other code could use cc::TestWGC3D instead of
>> ui::TestWGC3D and also need minimal renaming.
>>
>
> Gotcha.  Mind if I tackle the ui:: bits in a follow-up?  This patch has
> already gone a bit epic.


Absolutely.

[piman, 2013-02-11 21:51:20]

On Mon, Feb 11, 2013 at 1:10 PM, <jamesr@chromium.org> wrote:

> On 2013/02/11 19:53:00, piman wrote:
>
>> File cc/test/test_web_graphics_**context_3d.h (right):
>>
>
>
> https://codereview.chromium.**org/12211110/diff/7001/cc/**
>
test/test_web_graphics_**context_3d.h#newcode20<https://codereview.chromium.org/12211110/diff/7001/cc/test/test_web_graphics_context_3d.h#newcode20>
>
>> cc/test/test_web_graphics_**context_3d.h:20: public
>> NON_EXPORTED_BASE(**FakeWebGraphicsContext3D) {
>> nit: don't think NON_EXPORTED_BASE is useful since this class isn't
>>
> exported...
>
>
> Hmm, ok.  To be honest I still have no idea what NON_EXPORTED_BASE means.
>

It's to workaround a MSVC warning.
If Derived derives from Base, and Derived is exported but Base isn't, it
warns. It's legit if Base is in the same component and not exported, but if
it's exported from another component, it's not legit even though it warns.
In your case, Derived isn't exported, so NON_EXPORTED_BASE isn't useful.


>
https://codereview.chromium.**org/12211110/<https://codereview.chromium.org/1...
>