Summary: | [chromium] Software compositor initialization and base classes | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandre Elias <aelias> | ||||||||||||
Component: | New Bugs | Assignee: | Alexandre Elias <aelias> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, aelias, cc-bugs, danakj, dglazkov, eric.carlson, feature-media-reviews, fishd, jamesr, sievers, tkent+wkapi, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 88132 | ||||||||||||||
Attachments: |
|
Description
Alexandre Elias
2012-05-30 20:31:52 PDT
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. |