[chromium] Make WebViewImpl point at CCLayerTreeHost and related separation
Created attachment 103113 [details] Patch
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
Comment on attachment 103113 [details] Patch Attachment 103113 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9305990
Created attachment 103303 [details] All tests should pass.
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
Created attachment 103368 [details] Pass mac ews
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 ?
> > 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.
Created attachment 103548 [details] Patch
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.
Created attachment 103689 [details] Cleanup
Comment on attachment 103689 [details] Cleanup Boogie
Committed r92895: <http://trac.webkit.org/changeset/92895>