[chromium] Use TextureLayerChromium for WebGL content instead of a dedicated layer type
Created attachment 137829 [details] Patch
Note that with this patch CanvasLayerChromium is somewhat silly, since it's a only a base class for Canvas2DLayerChromium. I left this alone for two reasons: 1.) I'm going to port Canvas2DLayerChromium over to TextureLayerChromium soon anyway 2.) I don't want to cause collisions with other Canvas2DLayerChromium patches. I also plan to move up to depending on platform/WebExternalTextureLayerChromium soon, but I think going piece-by-piece is a better staging strategy.
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 137829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137829&action=review Looks good in general. Here's some assorted comments. > Source/WebCore/ChangeLog:14 > + This cuts down the compositor interface significantly and allows for more code use in exchange for code to make > + TextureLayerChromium more general. More code...*re*use? I'm not sure what you're trying to say here. > Source/WebCore/platform/graphics/chromium/TextureLayerChromium.h:39 > + virtual unsigned prepareTexture(CCTextureUpdater&) = 0; > + virtual GraphicsContext3D* context() = 0; Can you add comments for this API so it's clear what clients need to be doing to implement it? > Source/WebCore/platform/graphics/chromium/TextureLayerChromium.h:65 > + // Sets whether this texture has an alpha channel. Defaults to true. > + void setHasAlpha(bool); From an API perspective, how does hasAlpha() relate to opaque()? Could they be the same thing? > Source/WebCore/platform/graphics/gpu/DrawingBuffer.h:161 > + OwnPtr<DrawingBufferPrivate> m_private; (Huh. I thought you couldn't have a forward declared OwnPtr type. Maybe if the destructor knows the full type, it's ok?) > Source/WebKit/chromium/tests/WebGLLayerChromiumTest.cpp:-39 > -TEST(WebGLLayerChromiumTest, opaqueFormats) Is there some corresponding unit test you can write for DrawingBuffer setting up the right flags on TextureLayerChromium based on the GC3D attributes?
Comment on attachment 137829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137829&action=review >> Source/WebCore/ChangeLog:14 >> + TextureLayerChromium more general. > > More code...*re*use? I'm not sure what you're trying to say here. Me neither. Will take another crack at this sentence >> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.h:65 >> + void setHasAlpha(bool); > > From an API perspective, how does hasAlpha() relate to opaque()? Could they be the same thing? Yes, turns out it's redundant. setHasAlpha() is used to pick the blending mode, but if the caller sets premultipliedAlpha() correctly (which it does) that's sufficient to pick the blend mode. switched DrawingBuffer to using setOpaque() so we can disable blending completely if we don't need it >> Source/WebCore/platform/graphics/gpu/DrawingBuffer.h:161 >> + OwnPtr<DrawingBufferPrivate> m_private; > > (Huh. I thought you couldn't have a forward declared OwnPtr type. Maybe if the destructor knows the full type, it's ok?) (You can, so long as the c'tor and d'tor can see the real type. DrawingBuffer::DrawingBuffer() and DrawingBuffer::~DrawingBuffer() are in DrawingBufferChromium.cpp, so it's all good) >> Source/WebKit/chromium/tests/WebGLLayerChromiumTest.cpp:-39 >> -TEST(WebGLLayerChromiumTest, opaqueFormats) > > Is there some corresponding unit test you can write for DrawingBuffer setting up the right flags on TextureLayerChromium based on the GC3D attributes? I can take a swing. I doubt these provide much real value, though - it's pretty much testing a getter/setter
Created attachment 138477 [details] everything but the new unit test
Created attachment 138479 [details] unit test for alpha attribute propagating to layer
Comment on attachment 138479 [details] unit test for alpha attribute propagating to layer Other than maybe needing to be rebased, these changes look good to me. Thanks for removing the unnecessary hasAlpha; that really wormed its way through a lot of code.
Committed r114983: <http://trac.webkit.org/changeset/114983>