[Webkit-unassigned] [Bug 70634] Mark GraphicsLayers as opaque when possible

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=70634





--- Comment #45 from Simon Fraser (smfr) <simon.fraser at apple.com>  2011-11-09 17:14:00 PST ---
(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 114066 [details] [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?

Can you use platform/graphics/Region.h instead for this?

> > > 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.

Let's do this in the other bug.
> I will split this bug into two, so we can do the next step and discuss it in a new bug. I'll copy this comment over as well, as I have questions not related to the simpler version. Thanks.

Sounds good.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.