Bug 84311 - [chromium] Use TextureLayerChromium for WebGL content instead of a dedicated layer type
Summary: [chromium] Use TextureLayerChromium for WebGL content instead of a dedicated ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-18 20:03 PDT by James Robinson
Modified: 2012-04-23 19:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch (32.11 KB, patch)
2012-04-18 20:15 PDT, James Robinson
no flags Details | Formatted Diff | Diff
everything but the new unit test (38.98 KB, patch)
2012-04-23 17:50 PDT, James Robinson
no flags Details | Formatted Diff | Diff
unit test for alpha attribute propagating to layer (42.46 KB, patch)
2012-04-23 17:59 PDT, James Robinson
enne: review+
enne: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-04-18 20:03:27 PDT
[chromium] Use TextureLayerChromium for WebGL content instead of a dedicated layer type
Comment 1 James Robinson 2012-04-18 20:15:43 PDT
Created attachment 137829 [details]
Patch
Comment 2 James Robinson 2012-04-18 20:17:02 PDT
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.
Comment 3 WebKit Review Bot 2012-04-18 20:18:47 PDT
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 4 Adrienne Walker 2012-04-23 14:45:00 PDT
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 5 James Robinson 2012-04-23 17:48:46 PDT
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
Comment 6 James Robinson 2012-04-23 17:50:44 PDT
Created attachment 138477 [details]
everything but the new unit test
Comment 7 James Robinson 2012-04-23 17:59:57 PDT
Created attachment 138479 [details]
unit test for alpha attribute propagating to layer
Comment 8 Adrienne Walker 2012-04-23 18:39:12 PDT
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.
Comment 9 James Robinson 2012-04-23 19:01:03 PDT
Committed r114983: <http://trac.webkit.org/changeset/114983>