Bug 87821 - [Chromium] Compositor doesn't support translucent root layers.
Summary: [Chromium] Compositor doesn't support translucent root layers.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-29 22:17 PDT by David Reveman
Modified: 2012-06-08 19:29 PDT (History)
9 users (show)

See Also:


Attachments
Patch (13.02 KB, patch)
2012-06-01 11:57 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (15.83 KB, patch)
2012-06-01 13:30 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (24.19 KB, patch)
2012-06-06 16:49 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (27.11 KB, patch)
2012-06-07 21:40 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (28.21 KB, patch)
2012-06-08 13:16 PDT, David Reveman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2012-05-29 22:17:30 PDT
Set contents opaqueness correctly for root layers and use opaque value of owning layer to determine how to clear a render surface in LayerRendererChromium.
Comment 1 David Reveman 2012-06-01 11:57:15 PDT
Created attachment 145348 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-01 11:58:54 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 3 vollick 2012-06-01 12:16:05 PDT
(In reply to comment #1)
> Created an attachment (id=145348) [details]
> Patch

Sorry for the drive by -- I was wondering if a translucent root should affect subpixel aa. I was under the impression that it could only be used on opaque layers, currently.
Comment 4 James Robinson 2012-06-01 12:22:48 PDT
If the root is translucent then we would have to turn subpixel AA off.

What's this for?
Comment 5 David Reveman 2012-06-01 12:28:29 PDT
(In reply to comment #3)
> (In reply to comment #1)
> > Created an attachment (id=145348) [details] [details]
> > Patch
> 
> Sorry for the drive by -- I was wondering if a translucent root should affect subpixel aa. I was under the impression that it could only be used on opaque layers, currently.

Yes, good point. We need to disable sub-pixel aa for translucent root layers. We're currently making the decision if sub-pixel aa can be used based on if the layer has border texels. We should add layer opaqueness as a factor to that decision.
Comment 6 David Reveman 2012-06-01 12:28:49 PDT
(In reply to comment #4)
> If the root is translucent then we would have to turn subpixel AA off.
> 
> What's this for?

http://code.google.com/p/chromium/issues/detail?id=129787
Comment 7 David Reveman 2012-06-01 13:30:22 PDT
Created attachment 145368 [details]
Patch

Turn off subpixel AA for non-opaque layers
Comment 8 James Robinson 2012-06-04 12:43:04 PDT
Comment on attachment 145368 [details]
Patch

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

Code looks reasonable but it could be much simpler.

This also needs tests of some form.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:396
> +    if (renderSurface != rootRenderSurface || !settings().opaqueRootLayer)

This is weird as a setting - why not just check if the root layer is opaque directly?

> Source/WebKit/chromium/public/WebSettings.h:158
> +    virtual void setOpaqueRootLayer(bool) = 0;

I don't think it makes sense for WebSettings to be talking about the root layer.  This is really about whether the WebView may be rendered translucently, right?

What's supposed to happen when this view isn't composited?
Comment 9 David Reveman 2012-06-04 13:54:18 PDT
(In reply to comment #8)
> (From update of attachment 145368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145368&action=review
> 
> Code looks reasonable but it could be much simpler.
> 
> This also needs tests of some form.

We could unit test it based on glClear being called. Does that seem reasonable?

> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:396
> > +    if (renderSurface != rootRenderSurface || !settings().opaqueRootLayer)
> 
> This is weird as a setting - why not just check if the root layer is opaque directly?

Eventually, I'd like to make this check be "if (renderSurface->isOpaque())" to avoid clearing all render surfaces with an opaque backing layer. But that's a change in behavior that I would prefer to land in a separate patch. The reason I'm not using the backing layer opaque value for the root render surface in this patch is that this requires changes to both webkit and chromium (ui compositor root layer) to ensure that we're not doing unnecessary clearing of the root layer.

> 
> > Source/WebKit/chromium/public/WebSettings.h:158
> > +    virtual void setOpaqueRootLayer(bool) = 0;
> 
> I don't think it makes sense for WebSettings to be talking about the root layer.  This is really about whether the WebView may be rendered translucently, right?

It's about allowing the contents to be translucent. What would be a better way to expose this?

> 
> What's supposed to happen when this view isn't composited?

It's not supposed to have any effect. Having this only supported with compositing is good enough for the current use case. I happens to work right now without compositing but that's not a requirement afaik.
Comment 10 James Robinson 2012-06-04 14:16:43 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 145368 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=145368&action=review
> > 
> > Code looks reasonable but it could be much simpler.
> > 
> > This also needs tests of some form.
> 
> We could unit test it based on glClear being called. Does that seem reasonable?
> 

Sounds reasonable, but in release builds we only issue a glClear on non-root layers (since it's just a debug aid, not for correctness) so I'm not sure how useful of a test you would end up with.  What would the test actually do?

> > 
> > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:396
> > > +    if (renderSurface != rootRenderSurface || !settings().opaqueRootLayer)
> > 
> > This is weird as a setting - why not just check if the root layer is opaque directly?
> 
> Eventually, I'd like to make this check be "if (renderSurface->isOpaque())" to avoid clearing all render surfaces with an opaque backing layer. But that's a change in behavior that I would prefer to land in a separate patch. The reason I'm not using the backing layer opaque value for the root render surface in this patch is that this requires changes to both webkit and chromium (ui compositor root layer) to ensure that we're not doing unnecessary clearing of the root layer.
> 

The only chromium-side change you would need would be to not mark the root layer as being opaque, but you have to set that anyway to render correctly - no?  This setting seems strictly redundant with with setOpaque() on the root layer.

> > 
> > > Source/WebKit/chromium/public/WebSettings.h:158
> > > +    virtual void setOpaqueRootLayer(bool) = 0;
> > 
> > I don't think it makes sense for WebSettings to be talking about the root layer.  This is really about whether the WebView may be rendered translucently, right?
> 
> It's about allowing the contents to be translucent. What would be a better way to expose this?

I think my main problem is that the name is bad.  If it's about allowing the contents to be translucent, why does the name say nothing about contents or translucency?

> 
> > 
> > What's supposed to happen when this view isn't composited?
> 
> It's not supposed to have any effect. Having this only supported with compositing is good enough for the current use case. I happens to work right now without compositing but that's not a requirement afaik.

That seems pretty fragile.
Comment 11 David Reveman 2012-06-04 16:28:34 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 145368 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=145368&action=review
> > > 
> > > Code looks reasonable but it could be much simpler.
> > > 
> > > This also needs tests of some form.
> > 
> > We could unit test it based on glClear being called. Does that seem reasonable?
> > 
> 
> Sounds reasonable, but in release builds we only issue a glClear on non-root layers (since it's just a debug aid, not for correctness) so I'm not sure how useful of a test you would end up with.  What would the test actually do?

It would just check that glClear was issued with correct parameters for root layers when this feature is enabled. That needs to happen for both release and debug builds when translucent contents is allowed.

> 
> > > 
> > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:396
> > > > +    if (renderSurface != rootRenderSurface || !settings().opaqueRootLayer)
> > > 
> > > This is weird as a setting - why not just check if the root layer is opaque directly?
> > 
> > Eventually, I'd like to make this check be "if (renderSurface->isOpaque())" to avoid clearing all render surfaces with an opaque backing layer. But that's a change in behavior that I would prefer to land in a separate patch. The reason I'm not using the backing layer opaque value for the root render surface in this patch is that this requires changes to both webkit and chromium (ui compositor root layer) to ensure that we're not doing unnecessary clearing of the root layer.
> > 
> 
> The only chromium-side change you would need would be to not mark the root layer as being opaque, but you have to set that anyway to render correctly - no?  This setting seems strictly redundant with with setOpaque() on the root layer.

The layer representing the webview with translucent contents in the browser compositor is already not marked as opaque. We would have to mark the browser compositor root layer as opaque to not unnecessarily issue a glClear every time the browser compositor draws. And on the webkit side we need to correctly mark the actual root layer used there as opaque (what layer that is depends on if we need to use a scroll layer or not).

If you're OK with it, I'd like to leave this as part of fixing this bug:
https://bugs.webkit.org/show_bug.cgi?id=88265

> 
> > > 
> > > > Source/WebKit/chromium/public/WebSettings.h:158
> > > > +    virtual void setOpaqueRootLayer(bool) = 0;
> > > 
> > > I don't think it makes sense for WebSettings to be talking about the root layer.  This is really about whether the WebView may be rendered translucently, right?
> > 
> > It's about allowing the contents to be translucent. What would be a better way to expose this?
> 
> I think my main problem is that the name is bad.  If it's about allowing the contents to be translucent, why does the name say nothing about contents or translucency?

So how about setAllowTranslucentContentsWithAcceleratedCompositing()?

> 
> > 
> > > 
> > > What's supposed to happen when this view isn't composited?
> > 
> > It's not supposed to have any effect. Having this only supported with compositing is good enough for the current use case. I happens to work right now without compositing but that's not a requirement afaik.
> 
> That seems pretty fragile.

We could make a request for allowing translucent contents trigger compositing.
Comment 12 James Robinson 2012-06-04 17:49:43 PDT
(In reply to comment #11)
> > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:396
> > > > > +    if (renderSurface != rootRenderSurface || !settings().opaqueRootLayer)
> > > > 
> > > > This is weird as a setting - why not just check if the root layer is opaque directly?
> > > 
> > > Eventually, I'd like to make this check be "if (renderSurface->isOpaque())" to avoid clearing all render surfaces with an opaque backing layer. But that's a change in behavior that I would prefer to land in a separate patch. The reason I'm not using the backing layer opaque value for the root render surface in this patch is that this requires changes to both webkit and chromium (ui compositor root layer) to ensure that we're not doing unnecessary clearing of the root layer.
> > > 
> > 
> > The only chromium-side change you would need would be to not mark the root layer as being opaque, but you have to set that anyway to render correctly - no?  This setting seems strictly redundant with with setOpaque() on the root layer.
> 
> The layer representing the webview with translucent contents in the browser compositor is already not marked as opaque. We would have to mark the browser compositor root layer as opaque to not unnecessarily issue a glClear every time the browser compositor draws. And on the webkit side we need to correctly mark the actual root layer used there as opaque (what layer that is depends on if we need to use a scroll layer or not).

Isn't that what your changes to NonCompositedContentHost are doing?  If not, what are those changes doing?

> If you're OK with it, I'd like to leave this as part of fixing this bug:
> https://bugs.webkit.org/show_bug.cgi?id=88265

No, it's a different issue.

> 
> > 
> > > > 
> > > > > Source/WebKit/chromium/public/WebSettings.h:158
> > > > > +    virtual void setOpaqueRootLayer(bool) = 0;
> > > > 
> > > > I don't think it makes sense for WebSettings to be talking about the root layer.  This is really about whether the WebView may be rendered translucently, right?
> > > 
> > > It's about allowing the contents to be translucent. What would be a better way to expose this?
> > 
> > I think my main problem is that the name is bad.  If it's about allowing the contents to be translucent, why does the name say nothing about contents or translucency?
> 
> So how about setAllowTranslucentContentsWithAcceleratedCompositing()?
> 
> > 
> > > 
> > > > 
> > > > What's supposed to happen when this view isn't composited?
> > > 
> > > It's not supposed to have any effect. Having this only supported with compositing is good enough for the current use case. I happens to work right now without compositing but that's not a requirement afaik.
> > 
> > That seems pretty fragile.
> 
> We could make a request for allowing translucent contents trigger compositing.

The problem is requesting compositing doesn't necessarily mean that you will get compositing - if there's some failure in the initialization we'll silently fall back to software rendering.  Seems like a big footgun to have a setting that changes the rendering, but only sometimes.  Maybe this just needs big comment and long ugly name to warn people (as you suggested above).
Comment 13 David Reveman 2012-06-05 08:57:08 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:396
> > > > > > +    if (renderSurface != rootRenderSurface || !settings().opaqueRootLayer)
> > > > > 
> > > > > This is weird as a setting - why not just check if the root layer is opaque directly?
> > > > 
> > > > Eventually, I'd like to make this check be "if (renderSurface->isOpaque())" to avoid clearing all render surfaces with an opaque backing layer. But that's a change in behavior that I would prefer to land in a separate patch. The reason I'm not using the backing layer opaque value for the root render surface in this patch is that this requires changes to both webkit and chromium (ui compositor root layer) to ensure that we're not doing unnecessary clearing of the root layer.
> > > > 
> > > 
> > > The only chromium-side change you would need would be to not mark the root layer as being opaque, but you have to set that anyway to render correctly - no?  This setting seems strictly redundant with with setOpaque() on the root layer.
> > 
> > The layer representing the webview with translucent contents in the browser compositor is already not marked as opaque. We would have to mark the browser compositor root layer as opaque to not unnecessarily issue a glClear every time the browser compositor draws. And on the webkit side we need to correctly mark the actual root layer used there as opaque (what layer that is depends on if we need to use a scroll layer or not).
> 
> Isn't that what your changes to NonCompositedContentHost are doing?  If not, what are those changes doing?

No, that's just to make sure blending is used correctly when drawing the NonCompositedContent layer.

> 
> > If you're OK with it, I'd like to leave this as part of fixing this bug:
> > https://bugs.webkit.org/show_bug.cgi?id=88265
> 
> No, it's a different issue.

It doesn't have to be as part of the same patch. I really just want a change that makes us use layer opaque value for glClear to land separably because I don't want regressions related to such a change to be confused with the feature introduced by this patch.

> 
> > 
> > > 
> > > > > 
> > > > > > Source/WebKit/chromium/public/WebSettings.h:158
> > > > > > +    virtual void setOpaqueRootLayer(bool) = 0;
> > > > > 
> > > > > I don't think it makes sense for WebSettings to be talking about the root layer.  This is really about whether the WebView may be rendered translucently, right?
> > > > 
> > > > It's about allowing the contents to be translucent. What would be a better way to expose this?
> > > 
> > > I think my main problem is that the name is bad.  If it's about allowing the contents to be translucent, why does the name say nothing about contents or translucency?
> > 
> > So how about setAllowTranslucentContentsWithAcceleratedCompositing()?
> > 
> > > 
> > > > 
> > > > > 
> > > > > What's supposed to happen when this view isn't composited?
> > > > 
> > > > It's not supposed to have any effect. Having this only supported with compositing is good enough for the current use case. I happens to work right now without compositing but that's not a requirement afaik.
> > > 
> > > That seems pretty fragile.
> > 
> > We could make a request for allowing translucent contents trigger compositing.
> 
> The problem is requesting compositing doesn't necessarily mean that you will get compositing - if there's some failure in the initialization we'll silently fall back to software rendering.  Seems like a big footgun to have a setting that changes the rendering, but only sometimes.  Maybe this just needs big comment and long ugly name to warn people (as you suggested above).

Ok, makes sense.

FYI, currently compositing mode produces a different result compared to software rendering. And this new option actually makes compositing mode produce the same result as software rendering. The reason I'm not just making it default is that it will make us call glClear way more then we currently do.
Comment 14 Dana Jansens 2012-06-05 09:49:39 PDT
(In reply to comment #13)
> > > The layer representing the webview with translucent contents in the browser compositor is already not marked as opaque. We would have to mark the browser compositor root layer as opaque to not unnecessarily issue a glClear every time the browser compositor draws. And on the webkit side we need to correctly mark the actual root layer used there as opaque (what layer that is depends on if we need to use a scroll layer or not).
> > 
> > Isn't that what your changes to NonCompositedContentHost are doing?  If not, what are those changes doing?
> 
> No, that's just to make sure blending is used correctly when drawing the NonCompositedContent layer.

I'm confused by this. The NCCH constructor sets opaque(true) on its graphics layer.
Comment 15 David Reveman 2012-06-05 11:33:19 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > > > The layer representing the webview with translucent contents in the browser compositor is already not marked as opaque. We would have to mark the browser compositor root layer as opaque to not unnecessarily issue a glClear every time the browser compositor draws. And on the webkit side we need to correctly mark the actual root layer used there as opaque (what layer that is depends on if we need to use a scroll layer or not).
> > > 
> > > Isn't that what your changes to NonCompositedContentHost are doing?  If not, what are those changes doing?
> > 
> > No, that's just to make sure blending is used correctly when drawing the NonCompositedContent layer.
> 
> I'm confused by this. The NCCH constructor sets opaque(true) on its graphics layer.

Yes, NCCH uses opaque(true) by default on its graphics layer. It needs to be opaque(false) for translucent contents to be allowed. Alternatively, we can make  it opaque(false) by default but we probably still want to make sure it's set to opaque(true) when translucent contents is not allowed to avoid unnecessary blending.
Comment 16 James Robinson 2012-06-05 12:44:45 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > > > The layer representing the webview with translucent contents in the browser compositor is already not marked as opaque. We would have to mark the browser compositor root layer as opaque to not unnecessarily issue a glClear every time the browser compositor draws. And on the webkit side we need to correctly mark the actual root layer used there as opaque (what layer that is depends on if we need to use a scroll layer or not).
> > > > 
> > > > Isn't that what your changes to NonCompositedContentHost are doing?  If not, what are those changes doing?
> > > 
> > > No, that's just to make sure blending is used correctly when drawing the NonCompositedContent layer.
> > 
> > I'm confused by this. The NCCH constructor sets opaque(true) on its graphics layer.
> 
> Yes, NCCH uses opaque(true) by default on its graphics layer. It needs to be opaque(false) for translucent contents to be allowed. Alternatively, we can make  it opaque(false) by default but we probably still want to make sure it's set to opaque(true) when translucent contents is not allowed to avoid unnecessary blending.

Why not just set it to the correct value (opaque=true unless the view is marked as translucent) ?
Comment 17 David Reveman 2012-06-05 15:27:13 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #13)
> > > > > > The layer representing the webview with translucent contents in the browser compositor is already not marked as opaque. We would have to mark the browser compositor root layer as opaque to not unnecessarily issue a glClear every time the browser compositor draws. And on the webkit side we need to correctly mark the actual root layer used there as opaque (what layer that is depends on if we need to use a scroll layer or not).
> > > > > 
> > > > > Isn't that what your changes to NonCompositedContentHost are doing?  If not, what are those changes doing?
> > > > 
> > > > No, that's just to make sure blending is used correctly when drawing the NonCompositedContent layer.
> > > 
> > > I'm confused by this. The NCCH constructor sets opaque(true) on its graphics layer.
> > 
> > Yes, NCCH uses opaque(true) by default on its graphics layer. It needs to be opaque(false) for translucent contents to be allowed. Alternatively, we can make  it opaque(false) by default but we probably still want to make sure it's set to opaque(true) when translucent contents is not allowed to avoid unnecessary blending.
> 
> Why not just set it to the correct value (opaque=true unless the view is marked as translucent) ?

That's what the current patch is doing except that I didn't remove the existing setOpaque(true) call in the NCCH constructor. Do you want me to just remove that call in the NCCH constructor? I'll be happy to do that.
Comment 18 James Robinson 2012-06-05 15:53:40 PDT
Yeah, I think that would be much cleaner - set the value correctly in NCCH initialization and get rid of the setting.
Comment 19 James Robinson 2012-06-05 15:53:57 PDT
The compositor setting that is, you still need one on WebSettings (or WebView)
Comment 20 David Reveman 2012-06-06 16:49:01 PDT
Created attachment 146142 [details]
Patch

Remove WebSetting and forward the WebView transparency setting to CCLayerTreeHostImpl
Comment 21 David Reveman 2012-06-06 16:50:38 PDT
jamesr, I apologize for wasting your time with previous patches. After some more investigation I realized that there's already a mechanism to tell the renderer process to use a transparent background. It's just that this is not available to us at the time we instantiate the WebLayerTreeView and the NCCH. Transparency, just as background color, can change throughout the lifetime of the WebLayerTreeView and the NCCH.

The new patch forwards the transparency setting to CCLayerTreeHostImpl and uses it to determine how to draw the background and clear the root render pass. I also added some unit tests.
Comment 22 Dana Jansens 2012-06-06 16:57:19 PDT
Comment on attachment 146142 [details]
Patch

Nice, LGTM
Comment 23 James Robinson 2012-06-07 11:52:15 PDT
Comment on attachment 146142 [details]
Patch

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

> Source/Platform/chromium/public/WebLayerTreeView.h:122
> +    // Sets the transparency for the viewport.
> +    WEBKIT_EXPORT void setIsTransparent(bool);

I don't like setIsTransparent() on a view - we aren't saying that the view is transparent (you wouldn't be able to see anything in it if it was), we're saying something about the viewport. Could you put "viewport" in the function name?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:325
> +    if (!m_isTransparent)
> +        passes.last()->appendQuadsToFillScreen(m_rootLayerImpl.get(), m_backgroundColor, occlusionTracker);

Where are the tests for this change?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:67
> +    bool isTransparent() const { return m_isTransparent; }

I think this is really misleading - if the pass really was transparent, then we wouldn't have to draw anything at all. This bit means that there's some transparency within the pass and that clearing to an opaque color is not a no-op.  Seems like this could be better named.

Or even better, not put this state on CCRenderPass at all since LayerRendererChromium already knows how to special-case the root pass.  CCLTHI can tell LRC whether to clear the root pass to opaque or not in debug builds.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:70
> +    explicit CCRenderPass(CCRenderSurface*, bool isTransparent);

If you leave this, can you please make this a setter on pass instead of construction-time so you don't have to touch so many things? arguably being opaque is the exceptional case, so it should have the setter and the default should be "transparent" (or translucent)
Comment 24 David Reveman 2012-06-07 21:40:34 PDT
Created attachment 146472 [details]
Patch

setHasTransparentBackground(), add background unit test and avoid background filters on render passes with transparent bg
Comment 25 David Reveman 2012-06-07 21:46:45 PDT
(In reply to comment #23)
> (From update of attachment 146142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146142&action=review
> 
> > Source/Platform/chromium/public/WebLayerTreeView.h:122
> > +    // Sets the transparency for the viewport.
> > +    WEBKIT_EXPORT void setIsTransparent(bool);
> 
> I don't like setIsTransparent() on a view - we aren't saying that the view is transparent (you wouldn't be able to see anything in it if it was), we're saying something about the viewport. Could you put "viewport" in the function name?

Changed to setHasTransparentBackground as discussed.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:325
> > +    if (!m_isTransparent)
> > +        passes.last()->appendQuadsToFillScreen(m_rootLayerImpl.get(), m_backgroundColor, occlusionTracker);
> 
> Where are the tests for this change?

Fixed.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:67
> > +    bool isTransparent() const { return m_isTransparent; }
> 
> I think this is really misleading - if the pass really was transparent, then we wouldn't have to draw anything at all. This bit means that there's some transparency within the pass and that clearing to an opaque color is not a no-op.  Seems like this could be better named.
> 
> Or even better, not put this state on CCRenderPass at all since LayerRendererChromium already knows how to special-case the root pass.  CCLTHI can tell LRC whether to clear the root pass to opaque or not in debug builds.

Changed to setHasTransparentBackground and kept the state on CCRenderPass as discussed.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:70
> > +    explicit CCRenderPass(CCRenderSurface*, bool isTransparent);
> 
> If you leave this, can you please make this a setter on pass instead of construction-time so you don't have to touch so many things? arguably being opaque is the exceptional case, so it should have the setter and the default should be "transparent" (or translucent)

Default is now transparent and CCRenderPass has a setter.

Also fixed handling of background filters, which we have to avoid when drawing to non-opaque render surfaces.
Comment 26 James Robinson 2012-06-08 12:34:49 PDT
Comment on attachment 146472 [details]
Patch

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

R=me

One other possibly complication - not with this patch but with the idea in general - is that some callers want to use background filters for accessibility, so they have to be really careful to not set the viewport as transparent and try to set background filters.  I think that all we need to do on this side is be careful in our header comments, but it's something to be aware of.

> Source/Platform/chromium/public/WebLayerTreeView.h:121
> +    // Sets the background transparency for the viewport.

Please also document the default (I'm pretty sure it's transparent = false)

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:617
> +    // FIXME: We only allow background filters on an opaque render surface because other surfaces may contain

could you please also update the comment on WebLayer::setBackgroundFilters?
Comment 27 David Reveman 2012-06-08 13:16:20 PDT
Created attachment 146635 [details]
Patch

Update comment for WebLayer::setBackgroundFilters and document the default value for WebLayerTreeView::setHasTransparentBackground.
Comment 28 WebKit Review Bot 2012-06-08 19:29:26 PDT
Comment on attachment 146635 [details]
Patch

Clearing flags on attachment: 146635

Committed r119887: <http://trac.webkit.org/changeset/119887>
Comment 29 WebKit Review Bot 2012-06-08 19:29:32 PDT
All reviewed patches have been landed.  Closing bug.