In order to improve the testability of Chromium's accelerated compositing code, as well as to re-enable multi-DLL builds on Windows, we need to change the compositor (LayerRendererChromium and associated code) to build on top of GraphicsContext3D instead of GLES2Context. Once this effort is complete, the GLES2Context and WebGLES2Context will be removed from the WebKit tree. This master bug will track the multiple required checkins.
Created attachment 68127 [details] Patch From the ChangeLog: Switched Chromium's compositor to use GraphicsContext3D to issue its OpenGL rendering calls rather than the Chromium-specific GLES2Context and command buffer OpenGL implementation. The in-process software rendering path for GraphicsContext3D does not yet work with the compositor, at least not on Mac OS X. This will be worked on in subsequent bugs. Tested manually with 3D CSS, WebGL and video content on Mac OS X and Linux. No new tests.
Attachment 68127 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/chromium/src/WebViewImpl.cpp:2447: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 68127 [details] Patch Looking pretty good overall, some nits. The style bot's nit is accurate. Not touching review? so the rest of the bots get a chance to chug on it. View in context: https://bugs.webkit.org/attachment.cgi?id=68127&action=prettypatch > WebCore/platform/graphics/chromium/CanvasLayerChromium.h:50 > + SharedValues(GraphicsContext3D*); explicit > WebCore/platform/graphics/chromium/ContentLayerChromium.h:58 > + SharedValues(GraphicsContext3D*); explicit > WebCore/platform/graphics/chromium/LayerChromium.h:174 > + SharedValues(GraphicsContext3D*); explicit > WebCore/platform/graphics/chromium/LayerRendererChromium.h:106 > + void noticeResize(const IntSize&); Name is odd, it's not really telling the LayerRendererChromium to do something. I liked resizeOnscreenContent(). > WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:178 > + void* mem = context->mapTexSubImage2DCHROMIUM(GraphicsContext3D::TEXTURE_2D, 0, updateRect.x(), updateRect.y(), updateRect.width(), updateRect.height(), GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, GraphicsContext3D::WRITE_ONLY); I see that you aren't changing behavior here, but could you file a bug on the video guys to check for a NULL return here and handle it? The mapTexSub..() extension may return null if it can't get a suitable shared memory segment.
Comment on attachment 68127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68127&action=review > WebKit/chromium/src/GraphicsContext3D.cpp:807 > + if (!internal->initialize(attrs, hostWindow, renderStyle == RenderDirectlyToHostWindow ? true : false)) { Nit: you can omit the ternary. A relational expression in C++ is type bool already. > WebKit/chromium/src/GraphicsContext3D.cpp:810 > + PassOwnPtr<GraphicsContext3D> result = new GraphicsContext3D(attrs, hostWindow, renderStyle == RenderDirectlyToHostWindow ? true : false); Same as above.
Created attachment 68142 [details] Revised patch Addressed above code review feedback.
(In reply to comment #3) > (From update of attachment 68127 [details]) > Looking pretty good overall, some nits. The style bot's nit is accurate. Revised patch fixes style error. > Not touching review? so the rest of the bots get a chance to chug on it. > > View in context: https://bugs.webkit.org/attachment.cgi?id=68127&action=prettypatch > > > WebCore/platform/graphics/chromium/CanvasLayerChromium.h:50 > > + SharedValues(GraphicsContext3D*); > > explicit Done. > > WebCore/platform/graphics/chromium/ContentLayerChromium.h:58 > > + SharedValues(GraphicsContext3D*); > > explicit Done. > > WebCore/platform/graphics/chromium/LayerChromium.h:174 > > + SharedValues(GraphicsContext3D*); > > explicit > > > WebCore/platform/graphics/chromium/LayerRendererChromium.h:106 > > + void noticeResize(const IntSize&); > > Name is odd, it's not really telling the LayerRendererChromium to do something. I liked resizeOnscreenContent(). That's fine; changed it to resizeOnscreenContent. > > WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:178 > > + void* mem = context->mapTexSubImage2DCHROMIUM(GraphicsContext3D::TEXTURE_2D, 0, updateRect.x(), updateRect.y(), updateRect.width(), updateRect.height(), GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, GraphicsContext3D::WRITE_ONLY); > > I see that you aren't changing behavior here, but could you file a bug on the video guys to check for a NULL return here and handle it? The mapTexSub..() extension may return null if it can't get a suitable shared memory segment. Their new patch at https://bugs.webkit.org/show_bug.cgi?id=45069 handles the NULL return value. It will be best if they can also handle the absence of the GL_CHROMIUM_map_sub extension, which I'll file after this lands.
(In reply to comment #4) > (From update of attachment 68127 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68127&action=review > > > WebKit/chromium/src/GraphicsContext3D.cpp:807 > > + if (!internal->initialize(attrs, hostWindow, renderStyle == RenderDirectlyToHostWindow ? true : false)) { > > Nit: you can omit the ternary. A relational expression in C++ is type bool already. Good point. Done. > > WebKit/chromium/src/GraphicsContext3D.cpp:810 > > + PassOwnPtr<GraphicsContext3D> result = new GraphicsContext3D(attrs, hostWindow, renderStyle == RenderDirectlyToHostWindow ? true : false); > > Same as above. Done.
Comment on attachment 68142 [details] Revised patch R=me View in context: https://bugs.webkit.org/attachment.cgi?id=68142&action=prettypatch > WebCore/ChangeLog:66 > + (WebCore::LayerRendererChromium::noticeResize): nit: changelog out of date, this function is called resizeOnscreenContent() now.
Committed r67885: <http://trac.webkit.org/changeset/67885>
(In reply to comment #8) > (From update of attachment 68142 [details]) > R=me > > View in context: https://bugs.webkit.org/attachment.cgi?id=68142&action=prettypatch > > > WebCore/ChangeLog:66 > > + (WebCore::LayerRendererChromium::noticeResize): > > nit: changelog out of date, this function is called resizeOnscreenContent() now. Fixed in landed patch.
Committed r67903: <http://trac.webkit.org/changeset/67903>
Looks like you had a mis-merged ChangeLog, Darin. Reopening this bug.
(In reply to comment #12) > Looks like you had a mis-merged ChangeLog, Darin. Reopening this bug. Yes. webkit-patch is not my friend :-(