WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 87821
[Chromium] Compositor doesn't support translucent root layers.
https://bugs.webkit.org/show_bug.cgi?id=87821
Summary
[Chromium] Compositor doesn't support translucent root layers.
David Reveman
Reported
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.
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
David Reveman
Comment 1
2012-06-01 11:57:15 PDT
Created
attachment 145348
[details]
Patch
WebKit Review Bot
Comment 2
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
.
vollick
Comment 3
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.
James Robinson
Comment 4
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?
David Reveman
Comment 5
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.
David Reveman
Comment 6
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
David Reveman
Comment 7
2012-06-01 13:30:22 PDT
Created
attachment 145368
[details]
Patch Turn off subpixel AA for non-opaque layers
James Robinson
Comment 8
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?
David Reveman
Comment 9
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.
James Robinson
Comment 10
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.
David Reveman
Comment 11
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.
James Robinson
Comment 12
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).
David Reveman
Comment 13
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.
Dana Jansens
Comment 14
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.
David Reveman
Comment 15
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.
James Robinson
Comment 16
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) ?
David Reveman
Comment 17
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.
James Robinson
Comment 18
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.
James Robinson
Comment 19
2012-06-05 15:53:57 PDT
The compositor setting that is, you still need one on WebSettings (or WebView)
David Reveman
Comment 20
2012-06-06 16:49:01 PDT
Created
attachment 146142
[details]
Patch Remove WebSetting and forward the WebView transparency setting to CCLayerTreeHostImpl
David Reveman
Comment 21
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.
Dana Jansens
Comment 22
2012-06-06 16:57:19 PDT
Comment on
attachment 146142
[details]
Patch Nice, LGTM
James Robinson
Comment 23
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)
David Reveman
Comment 24
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
David Reveman
Comment 25
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.
James Robinson
Comment 26
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?
David Reveman
Comment 27
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.
WebKit Review Bot
Comment 28
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
>
WebKit Review Bot
Comment 29
2012-06-08 19:29:32 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug