Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13462003:
Add support for accelerated fixed root background (Closed)
|
Created:
7 years, 8 months ago by Ian Vollick Modified:
7 years, 5 months ago CC:
blink-reviews, enne (OOO) Base URL:
https://chromium.googlesource.com/chromium/blink.git@background-attachment-fixed2 Visibility:
Public. |
DescriptionAdd support for accelerated fixed root background
Putting a fixed root background in a separate composited layer
requires a tiled layer cache currently. This patch makes it possible to
use the feature, even without a tiled layer cache.
This patch also adds fixes from this WebKit patch by Simon Fraser:
https://bugs.webkit.org/show_bug.cgi?id=115951
This patch depends on tests and plumbing provided by https://codereview.chromium.org/17398002/
BUG=180885
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153648
Patch Set 1 #
Total comments: 7
Patch Set 2 : Now with no scroll compensation. #Patch Set 3 : Adding a FIXME for a bit that'll need fixing. #
Total comments: 2
Patch Set 4 : . #Patch Set 5 : Rebased after demise of the NCCH. #
Total comments: 5
Patch Set 6 : . #
Total comments: 1
Patch Set 7 : . #Patch Set 8 : rebase. #Patch Set 9 : rebase now that pixel result has landed. #Patch Set 10 : Updated with WK layout tests from smfr. #Patch Set 11 : Moved the layout tests. #
Total comments: 8
Patch Set 12 : . #
Total comments: 13
Patch Set 13 : . #Patch Set 14 : . #
Total comments: 4
Patch Set 15 : . #Patch Set 16 : . #
Messages
Total messages: 27 (0 generated)
This patch is based on https://codereview.chromium.org/13665002/
I think this can be simplified a bit without making any behavior changes.
Remember blink means chromium so there's no point in adding compile time guards
or testing for things that will never be true in the chromium port.
I want to make sure we understand what the GraphicsLayer tree is like and that
it fits what we want.
Here's our current GraphicsLayer tree (note that this isn't the same as the
WebKit2 GraphicsLayer tree, which is what Simon was designing for):
+ RLC::m_overflowControlsHost
+ RLC::m_clipLayer
+ RLC::m_scrollLayer
+ NCCH::m_graphicsLayer
+ RLC::m_rootContentLayer
children
+ RLC::m_layerForHorizontalScrollbar
+ RLC::m_layerForVerticalScrollbar
+ RLC::m_layerForScrollCorner
If the body has a background with background-attachment: fixed, it needs to
render under the composited content, under the NonCompositedContentHost, but not
render under the scrollbars if they are non-overlay. Seems like the right place
to put it would be as a child of the root RLC::m_clipLayer but not under the
m_scrollLayer so it doesn't move when the root scrolls.
It seems like if I understand correctly what actually happens with this patch is
that the whole subtree is underneath the RLC::m_rootContentLayer as such:
....
+ RLC::m_rootContentLayer
+ RLB::m_ancestorClippingLayer
+ RLB::m_contentsContainmentLayer
+ RLB::m_backgroundLayer <---- draws the background
+ RLB::m_graphicsLayer
+ RLB::m_childContainmentLayer
+ RLB::m_scrollingLayer
+ RLB::m_scrollingContentsLayer <--- hosts children
+ RLB::m_layerForHorizontalScrollbar
+ RLB::m_layerForVerticalScrollbar
+ RLB::m_layerForScrollCorner
Is that right? If so, a few questions:
*) Does this mean having a background-attachment: fixed means that there is
nothing in the NonCompositedContentHosts' GraphicsLayer tree and everything is
in an RLB under the root?
*) Why do we need to set this layer as fixed if it's an ancestor of the
scrolling layer?
*) Why is this on RenderLayerBacking at all if it's about the root?
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/page/FrameView...
File Source/WebCore/page/FrameView.cpp (right):
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/page/FrameView...
Source/WebCore/page/FrameView.cpp:2008: if (m_nestedLayoutCount <= 1 &&
(hasViewportConstrainedObjects() || hasFixedRootBackgroundLayer())) {
This change will make us run through all RenderLayers deciding which are
composited on every scroll due to background-attachment:fixed, won't it? That
seems like a bit of a waste since scrolling with a background-attachment:fixed
shouldn't cause any changes in what is or isn't composited on the page
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/page/Settings.h
File Source/WebCore/page/Settings.h (right):
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/page/Settings....
Source/WebCore/page/Settings.h:128: // NB: This define is generated by a script.
To add a new setting, you
not sure what this change has to do with the patch - is it an intentional
change?
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderLayerBacking.cpp (right):
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayerBacking.cpp:128: // FIXME: It's a little
weird that we base this decision on whether there's a scrolling coordinator or
not.
this if seems silly - if we're compositing at all (which we have to be since
we're in RenderLayerBacking), we always have a ScrollingCoordinator. You can
just delete this whole thing. The only thing that matters as far as whether the
fixed background applies or not is if this layer is a root layer
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayerBacking.cpp:294: #if PLATFORM(CHROMIUM)
we don't want to add *more* #if PLATFORM(CHROMIUM) guards in blink. blink means
platform==chromium so there's no need for a compile guard
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1249: if
(ScrollingCoordinator* scrollingCoordinator = this->scrollingCoordinator()) {
if we have compositing, we always have a scrolling coordinator
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayerCompositor.cpp:1251: if (!renderViewBacking)
do you know how this branch could fail?
https://codereview.chromium.org/13462003/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayerCompositor.cpp:2259: return
renderViewBacking && renderViewBacking->usingTiledBacking();
this part of the branch is clearly dead code now, so you can simply this
function to just return the settings value, or possibly fold the setting checks
into the caller(s) if that makes more sense
I'd find it useful if you would indicate what GraphicsLayer tree you expect to have with a fixed background and which GraphicsLayer is responsible for painting which bit of content.
You'll have to teach the NCCH not to paint the background phase at least, right?
https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/page/Frame... File Source/WebCore/page/FrameView.cpp (right): https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/page/Frame... Source/WebCore/page/FrameView.cpp:2008: if (m_nestedLayoutCount <= 1 && (hasViewportConstrainedObjects() || hasFixedRootBackgroundLayer())) { why do you need to updateCompositingLayers() on scroll due to a fixed root background layer? will that change the result of the overlap test? https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/rendering/... File Source/WebCore/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/rendering/... Source/WebCore/rendering/RenderLayerCompositor.cpp:1253: // FIXME: there must be a better way to insert this layer into the tree. yes, please figure out a better way. are the layers you want to be positioned relative to members of RenderLayerCompositor?
On 2013/04/09 19:45:48, jamesr wrote: > https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/page/Frame... > File Source/WebCore/page/FrameView.cpp (right): > > https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/page/Frame... > Source/WebCore/page/FrameView.cpp:2008: if (m_nestedLayoutCount <= 1 && > (hasViewportConstrainedObjects() || hasFixedRootBackgroundLayer())) { > why do you need to updateCompositingLayers() on scroll due to a fixed root > background layer? will that change the result of the overlap test? It's unnecessary. Removed. > > https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/rendering/... > File Source/WebCore/rendering/RenderLayerCompositor.cpp (right): > > https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/rendering/... > Source/WebCore/rendering/RenderLayerCompositor.cpp:1253: // FIXME: there must be > a better way to insert this layer into the tree. > yes, please figure out a better way. are the layers you want to be positioned > relative to members of RenderLayerCompositor? Done.
On 2013/05/08 20:48:56, vollick wrote: > On 2013/04/09 19:45:48, jamesr wrote: > > > https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/page/Frame... > > File Source/WebCore/page/FrameView.cpp (right): > > > > > https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/page/Frame... > > Source/WebCore/page/FrameView.cpp:2008: if (m_nestedLayoutCount <= 1 && > > (hasViewportConstrainedObjects() || hasFixedRootBackgroundLayer())) { > > why do you need to updateCompositingLayers() on scroll due to a fixed root > > background layer? will that change the result of the overlap test? > It's unnecessary. Removed. > > > > > https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/rendering/... > > File Source/WebCore/rendering/RenderLayerCompositor.cpp (right): > > > > > https://codereview.chromium.org/13462003/diff/10001/Source/WebCore/rendering/... > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1253: // FIXME: there must > be > > a better way to insert this layer into the tree. > > yes, please figure out a better way. are the layers you want to be positioned > > relative to members of RenderLayerCompositor? > Done. jamesr, PTAL. The removal of the NCCH has simplified this patch a fair bit.
Definitely looks better. I think messing with children based on their position in the hierarchy is the only blocker for me, we really should never need to do that. https://codereview.chromium.org/13462003/diff/24001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerBacking.cpp (right): https://codereview.chromium.org/13462003/diff/24001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:986: // This assumes that the background layer is only used for fixed backgrounds, which is currently a correct assumption. can we put this in assumption code with an ASSERT() as well so we stand a chance of noticing if it changes? https://codereview.chromium.org/13462003/diff/24001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:1893: // m_ancestorClippingLayer and m_childContainmentLayer are just used for masking or containment, so have no backing. aside: I'm pretty sure this whole backingStoreMemoryEstimate() function is dead code for us. No matter, though https://codereview.chromium.org/13462003/diff/24001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/13462003/diff/24001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.cpp:1180: m_clipLayer->children()[0]->removeFromParent(); this is really fragile - do we not have a proper name for the layer we're trying to remove?
Thanks for the review! https://codereview.chromium.org/13462003/diff/24001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerBacking.cpp (right): https://codereview.chromium.org/13462003/diff/24001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:986: // This assumes that the background layer is only used for fixed backgrounds, which is currently a correct assumption. On 2013/05/21 23:05:12, jamesr wrote: > can we put this in assumption code with an ASSERT() as well so we stand a chance > of noticing if it changes? Done. (Added an assert in paintIntoLayer to ensure that we're using the PaintLayerPaintingRootBackgroundOnly phase). https://codereview.chromium.org/13462003/diff/24001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/13462003/diff/24001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.cpp:1180: m_clipLayer->children()[0]->removeFromParent(); On 2013/05/21 23:05:12, jamesr wrote: > this is really fragile - do we not have a proper name for the layer we're trying > to remove? I've replaced this with code that ASSERTS about the number of children of the clip layer, removes all, and then reattaches the scroll layer. Does this seem less error-prone?
This is definitely not less error-prone. I have no idea what you are trying to do, so I can't really give more specific advice. There must be some layer you are trying to have your layer placed relative to. What is it? https://codereview.chromium.org/13462003/diff/34001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/13462003/diff/34001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.cpp:1182: m_clipLayer->addChild(backgroundLayer); yuck - why is this still mucking with the children list directly? what is this trying to accomplish? why isn't this part of the normal tree build?
On 2013/05/22 19:44:01, jamesr wrote: > This is definitely not less error-prone. I have no idea what you are trying to > do, so I can't really give more specific advice. There must be some layer you > are trying to have your layer placed relative to. What is it? > I'm sorry. Looking back at your earlier review comments, I see that you'd asked me to describe exactly what I was trying to do with the graphics layer tree, but I missed that comment. I've updated this patch set with a detailed comment (with a diagram) describing how I intend to modify the tree. As to your specific question above, I'm positioning the background layer with respect to the scrolling layer. I need the bg layer to be the sibling below it. Adding the layer was always easy via GraphicsLayer::addChildBelow, but the problem was removing the old background layer, which the RLC didn't own. Ideally, we'd just call GraphicsLayer::removeFromParent, but since the RLC didn't have a convenient way of getting at the old bg layer, I ended up doing this brittle junk with the child list. But that was dumb. The owner of the background layer, the RLB, already removes the background layer from the GraphicsLayer tree when necessary, so the RLC only needed to worry about adding into the hierarchy in the right spot. > https://codereview.chromium.org/13462003/diff/34001/Source/core/rendering/Ren... > File Source/core/rendering/RenderLayerCompositor.cpp (right): > > https://codereview.chromium.org/13462003/diff/34001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayerCompositor.cpp:1182: > m_clipLayer->addChild(backgroundLayer); > yuck - why is this still mucking with the children list directly? what is this > trying to accomplish? > I believe I've addressed these questions above. > why isn't this part of the normal tree build? > Good question. It's historical. IIUC, RLC::fixedRootBackgroundLayerChanged's old purpose was to talk to the platform's mechanism for fixing the position of the BG layer, but this shouldn't be necessary for us. I've moved the code for adding the layer into rebuildCompositingLayerTree so it can be part of the normal tree build. It's pretty similar to the fg layer case, so this seemed like a good home.
On 2013/05/23 14:20:09, vollick wrote: > On 2013/05/22 19:44:01, jamesr wrote: > > This is definitely not less error-prone. I have no idea what you are trying > to > > do, so I can't really give more specific advice. There must be some layer you > > are trying to have your layer placed relative to. What is it? > > > I'm sorry. Looking back at your earlier review comments, I see that you'd asked > me to describe exactly what I was trying to do with the graphics layer tree, but > I missed that comment. I've updated this patch set with a detailed comment (with > a diagram) describing how I intend to modify the tree. > > As to your specific question above, I'm positioning the background layer with > respect to the scrolling layer. I need the bg layer to be the sibling below it. > Adding the layer was always easy via GraphicsLayer::addChildBelow, but the > problem was removing the old background layer, which the RLC didn't own. > Ideally, we'd just call GraphicsLayer::removeFromParent, but since the RLC > didn't have a convenient way of getting at the old bg layer, I ended up doing > this brittle junk with the child list. > > But that was dumb. > > The owner of the background layer, the RLB, already removes the background layer > from the GraphicsLayer tree when necessary, so the RLC only needed to worry > about adding into the hierarchy in the right spot. > > > > https://codereview.chromium.org/13462003/diff/34001/Source/core/rendering/Ren... > > File Source/core/rendering/RenderLayerCompositor.cpp (right): > > > > > https://codereview.chromium.org/13462003/diff/34001/Source/core/rendering/Ren... > > Source/core/rendering/RenderLayerCompositor.cpp:1182: > > m_clipLayer->addChild(backgroundLayer); > > yuck - why is this still mucking with the children list directly? what is > this > > trying to accomplish? > > > I believe I've addressed these questions above. > > > why isn't this part of the normal tree build? > > > Good question. It's historical. IIUC, RLC::fixedRootBackgroundLayerChanged's old > purpose was to talk to the platform's mechanism for fixing the position of the > BG layer, but this shouldn't be necessary for us. I've moved the code for adding > the layer into rebuildCompositingLayerTree so it can be part of the normal tree > build. It's pretty similar to the fg layer case, so this seemed like a good > home. Rebased now that trchen's r151453 (https://chromiumcodereview.appspot.com/15973002) has landed. Noteworthy change from that rebase: RenderLayerBacking::updateContentsOpaque(). PTAL!
On 2013/05/31 14:17:07, vollick wrote: > On 2013/05/23 14:20:09, vollick wrote: > > On 2013/05/22 19:44:01, jamesr wrote: > > > This is definitely not less error-prone. I have no idea what you are trying > > to > > > do, so I can't really give more specific advice. There must be some layer > you > > > are trying to have your layer placed relative to. What is it? > > > > > I'm sorry. Looking back at your earlier review comments, I see that you'd > asked > > me to describe exactly what I was trying to do with the graphics layer tree, > but > > I missed that comment. I've updated this patch set with a detailed comment > (with > > a diagram) describing how I intend to modify the tree. > > > > As to your specific question above, I'm positioning the background layer with > > respect to the scrolling layer. I need the bg layer to be the sibling below > it. > > Adding the layer was always easy via GraphicsLayer::addChildBelow, but the > > problem was removing the old background layer, which the RLC didn't own. > > Ideally, we'd just call GraphicsLayer::removeFromParent, but since the RLC > > didn't have a convenient way of getting at the old bg layer, I ended up doing > > this brittle junk with the child list. > > > > But that was dumb. > > > > The owner of the background layer, the RLB, already removes the background > layer > > from the GraphicsLayer tree when necessary, so the RLC only needed to worry > > about adding into the hierarchy in the right spot. > > > > > > > > https://codereview.chromium.org/13462003/diff/34001/Source/core/rendering/Ren... > > > File Source/core/rendering/RenderLayerCompositor.cpp (right): > > > > > > > > > https://codereview.chromium.org/13462003/diff/34001/Source/core/rendering/Ren... > > > Source/core/rendering/RenderLayerCompositor.cpp:1182: > > > m_clipLayer->addChild(backgroundLayer); > > > yuck - why is this still mucking with the children list directly? what is > > this > > > trying to accomplish? > > > > > I believe I've addressed these questions above. > > > > > why isn't this part of the normal tree build? > > > > > Good question. It's historical. IIUC, RLC::fixedRootBackgroundLayerChanged's > old > > purpose was to talk to the platform's mechanism for fixing the position of the > > BG layer, but this shouldn't be necessary for us. I've moved the code for > adding > > the layer into rebuildCompositingLayerTree so it can be part of the normal > tree > > build. It's pretty similar to the fg layer case, so this seemed like a good > > home. > > Rebased now that trchen's r151453 > (https://chromiumcodereview.appspot.com/15973002) has landed. > > Noteworthy change from that rebase: RenderLayerBacking::updateContentsOpaque(). > > PTAL! I've rebased and updated the settings plumbing. I've also added some new layout tests from a recent patch by smfr. (We're passing them).
The setting and tests could (and probably should) be landed separately. The implementation itself still looks like it needs work. https://codereview.chromium.org/13462003/diff/57001/LayoutTests/compositing/f... File LayoutTests/compositing/fixed-background-composited-html.html (right): https://codereview.chromium.org/13462003/diff/57001/LayoutTests/compositing/f... LayoutTests/compositing/fixed-background-composited-html.html:27: window.setTimeout(function() { what's the setTimeout for? https://codereview.chromium.org/13462003/diff/57001/LayoutTests/compositing/f... File LayoutTests/compositing/fixed-background-negative-z-index-fixed.html (right): https://codereview.chromium.org/13462003/diff/57001/LayoutTests/compositing/f... LayoutTests/compositing/fixed-background-negative-z-index-fixed.html:32: window.setTimeout(function() { what's the setTimeout for? https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerBacking.cpp (right): https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:1152: if (!viewIsTransparent) { A default-initialized Color and a color initialized to Color::transparent are not the same. is there a specific reason you're leaving the backgroundColor invalid when viewIsTransparent is true? https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:1162: // FIXME(vollick): This setting is not behaving as expected. What to do what setting is this comment referring to? https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.cpp:1105: m_clipLayer->addChildBelow(layerBacking->backgroundLayer(), m_scrollLayer.get()); why is this here? the code above and below this portion are about building up the z-order and normal flow lists, but this comment and code have nothing to do with that. why is it in a layer->isStackingContext() block? what does that have to do with background layers? look at how the overflow controls are set up, which is somewhat similar. those only update when there's a change in overflow state and have a dedicated function to set up the correct hierarchy and positioning. how is this case different? https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.cpp:1211: setCompositingLayersNeedRebuild(); this seems really heavyweight. does promoting due to fixed background cause other layers to need to be rebuilt? https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.cpp:2125: && m_renderView->document()->settings()->acceleratedCompositingForFixedRootBackgroundEnabled(); why isn't this setting stashed along with the other compositing triggers? https://codereview.chromium.org/13462003/diff/57001/Tools/DumpRenderTree/chro... File Tools/DumpRenderTree/chromium/TestShell.h (right): https://codereview.chromium.org/13462003/diff/57001/Tools/DumpRenderTree/chro... Tools/DumpRenderTree/chromium/TestShell.h:120: void setAcceleratedCompositingForFixedRootBackgroundEnabled(bool enabled) { m_acceleratedCompositingForFixedRootBackgroundEnabled = enabled; } DumpRenderTree is dead, I wouldn't bother trying to patch it
Thanks for the review! I've done another pass. PTAL when you're able. On 2013/06/13 19:48:47, jamesr wrote: > The setting and tests could (and probably should) be landed separately. The > implementation itself still looks like it needs work. > Done. (Patch is here: https://codereview.chromium.org/17398002/) > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > File Source/core/rendering/RenderLayerBacking.cpp (right): > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayerBacking.cpp:1152: if (!viewIsTransparent) { > A default-initialized Color and a color initialized to Color::transparent are > not the same. is there a specific reason you're leaving the backgroundColor > invalid when viewIsTransparent is true? backgroundColor is now initialized to Color::transparent. > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayerBacking.cpp:1162: // FIXME(vollick): This > setting is not behaving as expected. What to do > what setting is this comment referring to? After rebasing, this comment no longer applies. Removed. > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > File Source/core/rendering/RenderLayerCompositor.cpp (right): > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayerCompositor.cpp:1105: > m_clipLayer->addChildBelow(layerBacking->backgroundLayer(), > m_scrollLayer.get()); > why is this here? the code above and below this portion are about building up > the z-order and normal flow lists, but this comment and code have nothing to do > with that. why is it in a layer->isStackingContext() block? what does that have > to do with background layers? > > look at how the overflow controls are set up, which is somewhat similar. those > only update when there's a change in overflow state and have a dedicated > function to set up the correct hierarchy and positioning. how is this case > different? I did this in response to your comment "why isn't [the insertion of the backgroundlayer] part of the normal tree build?", but I probably misunderstood what you meant by normal tree build. I've put this code back in a dedicated function. > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayerCompositor.cpp:1211: > setCompositingLayersNeedRebuild(); > this seems really heavyweight. does promoting due to fixed background cause > other layers to need to be rebuilt? Removed. > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayerCompositor.cpp:2125: && > m_renderView->document()->settings()->acceleratedCompositingForFixedRootBackgroundEnabled(); > why isn't this setting stashed along with the other compositing triggers? Looking around in RenderLayerCompositor.cpp, the settings for the other compositing triggers appear to be accessed like this: if (Settings* settings = m_renderView->document()->settings()) { if (!settings->acceleratedCompositingForMyParticularReasonEnabled()) return; } I've updated this code to look the same. Please let me know if I've misunderstood your question. > > https://codereview.chromium.org/13462003/diff/57001/Tools/DumpRenderTree/chro... > File Tools/DumpRenderTree/chromium/TestShell.h (right): > > https://codereview.chromium.org/13462003/diff/57001/Tools/DumpRenderTree/chro... > Tools/DumpRenderTree/chromium/TestShell.h:120: void > setAcceleratedCompositingForFixedRootBackgroundEnabled(bool enabled) { > m_acceleratedCompositingForFixedRootBackgroundEnabled = enabled; } > DumpRenderTree is dead, I wouldn't bother trying to patch it Sg. I've removed that code.
On 2013/06/21 14:53:45, vollick wrote: > Thanks for the review! I've done another pass. PTAL when you're able. > > On 2013/06/13 19:48:47, jamesr wrote: > > The setting and tests could (and probably should) be landed separately. The > > implementation itself still looks like it needs work. > > > Done. (Patch is here: https://codereview.chromium.org/17398002/) > > > > > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > > File Source/core/rendering/RenderLayerBacking.cpp (right): > > > > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > > Source/core/rendering/RenderLayerBacking.cpp:1152: if (!viewIsTransparent) { > > A default-initialized Color and a color initialized to Color::transparent are > > not the same. is there a specific reason you're leaving the backgroundColor > > invalid when viewIsTransparent is true? > backgroundColor is now initialized to Color::transparent. > > > > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > > Source/core/rendering/RenderLayerBacking.cpp:1162: // FIXME(vollick): This > > setting is not behaving as expected. What to do > > what setting is this comment referring to? > After rebasing, this comment no longer applies. Removed. > > > > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > > File Source/core/rendering/RenderLayerCompositor.cpp (right): > > > > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > > Source/core/rendering/RenderLayerCompositor.cpp:1105: > > m_clipLayer->addChildBelow(layerBacking->backgroundLayer(), > > m_scrollLayer.get()); > > why is this here? the code above and below this portion are about building up > > the z-order and normal flow lists, but this comment and code have nothing to > do > > with that. why is it in a layer->isStackingContext() block? what does that > have > > to do with background layers? > > > > look at how the overflow controls are set up, which is somewhat similar. those > > only update when there's a change in overflow state and have a dedicated > > function to set up the correct hierarchy and positioning. how is this case > > different? > I did this in response to your comment "why isn't [the insertion of the > backgroundlayer] part of the normal tree build?", but I probably misunderstood > what you meant by normal tree build. I've put this code back in a dedicated > function. > > > > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > > Source/core/rendering/RenderLayerCompositor.cpp:1211: > > setCompositingLayersNeedRebuild(); > > this seems really heavyweight. does promoting due to fixed background cause > > other layers to need to be rebuilt? > Removed. > > > > > https://codereview.chromium.org/13462003/diff/57001/Source/core/rendering/Ren... > > Source/core/rendering/RenderLayerCompositor.cpp:2125: && > > > m_renderView->document()->settings()->acceleratedCompositingForFixedRootBackgroundEnabled(); > > why isn't this setting stashed along with the other compositing triggers? > Looking around in RenderLayerCompositor.cpp, the settings for the other > compositing triggers appear to be accessed like this: > > if (Settings* settings = m_renderView->document()->settings()) { > if (!settings->acceleratedCompositingForMyParticularReasonEnabled()) > return; > } > > I've updated this code to look the same. > > Please let me know if I've misunderstood your question. > > > > > > https://codereview.chromium.org/13462003/diff/57001/Tools/DumpRenderTree/chro... > > File Tools/DumpRenderTree/chromium/TestShell.h (right): > > > > > https://codereview.chromium.org/13462003/diff/57001/Tools/DumpRenderTree/chro... > > Tools/DumpRenderTree/chromium/TestShell.h:120: void > > setAcceleratedCompositingForFixedRootBackgroundEnabled(bool enabled) { > > m_acceleratedCompositingForFixedRootBackgroundEnabled = enabled; } > > DumpRenderTree is dead, I wouldn't bother trying to patch it > > Sg. I've removed that code. Friendly ping :)
https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:3526: else if (!backing()->paintsIntoCompositedAncestor() style nit: [braces-single-line] [braces-must-match] https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerBacking.cpp (left): https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:1515: if (m_contentsContainmentLayer) Is this just a separate removal that's unrelated to this patch? https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerBacking.cpp (right): https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:271: m_backgroundLayer->setContentsOpaque(m_owningLayer->backgroundIsKnownToBeOpaqueInRect(compositedBounds())); This seems at odds with the code in updateRootLayerConfiguration. It seems to me that the original code before this change is still correct. The background layer doesn't need to be updated and the graphics layer can be set to contents opaque if it's opaque in its composited bounds. https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:1152: FrameView* frameView = toRenderView(renderer())->frameView(); It seems like this background color code should all be in updateBackgroundColor and not in updateRootLayerConfiguration. Also, this seems like a lot of code to figure out the right background color. It's kind of duplicated with what WebViewImpl does in setting the background color on the WebLayerTreeView. Can you convince me that you need this check for transparency here? I am pretty sure that the compositor will do the right thing if the LayerTreeHost has been set to have a transparent background. https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:1165: m_backgroundLayer->contentLayer()->setDrawCheckerboardForMissingTiles(true); Can you do the checkerboard setup when the background layer is created/destroyed similar to how it's done for the primary graphics layer? https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.cpp:1199: // stay put, we position it in the layer tree as follows: Thanks for the layer diagram comment. https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.cpp:2114: if (Settings* settings = m_renderView->document()->settings()) { I think the caching that jamesr was talking about is https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:3526: else if (!backing()->paintsIntoCompositedAncestor() On 2013/07/02 19:32:41, enne wrote: > style nit: [braces-single-line] [braces-must-match] I went with braces-single-line. https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerBacking.cpp (left): https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:1515: if (m_contentsContainmentLayer) On 2013/07/02 19:32:41, enne wrote: > Is this just a separate removal that's unrelated to this patch? Yep, it's related. The only reason m_contentsContainmentLayer existed was to plug m_backgroundLayer into the RLB's graphics layer tree, but since we're hooking it up via the RLC, this is no longer necessary. https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerBacking.cpp (right): https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:271: m_backgroundLayer->setContentsOpaque(m_owningLayer->backgroundIsKnownToBeOpaqueInRect(compositedBounds())); On 2013/07/02 19:32:41, enne wrote: > This seems at odds with the code in updateRootLayerConfiguration. It seems to > me that the original code before this change is still correct. The background > layer doesn't need to be updated and the graphics layer can be set to contents > opaque if it's opaque in its composited bounds. If I skip making m_graphicsLayer non-opaque here, we're unable to see m_backgroundLayer behind it. I think it's still necessary, but let me know if this should be set up differently. That said, it totally is at odds with updateRootLayerConfiguration. That function should never have been messing with opaqueness since it's already handled here. https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:1152: FrameView* frameView = toRenderView(renderer())->frameView(); On 2013/07/02 19:32:41, enne wrote: > It seems like this background color code should all be in updateBackgroundColor > and not in updateRootLayerConfiguration. > > Also, this seems like a lot of code to figure out the right background color. > It's kind of duplicated with what WebViewImpl does in setting the background > color on the WebLayerTreeView. Can you convince me that you need this check for > transparency here? I am pretty sure that the compositor will do the right thing > if the LayerTreeHost has been set to have a transparent background. You're right, I think this is covered by the code in WebViewImpl. Removed. https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:1165: m_backgroundLayer->contentLayer()->setDrawCheckerboardForMissingTiles(true); On 2013/07/02 19:32:41, enne wrote: > Can you do the checkerboard setup when the background layer is created/destroyed > similar to how it's done for the primary graphics layer? Done.
https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... Source/core/rendering/RenderLayer.cpp:3526: else if (!backing()->paintsIntoCompositedAncestor() On 2013/07/03 01:32:02, vollick wrote: > On 2013/07/02 19:32:41, enne wrote: > > style nit: [braces-single-line] [braces-must-match] > > I went with braces-single-line. This hasn't always been the case, but [braces-single-line] says that {} are required when your if statement body has multiple lines. https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerBacking.cpp (right): https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:270: m_graphicsLayer->setContentsOpaque(false); I can see where you're coming from. I guess since the background is not painted by this graphics layer, technically the background might known to be opaque, but the background layer is still in front of it. That seems like an issue with backgroundIsKnownToBeOpaqueInRect, but maybe not something to solve here. https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:840: if (!m_childContainmentLayer && !m_isMainFrameRenderViewLayer) { Could you explain this change to me? I'm not sure I follow. https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerBacking.cpp:1142: nit: needless whitespace change https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... File Source/core/rendering/RenderLayerCompositor.cpp (right): https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderLayerCompositor.cpp:2115: if (Settings* settings = m_renderView->document()->settings()) { I also agree that it's probably not worth caching this flag.
On 2013/07/03 01:52:26, enne wrote: > https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... > File Source/core/rendering/RenderLayer.cpp (right): > > https://codereview.chromium.org/13462003/diff/67001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayer.cpp:3526: else if > (!backing()->paintsIntoCompositedAncestor() > On 2013/07/03 01:32:02, vollick wrote: > > On 2013/07/02 19:32:41, enne wrote: > > > style nit: [braces-single-line] [braces-must-match] > > > > I went with braces-single-line. > > This hasn't always been the case, but [braces-single-line] says that {} are > required when your if statement body has multiple lines. Sorry, I should have known you were talking about the curlies, not the bananas. I've put back the curlies I removed, and the style checker then demanded I put curlies elsewhere, too. Does this look ok now? > > https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... > File Source/core/rendering/RenderLayerBacking.cpp (right): > > https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayerBacking.cpp:270: > m_graphicsLayer->setContentsOpaque(false); > I can see where you're coming from. I guess since the background is not painted > by this graphics layer, technically the background might known to be opaque, but > the background layer is still in front of it. > > That seems like an issue with backgroundIsKnownToBeOpaqueInRect, but maybe not > something to solve here. > > https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayerBacking.cpp:840: if (!m_childContainmentLayer > && !m_isMainFrameRenderViewLayer) { > Could you explain this change to me? I'm not sure I follow. Yeah, this isn't obvious. I've added a comment: // We don't need a child containment layer if we're the main frame render view // layer. It's redundant as the frame clip above us will handle this clipping. > > https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayerBacking.cpp:1142: > nit: needless whitespace change Removed. > > https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... > File Source/core/rendering/RenderLayerCompositor.cpp (right): > > https://codereview.chromium.org/13462003/diff/87001/Source/core/rendering/Ren... > Source/core/rendering/RenderLayerCompositor.cpp:2115: if (Settings* settings = > m_renderView->document()->settings()) { > I also agree that it's probably not worth caching this flag. K, cool.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vollick@chromium.org/13462003/93001
Failed to apply patch for LayoutTests/TestExpectations: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file LayoutTests/TestExpectations Hunk #1 FAILED at 1433. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/TestExpectations.rej Patch: LayoutTests/TestExpectations Index: LayoutTests/TestExpectations diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations index c3b5a66f26191b7a514ebfd04c0594473370c086..a28bd6d16fab82017e428a2d30e782e6d621dec1 100644 --- a/LayoutTests/TestExpectations +++ b/LayoutTests/TestExpectations @@ -1433,9 +1433,6 @@ crbug.com/242511 [ Mac Debug ] svg/custom/uri-reference-handling.svg [ Crash Pas # Seems to be flaky on all platforms crbug.com/244921 http/tests/navigation/back-twice-without-commit.html [ Failure Pass ] -# Support for accelerated fixed root backgrounds has not been added. -crbug.com/180885 compositing/fixed-body-background-positioned.html [ Failure ] - # For reference tests, the expected and actual appear to clipped differently. crbug.com/252693 compositing/fixed-background-composited-html.html [ ImageOnlyFailure ]
lgtm2, but you need to update the patch description now that you've split out the tests/etc
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vollick@chromium.org/13462003/103001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vollick@chromium.org/13462003/105004
Message was sent while issue was closed.
Change committed as 153648 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
