Bug 75732 – [chromium] Add WebSolidColorLayer interface to draw non-textured color layers from Aura.

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

[W. James MacLean]

[chromium] Add WebSolidColorLayer interface to draw non-textured color layers from Aura. [NOT FOR REVIEW]

[W. James MacLean]

Created attachment 121477 [details]
Patch

[W. James MacLean]

Comment on attachment 121477 [details]
Patch

Patch for discussion - do not commit.

[WebKit Review Bot]

Attachment 121477 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/src/WebSolidColorLayer.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[James Robinson]

Comment on attachment 121477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121477&action=review

> Source/WebCore/platform/graphics/chromium/SolidColorLayerChromium.h:44
> +class SolidColorLayerChromium : public LayerChromium {

so we're just using the type to indicate that we want to display the backgroundColor and nothing else? Do we really need a whole type for this or can we just use a LayerChromium with appropriate bits set?

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

I defer to James Robinson.

[W. James MacLean]

(In reply to comment #5)
> (From update of attachment 121477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121477&action=review
> 
> > Source/WebCore/platform/graphics/chromium/SolidColorLayerChromium.h:44
> > +class SolidColorLayerChromium : public LayerChromium {
> 
> so we're just using the type to indicate that we want to display the backgroundColor and nothing else? Do we really need a whole type for this or can we just use a LayerChromium with appropriate bits set?

I was hoping to get it to use a CCColidColorDrawQuad, since that would be textureless and thus lightweight in terms of memory and bandwidth. If this can be achieved with a LayerChromium directly, I'm all for making it simpler.

[James Robinson]

Right, I agree you want to end up there. I don't think you need a *LayerChromium side class to get that quad - that can be done by smarts on the Impl side.

[W. James MacLean]

(In reply to comment #8)
> Right, I agree you want to end up there. I don't think you need a *LayerChromium side class to get that quad - that can be done by smarts on the Impl side.

Sure ... let me take another look and figure out how it would work.

[W. James MacLean]

(In reply to comment #8)
> Right, I agree you want to end up there. I don't think you need a *LayerChromium side class to get that quad - that can be done by smarts on the Impl side.

Here are some alternatives (Enne, please jump in if I'm mis-representing anything here):

1) Add a setImplLayer(PasRef<CCLayerImpl>) function to allow a user to change the impl layer associated with any given LayerChromium.

In this case we could use the proposed CCSolidLayerImpl attached to a LayerChromium to implement the solid colour layer.

2) Have CCLayerImpl default to using a CCSolidColorDrawQuad in appendQuads() instead of a CCCustomLayerDrawQuad. The latter is only used by derived classes of CCLayerImpl, so this would force these derived classes to override appendQuads() to specify that that want to use CCCustomDrawQuad (they already have to override CCLayerImpl::draw()).

[Adrienne Walker]

(In reply to comment #10)
> (In reply to comment #8)
> > Right, I agree you want to end up there. I don't think you need a *LayerChromium side class to get that quad - that can be done by smarts on the Impl side.
> 
> Here are some alternatives (Enne, please jump in if I'm mis-representing anything here):
> 
> 1) Add a setImplLayer(PasRef<CCLayerImpl>) function to allow a user to change the impl layer associated with any given LayerChromium.

Maybe I'm misunderstanding, but I'm not sure it could work that way.  LayerChromium-derived classes create impl layers on demand.
 
> 2) Have CCLayerImpl default to using a CCSolidColorDrawQuad in appendQuads() instead of a CCCustomLayerDrawQuad. The latter is only used by derived classes of CCLayerImpl, so this would force these derived classes to override appendQuads() to specify that that want to use CCCustomDrawQuad (they already have to override CCLayerImpl::draw()).

Yeah, I think this is what I was suggesting, if you go the "LayerChromium implicitly is a SolidColorLayerChromium" route.


For what it's worth, I think I slightly prefer the original more explicit route, where raw LayerChromium/CCLayerImpl layers remain merely intermediate transform/clipping layers and never draw any content themselves.  If somebody was looking at the codebase and wanted to draw a solid color layer, I'm not sure that they'd have any idea where to look if it was implicit.

I don't really feel that strongly about it, though; #2 above seems quite reasonable and avoids a lot of boilerplate.

[W. James MacLean]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > Right, I agree you want to end up there. I don't think you need a *LayerChromium side class to get that quad - that can be done by smarts on the Impl side.
> > 
> > Here are some alternatives (Enne, please jump in if I'm mis-representing anything here):
> > 
> > 1) Add a setImplLayer(PasRef<CCLayerImpl>) function to allow a user to change the impl layer associated with any given LayerChromium.
> 
> Maybe I'm misunderstanding, but I'm not sure it could work that way.  LayerChromium-derived classes create impl layers on demand.

Yeah, that's right ... I suppose we could pass a function pointer to

static PassRefPtr<CCSolidColorLayerImpl> create(int id)

except you'd really then prefer it to be

static PassRefPtr<CCLayerImpl> create(int id)

so there was just one pointer type, but then that conflicts with the fact that all the create functions return RefPtrs to their specific types, so that doesn't quite help ... bah!


> > 2) Have CCLayerImpl default to using a CCSolidColorDrawQuad in appendQuads() instead of a CCCustomLayerDrawQuad. The latter is only used by derived classes of CCLayerImpl, so this would force these derived classes to override appendQuads() to specify that that want to use CCCustomDrawQuad (they already have to override CCLayerImpl::draw()).
> 
> Yeah, I think this is what I was suggesting, if you go the "LayerChromium implicitly is a SolidColorLayerChromium" route.
> 
> 
> For what it's worth, I think I slightly prefer the original more explicit route, where raw LayerChromium/CCLayerImpl layers remain merely intermediate transform/clipping layers and never draw any content themselves.  If somebody was looking at the codebase and wanted to draw a solid color layer, I'm not sure that they'd have any idea where to look if it was implicit.

I'm also OK with this, if that's the preferred route.

> I don't really feel that strongly about it, though; #2 above seems quite reasonable and avoids a lot of boilerplate.

Yes. It has one more wrinkle, in that Aura currently draws the tab contents via PluginLayerChromium, which directly uses CCLayerImpl, so if we do #2 we'd have to write a new impl-side class stucture to display the tab contents ...

So the original route looks even better in this light.

[W. James MacLean]

Created attachment 123145 [details]
Patch

[W. James MacLean]

New patch. This patch breaks the solid color layer into multiple CCSolidColorDrawQuads so that we can rely on the culler to reduce the number of pixels drawn.

I'm unsure as to what would constitute a reasonable set of tests, and would appreciate input on this.

[James Robinson]

Comment on attachment 123145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123145&action=review

> Source/WebCore/platform/graphics/chromium/SolidColorLayerChromium.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

copyright headers throughout this patch are inconsistent. for all new files they should say 2012 and be 2-clause, not 3-clause

> Source/WebCore/platform/graphics/chromium/SolidColorLayerChromium.h:44
> +class SolidColorLayerChromium : public LayerChromium {

it's strange that there's no color parameter anywhere in this class. can you at least document how you specify the color (guessing it's by setBackgroundColor() on the base class)?

> Source/WebCore/platform/graphics/chromium/SolidColorLayerChromium.h:47
> +    static PassRefPtr<SolidColorLayerChromium> create(CCLayerDelegate*);

this class doesn't need a delegate

> Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:40
> +// This class represents a layer that renders a solid color that is generated
> +// externally (not managed by the WebLayerTreeView).
> +// When in single-thread mode, this means during WebLayerTreeView::composite().
> +// When using the threaded compositor, this can mean at an arbitrary time until
> +// the WebLayerTreeView is destroyed.

whaaaat? why is the lifecycle different for this layer type?

> Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:52
> +    WebSolidColorLayer() { } 
> +    WebSolidColorLayer(const WebSolidColorLayer& layer) : WebLayer(layer) { }
> +    virtual ~WebSolidColorLayer() { } 
> +    WebSolidColorLayer& operator=(const WebSolidColorLayer& layer)
> +    {
> +        WebLayer::assign(layer);
> +        return *this;
> +    }

do you need all these? why not just the create()?

> Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:54
> +    WEBKIT_EXPORT void setBackgroundColor(const WebColor&);

do we need to be able to change the color after the layer is constructed, or can it be a construction-time invariant?

> Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:60
> +#if WEBKIT_IMPLEMENTATION
> +    WebSolidColorLayer(const WTF::PassRefPtr<WebSolidColorLayerImpl>&);
> +    WebSolidColorLayer& operator=(const WTF::PassRefPtr<WebSolidColorLayerImpl>&);
> +    operator WTF::PassRefPtr<WebSolidColorLayerImpl>() const;
> +#endif

do you need these?

> Source/WebKit/chromium/src/WebSolidColorLayerImpl.cpp:51
> +void WebSolidColorLayerImpl::paintContents(GraphicsContext&, const IntRect&)
> +{
> +}

you don't need this

> Source/WebKit/chromium/src/WebSolidColorLayerImpl.h:34
> +class WebSolidColorLayerImpl : public WebCore::SolidColorLayerChromium, public WebCore::CCLayerDelegate {

no need to be a delegate

[Adrienne Walker]

(In reply to comment #14)
> New patch. This patch breaks the solid color layer into multiple CCSolidColorDrawQuads so that we can rely on the culler to reduce the number of pixels drawn.
> 
> I'm unsure as to what would constitute a reasonable set of tests, and would appreciate input on this.

You could do something like CCTiledLayerImplTest, where you make sure that all the generated quads exactly fill the visibleLayerRect and don't overlap.

[W. James MacLean]

Created attachment 123342 [details]
Patch

[W. James MacLean]

(In reply to comment #15)
> 
> it's strange that there's no color parameter anywhere in this class. can you at least document how you specify the color (guessing it's by setBackgroundColor() on the base class)?

Yes, setBackgroundColor() on the base class. Added documentation in form of comment at start of class.
 
> > Source/WebCore/platform/graphics/chromium/SolidColorLayerChromium.h:47
> > +    static PassRefPtr<SolidColorLayerChromium> create(CCLayerDelegate*);
> 
> this class doesn't need a delegate

Removed.

> > Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:40
> > +// This class represents a layer that renders a solid color that is generated
> > +// externally (not managed by the WebLayerTreeView).
> > +// When in single-thread mode, this means during WebLayerTreeView::composite().
> > +// When using the threaded compositor, this can mean at an arbitrary time until
> > +// the WebLayerTreeView is destroyed.
> 
> whaaaat? why is the lifecycle different for this layer type?

Sorry, out of date ... comments removed.

> > Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:52
> > +    WebSolidColorLayer() { } 
> > +    WebSolidColorLayer(const WebSolidColorLayer& layer) : WebLayer(layer) { }
> > +    virtual ~WebSolidColorLayer() { } 
> > +    WebSolidColorLayer& operator=(const WebSolidColorLayer& layer)
> > +    {
> > +        WebLayer::assign(layer);
> > +        return *this;
> > +    }
> 
> do you need all these? why not just the create()?

Removed.

> > Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:54
> > +    WEBKIT_EXPORT void setBackgroundColor(const WebColor&);
> 
> do we need to be able to change the color after the layer is constructed, or can it be a construction-time invariant?

I am not the end-user for this class, but it is my belief they will want to be able to change its color after creation.

> > Source/WebKit/chromium/public/platform/WebSolidColorLayer.h:60
> > +#if WEBKIT_IMPLEMENTATION
> > +    WebSolidColorLayer(const WTF::PassRefPtr<WebSolidColorLayerImpl>&);
> > +    WebSolidColorLayer& operator=(const WTF::PassRefPtr<WebSolidColorLayerImpl>&);
> > +    operator WTF::PassRefPtr<WebSolidColorLayerImpl>() const;
> > +#endif
> 
> do you need these?

Just the constructor with the impl class. Others removed.

> > Source/WebKit/chromium/src/WebSolidColorLayerImpl.cpp:51
> > +void WebSolidColorLayerImpl::paintContents(GraphicsContext&, const IntRect&)
> > +{
> > +}
> 
> you don't need this

Removed.

> > Source/WebKit/chromium/src/WebSolidColorLayerImpl.h:34
> > +class WebSolidColorLayerImpl : public WebCore::SolidColorLayerChromium, public WebCore::CCLayerDelegate {
> 
> no need to be a delegate

Removed.

[James Robinson]

Comment on attachment 123342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123342&action=review

Nearly there!

> Source/WebCore/platform/graphics/chromium/cc/CCSolidColorLayerImpl.cpp:43
> +    , m_tileSize(256)

why is this a member variable if it's always the same? if it's a constant, make it a constant

> Source/WebKit/chromium/tests/CCSolidColorLayerImplTest.cpp:42
> +// FIXME: The next three items have been borrowed from CCTileLayerImplTest.cpp,
> +// but have more general applicability - move them into a something like "CCLayerTestHelper.h/.cpp"?

could you just go ahead and do this?

> Source/WebKit/chromium/tests/CCSolidColorLayerImplTest.cpp:99
> +    layer->setBackgroundColor(0xFFA55AFF);

can you make this a named constant?

> Source/WebKit/chromium/tests/CCSolidColorLayerImplTest.cpp:106
> +    EXPECT_EQ(quadList.size(), 1U);
> +    if (!quadList.isEmpty())
> +        EXPECT_EQ(quadList[0]->toSolidColorDrawQuad()->color(), 0xFFA55AFF);

if you're just worried about running off the end of the list if the quadList doesn't have the expected size, then do this instead:

ASSERT_EQ(quadList.size(), 1u);
EXPECT_EQ(quadList[0]->....);

if the ASSERT_XXX() fails it'll immediately abort the test and not try to continue

[W. James MacLean]

(In reply to comment #19)
> (From update of attachment 123342 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123342&action=review
> 
> Nearly there!
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSolidColorLayerImpl.cpp:43
> > +    , m_tileSize(256)
> 
> why is this a member variable if it's always the same? if it's a constant, make it a constant

I guess I figured we might want to alter it at some point, but since that's not happening at present I'll take it out.

> > Source/WebKit/chromium/tests/CCSolidColorLayerImplTest.cpp:42
> > +// FIXME: The next three items have been borrowed from CCTileLayerImplTest.cpp,
> > +// but have more general applicability - move them into a something like "CCLayerTestHelper.h/.cpp"?
> 
> could you just go ahead and do this?

Will do.

> > Source/WebKit/chromium/tests/CCSolidColorLayerImplTest.cpp:99
> > +    layer->setBackgroundColor(0xFFA55AFF);
> 
> can you make this a named constant?

Will do.

> > Source/WebKit/chromium/tests/CCSolidColorLayerImplTest.cpp:106
> > +    EXPECT_EQ(quadList.size(), 1U);
> > +    if (!quadList.isEmpty())
> > +        EXPECT_EQ(quadList[0]->toSolidColorDrawQuad()->color(), 0xFFA55AFF);
> 
> if you're just worried about running off the end of the list if the quadList doesn't have the expected size, then do this instead:
> 
> ASSERT_EQ(quadList.size(), 1u);
> EXPECT_EQ(quadList[0]->....);
> 
> if the ASSERT_XXX() fails it'll immediately abort the test and not try to continue

Good to know, will revise.

New patch later in the weekend.

[W. James MacLean]

Created attachment 123561 [details]
Patch

[James Robinson]

Comment on attachment 123561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123561&action=review

R=me

> Source/WebCore/platform/graphics/chromium/cc/CCSolidColorLayerImpl.cpp:65
> +    for (int x = 0; x < width; x += m_tileSize)

add curly braces for this for loop as well since it's more than one line (in WebKit, braces are determined by the # of lines, not the # of statements)

[W. James MacLean]

Committed r105634: <http://trac.webkit.org/changeset/105634>