[webkit-reviews] review denied: [Bug 82251] [chromium] Layers should should know their visibility outside their content bounds : [Attachment 158730] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied Eric Penner
<epenner at chromium.org>'s request for review:
Bug 82251: [chromium] Layers should should know their visibility outside their
content bounds
https://bugs.webkit.org/show_bug.cgi?id=82251

Attachment 158730: Patch
https://bugs.webkit.org/attachment.cgi?id=158730&action=review

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=158730&action=review


It looks like your math is just accumulating Manhattan distance, yeah? I have
no idea how that is supposed to be correct down a layer hierarchy across
transforms.  There aren't any unit tests for layers with scales, or
contentBounds() != bounds(), or deviceScale != 1, or weird projections, and
that lack makes me more skeptical.

I agree with all of Shawn's concerns that an empty IntRect with a valid
position and an empty size with potentially only one non-zero component is a
serious foot gun.  I have no idea whether WebRect or SkRect would continue to
function as IntRect does if we switch to them. 

After looking at this patch in more detail, I don't think that this approach
(that you changed to in comment #24) is more elegant or simple.  I continue to
prefer the previous approach of using an unclipped "visible" content rect.  I
totally agree that case #3 in verifyVisibleRectOutsideContentBounds that you
point out in comment #26 is not solved by this approach, but I am ok with that.
 I would rather have an elegant 90% solution than a brittle 100% one.  I can't
think of a use case with scrolling or animation that would use multiple clip
rects like this.  Also, as I see it, layer #3 is *infinite* distance away to
visible--there is no distance that it can move such that it will be visible.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:145
>>> +static void transformVisibleContentRectForLayer(LayerType* layer)
>> 
>> I'm not quite understanding why this new name is more appropriate?	 the
"ForLayer" suffix seems nice, but the "transform" part is what I don't
understand.  Seems to me the clamping/intersecting is more important than the
transforming, the transforming is just done as a detail so that rects in
different spaces can be converted to the same space for correct
intersection/clamping, right?
> 
> I added it since there is now an intermediate value in the
visibleContentRect() that get's transformed, but maybe the name should stay as
calculate.
> 
> And now that I think of it, maybe it's worth biting the bullet and storing a
totally separate intermediate rect rather than 're-using' the
visibleContentRect. Since some layers don't end up in the iterator, those
layers get bogus values since this calculation wasn't finished... Although
maybe that's fine since those layers are culled from all further calculations
anyway.

By "don't end up in the iterator", you're talking about layers (or subtrees)
that don't end up in a render surface layer list due to layerShouldBeSkipped? I
think that's probably ok.  The only case in that function that affects this
distance calculation is backfacing layers and if they're animating, we consider
them visible already.  Those layers won't be considered for painting or texture
prioritization at all and so all their textures (if they have any) will get
reset to the lowest priority, which seems like the right thing to do anyway. 
This is a long-winded way to say that I agree with you.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:732
> +    layer->setVisibleContentProxyRect(visibleContentProxyRectOfLayer,
accumulatedDistanceToVisible);

visibleContentProxyRectOfLayer is in the layer's content space.  Here, you are
setting it to rectInTargetSpace, which is in target space.  What's going on
with that?