Bug 74893 – [chromium] Make sure rootDamageRect gets passed to renderer

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

In the process of refactoring, the root damage rect got "disconnected" from where it is used to perform the partial swap.  The fix is very straightforward, and works fine.  However, I would like to add a unit test to make sure this does not happen again.  An effective unit test needs to mock the "getExtensions()" accessor, so we can pretend that partial swap is supported.  Currently getExtensions() is implemented as non-virtual in un-mockable GraphicsContext3DPrivate.

Is it OK to move the implementation of getExtensions() from GraphicsContext3DPrivate into WebGraphicsContext3D, where we can make it virtual and mock-able?  or is there some reason not to do this?

[James Robinson]

Extensions3DChromium simply thunks to WebGraphicsContext3D for all of its operations and WebGraphicsContext3D is fully virtual and easy to mock. I don't understand why you would need more. Is there some scenario that you can't mock out today?

[Shawn Singh]

Thanks, it works exactly as you suggested.  Will post a patch soon.

[James Robinson]

OK cool, glad that worked.

If it helps shuffling around all of the Platform headers should make it pretty easy for us to cut out at least one layer of indirection in our GraphicsContext3D->WebGraphicsContext3D mappings.

[Shawn Singh]

Created attachment 119972 [details]
Patch

This patch has (1) the fix itself (2) appropriate unit test (3) drive-by style fix on ordering of include files.

[James Robinson]

Comment on attachment 119972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119972&action=review

haven't looked through full patch, just some drive by nits for now. will try to look at this later

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:108
> +    void swapBuffers(IntRect subBuffer);

we typically pass IntRects by const ref, can we do that here?

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:311
> +    FakeDrawableCCLayerImpl(int id) : CCLayerImpl(id) { }

nit: explicit

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:337
> +class PartialSwapTrackerContext: public MockWebGraphicsContext3D {

nit: space between PartialSwapTrackerContext and the :

[Shawn Singh]

Created attachment 119981 [details]
addressed nits and added one more case to unit test

[Shawn Singh]

Created attachment 120960 [details]
updated to close to tip of tree

[James Robinson]

Comment on attachment 120960 [details]
updated to close to tip of tree

R=me

[WebKit Review Bot]

Comment on attachment 120960 [details]
updated to close to tip of tree

Clearing flags on attachment: 120960

Committed r103988: <http://trac.webkit.org/changeset/103988>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.