Bug 84311 – [chromium] Use TextureLayerChromium for WebGL content instead of a dedicated layer type

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Use TextureLayerChromium for WebGL content instead of a dedicated layer type

[James Robinson]

Created attachment 137829 [details]
Patch

[James Robinson]

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.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adrienne Walker]

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?

[James Robinson]

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

[James Robinson]

Created attachment 138477 [details]
everything but the new unit test

[James Robinson]

Created attachment 138479 [details]
unit test for alpha attribute propagating to layer

[Adrienne Walker]

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.

[James Robinson]

Committed r114983: <http://trac.webkit.org/changeset/114983>