[Webkit-unassigned] [Bug 71972] Consider all RenderObjects in a GraphicsLayers when marking opaque
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 9 17:06:52 PST 2011
https://bugs.webkit.org/show_bug.cgi?id=71972
Dana Jansens <danakj at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |backer at chromium.org,
| |enne at google.com,
| |jamesr at chromium.org,
| |nduca at chromium.org,
| |piman at chromium.org,
| |simon.fraser at apple.com,
| |wjmaclean at chromium.org
Depends on| |70634
--- Comment #1 from Dana Jansens <danakj at chromium.org> 2011-11-09 17:06:52 PST ---
(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()?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list