[chromium] Software compositor initialization and base classes
Created attachment 144978 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 144978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144978&action=review nice! > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:126 > + if (!context3D) { > + // ASSERT(false); How come the assert is commented out here, and in a number of other places? Also, ASSERT(false) is generally done via ASSERT_NOT_REACHED() I think, or assert on opposite thing as in the if, before doing the if()return.
Comment on attachment 144978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144978&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:79 > +class CCRenderer { Please put this in a separate file. Same with the definition. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:99 > + const FloatQuad& sharedGeometryQuad() const { return m_sharedGeometryQuad; } Does this belong in the base class? It seems like a GL implementation detail. Are you really planning to use this for software mode? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:122 > + static void toGLMatrix(float*, const TransformationMatrix&); Same with this. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:132 > + virtual void drawTexturedQuad(const TransformationMatrix& layerMatrix, > + float width, float height, float opacity, const FloatQuad&, > + int matrixLocation, int alphaLocation, int quadLocation) = 0; Same with this. > Source/WebCore/platform/graphics/chromium/LayerTextureSubImage.cpp:95 > + if (!context3D) { > + // ASSERT(false); > + return; > + } I'm guessing that these are just early outs so that you can incrementally get software compositing working. If so, can you remove the commented assert and instead add a "FIXME: enable this path for software compositing" or some such, here and in a number of other places in the code?
Comment on attachment 144978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144978&action=review This does end up being pretty nice, I think it'll definitely work out. Some things need addressing of the more nitpicky variety. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:252 > + PassRefPtr<CCGraphicsContext> context, Why does LRC need a CCGraphicsContext? It seems to me like it'd make more sense for whoever constructs a LRC to have already extracted the GraphicsContext3D and pass that in, since they've clearly already decided to use the GC3D-specific CCRenderer implementation. I don't think we should need any changes in the implementation of any LayerRendererChromium functions due to CCGraphicsContext >> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:126 >> + // ASSERT(false); > > How come the assert is commented out here, and in a number of other places? > > Also, ASSERT(false) is generally done via ASSERT_NOT_REACHED() I think, or assert on opposite thing as in the if, before doing the if()return. What Dana says is truth. Also, normally we just ASSERT() our invariants and keep going, we don't add a release mode branch for it if we expect it to be a "can never happen" scenario. If we want to make sure we explicitly crash instead of potentially doing weird things we add CRASH() instead of just return, but that's rare > Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:32 > +#if USE(ACCELERATED_COMPOSITING) I'd not bother with this guard - we're in the chromium compositor directory, so obviously we have accelerated_compositing enabled. We can't even compile the chromium port these days without USE(ACCELERATED_COMPOSITING) and I don't think we will ever want to, so having the guard is just line noise > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:552 > + if (!context3d) { > + ASSERT(false); > + return false; same comments here RE assert()s
Created attachment 145234 [details] Patch
All done. > Why does LRC need a CCGraphicsContext? It seems to me like it'd make more sense for whoever constructs a LRC to have already extracted the GraphicsContext3D and pass that in, since they've clearly already decided to use the GC3D-specific CCRenderer implementation. I don't think we should need any changes in the implementation of any LayerRendererChromium functions due to CCGraphicsContext I needed it because some holders of CCRenderer assumed they could get a CCGraphicsContext from it. But there weren't too many, mainly CCLayerImpl::willDraw() overriders and CCLayerTreeHostImpl. I changed to keep around an extra CCGraphicsContext ref in CCLayerTreeHostImpl instead. > > >> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:126 > >> + // ASSERT(false); > > > > How come the assert is commented out here, and in a number of other places? > > > > Also, ASSERT(false) is generally done via ASSERT_NOT_REACHED() I think, or assert on opposite thing as in the if, before doing the if()return. > > What Dana says is truth. Also, normally we just ASSERT() our invariants and keep going, we don't add a release mode branch for it if we expect it to be a "can never happen" scenario. If we want to make sure we explicitly crash instead of potentially doing weird things we add CRASH() instead of just return, but that's rare OK, I added an early return with a FIXME in all the places I had that, except I put a CRASH() in WebViewImpl createContext functions, because I want to be alerted if some random component tries to create a context3D in software mode.
Created attachment 145363 [details] Patch
Rebased.
Comment on attachment 145363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145363&action=review R=me > Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:36 > +class CCGraphicsContext : public RefCounted<CCGraphicsContext> { As a follow-up, could you please make this class be single-ownership instead of refcounted?
Agreed, I'm somewhat uneasy about all the refcounting as well. I put that on my todo list.
Comment on attachment 145363 [details] Patch Rejecting attachment 145363 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.cpp:65: note: because the following virtual functions are pure within '<unnamed>::MockLayerTreeHostClient': Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:63: note: virtual WTF::PassRefPtr<WebCore::GraphicsContext3D> WebCore::CCLayerTreeHostClient::createContext3D() make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/12873400
Created attachment 145388 [details] Patch
Rebased again and resolved the conflict.
Comment on attachment 145388 [details] Patch Take 2
It looks like some layout tests are failing?
Comment on attachment 145388 [details] Patch Rejecting attachment 145388 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: FAILED at 69. 1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/chromium/tests/GraphicsLayerChromiumTest.cpp.rej patching file Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp patching file Source/WebKit/chromium/tests/TextureCopierTest.cpp patching file Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12859885
Created attachment 145426 [details] Patch
Comment on attachment 145426 [details] Patch Clearing flags on attachment: 145426 Committed r119313: <http://trac.webkit.org/changeset/119313>
All reviewed patches have been landed. Closing bug.