[webkit-reviews] review granted: [Bug 77564] Tile cache doesn't have an upper limit : [Attachment 124976] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 77564: Tile cache doesn't have an upper limit
https://bugs.webkit.org/show_bug.cgi?id=77564

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=124976&action=review


> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:941
> +    // Let the client know that it needs to sync the layer state. We use
this to ensure that
> +    // the layout is up to date before the individual tiles are painted.

The second half of the comment is the good part; the first sort of repeats what
the code says and that is not needed. Merging the two sentences might make
things clearer and shorter.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:69
> +	   // Revalidate the tiles right away if we don't have any. This avoids

> +	   // flashing when transitioning from one page to another.

Same thought. The first sentence just says what the code does. A shorter
comment that focused just on the “why” may be possible. Like:

    // We must revalidate immediately instead of using a timer when there are
no tiles to avoid a flash when transitioning from one page to another.

Wording it that way makes it clear there is something else to say. Like
explaining how “no tiles” happens in a page transition.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:262
> +    // FIXME: Figure out the optimal values here.
> +    tileCoverageRect.inflateX(tileCoverageRect.width() / 2);
> +    tileCoverageRect.inflateY(tileCoverageRect.height() * 2);

Another comment should also indicate why you think these are good starting
values. Such as “it’s common to have tall pages and scroll vertically so we
keep more tiles above and below the current area than on the left and right”.
Or whatever that current thinking is.

This code seems to do 5X in the vertical dimension and 2X in the horizontal. Or
a border of 2X on top and bottom and 0.5X on left and right. Your comment in
change log cites different figures.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:274
> +	       [tileLayer removeFromSuperlayer];
> +	       [tileLayer setTileCache:0];

Can tileLayer be deallocated here as a side effect of the call to
removeFromSuperlayer?

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:280
> +    // FIXME: Be more clever about which tiles to remove.

Unclear what more clever means. Maybe you could write a better comment?

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:298
> +	       if (!result.second) {
> +		   // We already have a layer for this tile.
> +		   continue;
>	       }

Somethings I think that if (result.first->second) is clearer than if
(!result.second). I like how it’s closely related to the createTileLayer line
of code. Obviously neither is all that clear until we change the return types
so they use named data members instead of pair/first,second naming.