New bug to consider next steps on bug #70634
(Comment stolen from bug #70634) > (From update of attachment 114066 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114066&action=review > > I don't like that this patch is getting more and more complex. I think you should start by getting the easy cases working first (e.g. the layer's RenderObject is known to be opaque), and then progress to doing RenderObject walks in later patches. The IntRectEdge code is a large amount of new code. It should be tested on all platforms, not just Chromium, but I'd prefer that it just not be included in the initial patch. I was starting to feel the same way, and I'll be happy to do so. I feel I should clarify what the patch will lose though in the process. The IntRectEdge code was used to determine if the RenderLayers in a backing together would make it opaque. Removing that will revert the check to this back to "is there a single opaque RenderLayer that fills the backing?" I think the inconsistency I wanted to pointed here is that the IntRectEdge code has less to do with walking the RenderObject tree, and more to do with considering all the layers in a backing together (eventually all the RenderObjects in a backing). Regarding testing: The IntRectEdge code will be tested by LayoutTests across all platforms. But I wanted a unit test also for the unionOfRectsCoverARect logic, especially while I was writing it. I would write unit tests for all platforms if such a general mechanism existed, but to my knowledge it does not? > > > Source/WebCore/rendering/RenderBoxModelObject.cpp:2693 > > + const LayoutRect& bounds = borderBoundingBox(); > > + > > + // We do not consider overflow bounds here, and if we get a query rect on the overflow area for the > > + // render object (i.e. outside the border bounds), then we just return false. This does not mean the area > > + // in the rect is neccessarily non-opaque, and may be a false negative. > > + // NOTE: Because of this, we don't need to look for style()->boxShadow(), since that is always outside of the border > > + // bounds. > > + if (!bounds.contains(rect)) > > + return false; > > + > > + // FIXME: Check border-image. With 'fill' it can be drawn behind the contents area and could be opaque. > > + // Currently we check background color only, but border-image could make the rect opaque even though the > > + // background color is not. > > + > > + const Color color = style()->visitedDependentColor(CSSPropertyBackgroundColor); > > + if (!color.isValid() || color.hasAlpha()) > > + return false; > > + > > + return true; > > This is contrary to the direction I expressed a preference for. I think the methods should default to returning false, and return true only in cases they know they can give the correct answer for. Understood, I will change how these are written. > > Source/WebCore/rendering/RenderBoxModelObject.cpp:2740 > > + return true; > > Ditto. > > > Source/WebCore/rendering/RenderImage.cpp:432 > > + return true; > > Ditto. > > > Source/WebCore/rendering/RenderLayer.h:794 > > + // This is a mapping from RenderObject* to IntRect, which is the opaque rect for each RenderObject > > + // belonging to this RenderLayer. > > + HashMap<const RenderObject*, IntRect> m_opaqueRectForRenderObject; > > + > > + // This list contains bounding box rectangles of opaque RenderObjects in the layer. > > + OwnPtr<Vector<IntRectEdge> > m_opaqueRectsList; > > It seems wrong to be adding these members to RenderLayer, when there's no contract about who updates them. A caller has no way to know if opaqueRects() will be return a stale list. There's just an implicit contract that RenderLayerBacking will update the rects at the right time. - The set of opaque rects is contained in the RenderLayer, since we already walk RenderLayers for any given backing, and it is easier to tell which Layer a RO belongs to, than which backing. - The /set/ of opaque rects in a RenderLayer is updated by each RenderObject. When the RenderObject makes a change to its opaque rect, the list is marked dirty (by being deleted). - The list of opaque rects is rebuilt by RenderLayer whenever the list is requested, and the previous list was dirtied. Thus, the backing does not actually update the list explicitly, it just queries on the RenderLayer, who builds the list if required and returns it. And a dirty list is never returned. I don't particularly like this approach though, either. Is there another RO tree walk that could be considered? The previous patch was a lot cleaner. > > Source/WebCore/rendering/RenderLayerCompositor.cpp:474 > > + LayoutPoint ancestorRelOffset; > > + layer->convertToLayerCoords(ancestorLayer, ancestorRelOffset); > > Have you tested composited layers that contain 2d-transformed layers? It seems like this convertToLayerCoords() could cross a transform boundary, which is bad. Will look at this, thank you. > > Source/WebCore/rendering/RenderObject.cpp:2703 > > +void RenderObject::updateOpaqueRect() > > I don't know why this has to be on RenderObject, since it's a layer thing. Each RO has an opaque Rect (its entire area or an empty rect at the moment). This function mostly was added to not interact with the RenderLayer class in the .h file as that would need another include. It's purpose is to determine if "this" is opaque and update its opaque rect in the RenderLayer's set. > > Source/WebCore/rendering/RenderObject.h:1004 > > + updateOpaqueRect(); > > This is going to be way too expensive. This is a hot function (hence the inline). I had the impression that the function is especially hot when b==true (marking as needing layout). Is that incorrect? With b==false, the function is called once per layout(). And if the updating is to be done in the layout() tree walk, then it needs to be done exactly this many times. As above, is there a better tree walk to consider than layout()?
(In reply to comment #1) > As above, is there a better tree walk to consider than layout()? So I'd really like to know where you think this should be going next. Things I have explored so far: 1. Walk the ROs associated with a RL when the RL is visited in RenderLayerCompositor::calculateCompositedBounds. I liked this one most as it was modular and clean, and didn't touch any other code. However it requires 1 new visit to each RenderObject during the computation. I also have the intuition that calculateCompositedBounds() is not called very frequently, so the computation of the opaque flag will be done more or less when it is necessary, and the extra overhead will not have too much impact. 2. Save the opaque rect for each RO whenever it finishes a layout(). Collect them rects in the RL, so they can be grabbed from each RL that is visited in RenderLayerCompositor::calculateCompositedBounds. This doesn't require another trip through the ROs. But it does require looking at something for each opaque RO belonging to the RL. If everything is opaque, it ends up having the same cost as option 1.** This code was much more complex and invasive, the layout() function can be called quite frequently, and can be called for arbitrary positions in the RenderObject tree, requiring extra walks up to find its position relative to the RenderLayer. We could build a Region in the RL, which would save us the cost of **, however this would require visiting the entire RO subtree for a RL whenever something in the subtree was changed. Thus this option seems to have a cost >= the cost of option 1. 3. You mentioned the painting tree-walk, but that is too late I believe, as we want these opaque flags set before painting to make decisions during the painting process. Painting is done bottom to top, and the opaque flag depends on the entire RL/RO subtree of a backing, which would require an extra visit to each RO during the painting tree walk. 4. Is there another walk you'd like to consider? I am only familiar with the code as far as I've been touching it in these CLs. Where do you think this should go? Thanks.
Created attachment 116265 [details] Patch
Here's how I think we could approach considering all RenderObjects in a RLBacking. The key ideas behind the code are: 1) RO::layout() is problematic to hop on, since it does partial tree walks. To build the full Region requires walking through a Rect for each RO anyway. 2) The RL's opaque region should depend on its scroll offset, and change when a scroll happens. 3) I chose RL::computeRepaintRects() to compute the opaque region since it is executed after scrolling, and recomputes various other painting bounds. 4) It walks the tree of ROs for a single RL. Considers only ROs that are visible after scrolling. Constructs a Region from the opaque ones. If the box decorations area is opaque but contents area is not, it also checks if ROs in its subtree fill its contents area. Includes a LayoutTest with scrolling to verify the code is working, and tests for RO children which are divs or images.
Created attachment 116416 [details] Patch
Dropping this as bug as we're opting for a more simple approach to marking layers opaque.