[webkit-reviews] review denied: [Bug 82117] [chromium] Layers with animating transforms should prepaint even if they are not visible yet : [Attachment 133618] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 82117: [chromium] Layers with animating transforms should prepaint even if
they are not visible yet
https://bugs.webkit.org/show_bug.cgi?id=82117

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

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


> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:620
> -    if (m_tiler->isEmpty())
> +    if ((m_tiler->isEmpty() || layerRect.isEmpty()) &&
!drawTransformIsAnimating() && !screenSpaceTransformIsAnimating())
>	   return;

I think this conditional is wrong, possibly due to m_tiler->isEmpty() being
confusing.  I think if !m_tiler->numTiles() (i.e. the bounds are zero), then we
definitely need to bail out even if we're animating, because the layer size is
zero.  I'm pretty sure layerRectToTileIndices will assert at you if that's the
case.

Can you add a test involving prepareToUpdate/prepareToUpdateIdle where there's
a layer with empty bounds?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:672
> +    if ((m_tiler->isEmpty() || layerRect.isEmpty()) &&
!drawTransformIsAnimating() && !screenSpaceTransformIsAnimating())
> +	   return false;

This should be changed to match the above.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:696
> +    if (visibleLayerRect.isEmpty() && (drawTransformIsAnimating() ||
screenSpaceTransformIsAnimating()))
> +	   return IntRect(IntPoint(), contentBounds());

I'm not so sure about this, since returning the whole bounds means that it's
going to try to prepaint the entire layer in one go.  Current behavior is that
if we OOM during prepainting, we don't do any prepainting, but if it happens to
fit and is really large, that could be a huge hitch while you wait for all of
that to paint, with no guarantee it will fit.  Additionally, we prepaint in
front-to-back order, so prepainting a large animated layer would take
precedence over a scrollable layer, which also seems problematic.

This kind of feels like a stopgap solution, where in some ideal world you could
predict which tiles will eventually be visible during the animation,
prioritizing the ones that become visible first and filtering out the ones that
will never be visible.	Maybe I'd prefer that this solution be more
conservative, possibly by creating a rect of n tiles (maybe ~3ish?), and
letting prepaint rect growth take care of reserving more if there's not more
memory contention from other layers.  For small layers, we'll still get them
prepainted quickly, but larger layers have less of a chance of causing
problematic behavior.