Bug 65791 – [chromium] Make WebViewImpl point at CCLayerTreeHost and related separation

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Make WebViewImpl point at CCLayerTreeHost and related separation

[Nat Duca]

Created attachment 103113 [details]
Patch

[WebKit Review Bot]

Comment on attachment 103113 [details]
Patch

Attachment 103113 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9324055

New failing tests:
fast/canvas/webgl/premultiplyalpha-test.html
fast/canvas/webgl/program-test.html
fast/canvas/webgl/index-validation.html
fast/canvas/webgl/index-validation-with-resized-buffer.html
fast/canvas/webgl/index-validation-copies-indices.html
fast/canvas/webgl/tex-image-and-sub-image-2d-with-array-buffer-view.html
fast/canvas/webgl/index-validation-verifies-too-many-indices.html
fast/canvas/webgl/copy-tex-image-and-sub-image-2d.html
fast/canvas/webgl/context-lost-restored.html
fast/canvas/webgl/gl-pixelstorei.html
fast/canvas/webgl/gl-bind-attrib-location-test.html
fast/canvas/webgl/canvas-test.html
fast/canvas/webgl/css-webkit-canvas-repaint.html
fast/canvas/webgl/css-webkit-canvas.html
fast/canvas/webgl/canvas-zero-size.html
fast/canvas/webgl/context-attributes-alpha-depth-stencil-antialias.html
fast/canvas/webgl/gl-vertex-attrib-zero-issues.html
fast/canvas/webgl/invalid-passed-params.html
fast/canvas/webgl/read-pixels-pack-alignment.html
fast/canvas/webgl/gl-teximage.html

[WebKit Review Bot]

Comment on attachment 103113 [details]
Patch

Attachment 103113 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9305990

[Nat Duca]

Created attachment 103303 [details]
All tests should pass.

[WebKit Review Bot]

Comment on attachment 103303 [details]
All tests should pass.

Attachment 103303 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9335191

[Nat Duca]

Created attachment 103368 [details]
Pass mac ews

[Vangelis Kokkevis]

Comment on attachment 103368 [details]
Pass mac ews

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

The separation looks nice and clean!  I like where this is going.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:56
> +class CCLayerTreeHost;

You're not using CCLayerTreeHost yet so not necessary

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:153
> +    // TOOD, replace with CCLayerTreeHost

TOOD -> FIXME

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:470
> +

nit: one too many newlines

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:489
> +bool LayerRendererChromium::contextSupportsAcceleratedPainting(GraphicsContext3D* context)

Since this method is only called by the CCLayerTreeHost, why not move it up there?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:200
> +    // For this rev, the LRC is owned by the CCLayerTreeHost. This will change in a following revision.

FIXME:

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:206
> +    // TODO: split the texture updater and tiler into two parts. Then,

TODO: -> FIXME:

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:207
> +    // keep the impl here and put the painting-side on the LayerTreeHost

Period '.' missing at the end of the comment.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:42
> +#include "cc/CCLayerTreeHost.h"

Is this include necessary?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:105
> +    // TODO: only called in threading case, fix when possible.

TODO -> FIXME

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:123
> +    // TODO: need to implement this

TODO: -> FIXME:

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:138
> +    // TODO: need to implement this

TODO -> FIXME

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:243
> +    // GraphicsContext3D::create might fail and return 0, in that case fall back to softwaree

typo: software

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:250
> +    // Check that the context supports accelerated drawing, if needed.

Ideally ::initialize and ::reallocateRender should share this bit of code to eliminate code duplication.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:29
> +#include "Extensions3D.h"

It's not clear to me whether changes to CCLayerTreeHostImpl.cpp and the proxy are required for the CL.

> Source/WebKit/chromium/src/WebSettingsImpl.cpp:50
> +    , m_acceleratedDrawingEnabled(false)

Why are we switching to a new member instead of using the existing one in Settings ?

[Nat Duca]

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:29
> > +#include "Extensions3D.h"
> 
> It's not clear to me whether changes to CCLayerTreeHostImpl.cpp and the proxy are required for the CL.

Ideally, I'd make zero changes. But, the removal of the LayerRendererChromiumImpl from the previous version then causes the CCLayerTreeHostImpl to start breaking. This is sort of "temporary fixyness" to get things compiling enough to move on to the next patch set.

[Nat Duca]

Created attachment 103548 [details]
Patch

[James Robinson]

Comment on attachment 103548 [details]
Patch

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

This is looking really great.  Left a bunch of nits, but besides a bunch of style-type things I think we're ready to rock and roll.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:141
> +    // TODO: This setting is currently ignored.

FIXME is preferred in WebKit style

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:152
> +    // TOOD, replace with CCLayerTreeHost

TOOD->TODO, or even better FIXME

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:595
> +    // FIXME: The multithreaded compositor case will not work as long as
> +    // copyTexImage2D resolves to the parent texture, because the main
> +    // thread can execute WebGL calls on the child context at any time,
> +    // potentially clobbering the parent texture that is being renderered
> +    // by the compositor thread.

I'm pretty sure you can nuke this FIXME now that Al fixed the command buffer's glFlush() semantics.  This is all good now.

I also don't think we really need to flush() all child contexts any more since we do a flush() on the appropriate context in updateCompositorResources().  That's probably for another patch, tho.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:620
> +        // TODOD: might have to call

TODOD->FIXME.  I'm pretty sure you don't need the call, though, the root layer render surface will get blown away on the next draw anyway.  So maybe just delete the comment completely.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:195
> +

nit: extra newline

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:270
> +#endif

nit: would you mind adding a comment to this #endif saying what it was for? something like:
#endif // !USE(THREADED_COMPOSITING)
would be sufficient

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:58
> +struct CCSettings {

could you add a no-arg constructor that initializes the bools to something reasonable (maybe all false), just to be sure that we don't allocate one of these, forget to set a bool, and have it be some random value? it can still be a struct

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:116
> +    explicit CCLayerTreeHost(CCLayerTreeHostClient*, const CCSettings&);

nit: no 'explicit' for 2-arg c'tors, only for 1-arg c'tors

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:121
> +    PassRefPtr<LayerRendererChromium> createRenderer();

although it's quite verbose, would you mind naming this createLayerRenderer()?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:68
> +    explicit CCLayerTreeHostImpl(CCLayerTreeHostImplClient*, PassRefPtr<LayerRendererChromium>);

nit: no 'explicit' for 2-arg constructors, just 1-arg

> Source/WebKit/chromium/src/WebViewImpl.cpp:76
> +#include "LayerChromium.h"

do we need this #include? it's a surprising dependency

> Source/WebKit/chromium/src/WebViewImpl.cpp:343
> +    , m_layerTreeHost(0)

WTF smart pointers (RefPtr<>, OwnPtr<>, etc) will null-initialize themselves, no need to do so yourself.

[Nat Duca]

Created attachment 103689 [details]
Cleanup

[James Robinson]

Comment on attachment 103689 [details]
Cleanup

Boogie

[Nat Duca]

Committed r92895: <http://trac.webkit.org/changeset/92895>