Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3056)

Issue 12211110: Implement WebKit::WebUnitTestSupport::createLayerTreeViewForTesting() (Closed)

Created:
7 years, 6 months ago by jamesr
Modified:
7 years, 6 months ago
CC:
chromium-reviews, cc-bugs_chromium.org, darin-cc_chromium.org, apatrick_chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : win component fixes #

Patch Set 3 : win component build fix for cc::FakeWebGraphicsContext3D #

Patch Set 4 : include #

Total comments: 2

Patch Set 5 : add enum, fix NON_EXPORTED_BASE #

Unified diffs Side-by-side diffs Delta from patch set Stats (+618 lines, -1537 lines) Patch
M cc/cc.gyp View 1 chunk +2 lines, -0 lines 0 comments Download
M cc/cc_tests.gyp View 2 chunks +2 lines, -2 lines 0 comments Download
M cc/delegated_renderer_layer_impl_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M cc/delegating_renderer_unittest.cc View 2 chunks +3 lines, -3 lines 0 comments Download
A + cc/fake_web_graphics_context_3d.h View 1 2 3 2 chunks +10 lines, -69 lines 0 comments Download
A + cc/fake_web_graphics_context_3d.cc View 8 chunks +17 lines, -142 lines 0 comments Download
M cc/gl_renderer_unittest.cc View 10 chunks +10 lines, -10 lines 0 comments Download
M cc/layer_tree_host_impl_unittest.cc View 12 chunks +16 lines, -16 lines 0 comments Download
M cc/layer_tree_host_unittest.cc View 4 chunks +4 lines, -4 lines 0 comments Download
M cc/layer_tree_host_unittest_context.cc View 8 chunks +11 lines, -11 lines 0 comments Download
M cc/resource_provider_unittest.cc View 6 chunks +6 lines, -6 lines 0 comments Download
M cc/resource_update_controller_unittest.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M cc/scrollbar_layer_unittest.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M cc/test/fake_graphics_context_3d_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M cc/test/fake_layer_tree_host_client.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M cc/test/fake_output_surface.h View 4 chunks +4 lines, -4 lines 0 comments Download
D cc/test/fake_web_graphics_context_3d.h View 1 chunk +0 lines, -648 lines 0 comments Download
D cc/test/fake_web_graphics_context_3d.cc View 1 chunk +0 lines, -424 lines 0 comments Download
M cc/test/fake_web_graphics_context_3d_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
A cc/test/test_web_graphics_context_3d.h View 1 2 3 4 1 chunk +151 lines, -0 lines 0 comments Download
A + cc/test/test_web_graphics_context_3d.cc View 10 chunks +50 lines, -160 lines 0 comments Download
M cc/texture_copier_unittest.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M cc/texture_uploader_unittest.cc View 5 chunks +6 lines, -6 lines 0 comments Download
M webkit/compositor_bindings/compositor_bindings.gyp View 1 chunk +2 lines, -0 lines 0 comments Download
M webkit/compositor_bindings/web_compositor_support_impl.h View 2 chunks +3 lines, -3 lines 0 comments Download
M webkit/compositor_bindings/web_compositor_support_impl.cc View 2 chunks +16 lines, -16 lines 0 comments Download
A webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.h View 1 chunk +84 lines, -0 lines 0 comments Download
A webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.cc View 1 chunk +187 lines, -0 lines 0 comments Download
M webkit/support/test_webkit_platform_support.h View 1 2 3 4 2 chunks +6 lines, -1 line 0 comments Download
M webkit/support/test_webkit_platform_support.cc View 1 2 3 4 2 chunks +15 lines, -0 lines 0 comments Download
M webkit/support/webkit_support.gypi View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
jamesr
This patch looks big but really isn't - see the description for a rough idea ...
7 years, 6 months ago (2013-02-11 07:56:33 UTC) #1
danakj
What if we left FakeWGC3D in cc/test and made TestWGC3D in cc/ instead? Then all ...
7 years, 6 months ago (2013-02-11 18:35:16 UTC) #2
danakj
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 webkit/compositor_bindings/web_layer_tree_view_impl_for_testing.h:23: class WebLayerTreeViewImplForTesting : public WebKit::WebLayerTreeView, Since there are different ...
7 years, 6 months ago (2013-02-11 18:43:02 UTC) #3
piman
LGTM+nit. I leave Dana in charge of the naming bikeshedding :) +1 if we can ...
7 years, 6 months ago (2013-02-11 19:53:00 UTC) #4
jamesr
On 2013/02/11 18:35:16, danakj wrote: > What if we left FakeWGC3D in cc/test and made ...
7 years, 6 months ago (2013-02-11 21:08:28 UTC) #5
jamesr
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 > 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 > ...
7 years, 6 months ago (2013-02-11 21:09:43 UTC) #6
jamesr
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 > cc/test/test_web_graphics_context_3d.h:20: public ...
7 years, 6 months ago (2013-02-11 21:10:15 UTC) #7
danakj
On Mon, Feb 11, 2013 at 1:09 PM, <jamesr@chromium.org> wrote: > On 2013/02/11 18:43:02, danakj ...
7 years, 6 months ago (2013-02-11 21:11:41 UTC) #8
danakj
On Mon, Feb 11, 2013 at 1:08 PM, <jamesr@chromium.org> wrote: > On 2013/02/11 18:35:16, danakj ...
7 years, 6 months ago (2013-02-11 21:12:25 UTC) #9
jamesr1
On Mon, Feb 11, 2013 at 1:11 PM, Dana Jansens <danakj@chromium.org> wrote: > On Mon, ...
7 years, 6 months ago (2013-02-11 21:14:16 UTC) #10
danakj
On Mon, Feb 11, 2013 at 1:14 PM, James Robinson <jamesr@google.com> wrote: > > > ...
7 years, 6 months ago (2013-02-11 21:15:23 UTC) #11
piman
7 years, 6 months ago (2013-02-11 21:51:20 UTC) #12
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...
>

Powered by Google App Engine
This is Rietveld 408576698