[webkit-reviews] review denied: [Bug 100169] We should make TileCache tiles the size of the tile coverage rect when we can't do fast scrolling : [Attachment 170455] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 100169: We should make TileCache tiles the size of the tile coverage rect
when we can't do fast scrolling
https://bugs.webkit.org/show_bug.cgi?id=100169

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=170455&action=review


> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:59
> +    , m_tileSize(IntSize(defaultTileCacheWidth, defaultTileCacheHeight))

You shouldn't need the IntSize() there.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:317
> +    // If the m_tileSize and clampedRect.size() are the same in either
dimension, then we
> +    // don't want to overdraw, so explicity set x/y to 0 in that case.
> +    if (clampedRect.maxX() !=  m_tileSize.width())
> +	   bottomRight.setX(max(clampedRect.maxX() / m_tileSize.width(), 0));
> +    else
> +	   bottomRight.setX(0);
> +
> +    if (clampedRect.maxY() !=  m_tileSize.height())
> +	   bottomRight.setY(max(clampedRect.maxY() / m_tileSize.height(), 0));
> +    else
> +	   bottomRight.setY(0);

Can't this be done with ceil()? or floor(), since presumably the issue occurs
at other integral boundaries too.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:349
> +void TileCache::adjustTileSizeForCoverageRect(const IntRect& coverageRect)
> +{
> +    if (m_tileCoverage == CoverageForVisibleArea) {
> +	   m_tileSize = coverageRect.size();
> +	   return;
> +    }
> +
> +    m_tileSize = IntSize(defaultTileCacheWidth, defaultTileCacheHeight);
> +}

I think it would be clearer to make this  IntSize tileSizeForCoverageRect(const
IntRect&) const. That way the changing of m_tileSize isn't hidden from the
caller.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:441
> +    int numTiles = 0;
>      for (int y = topLeft.y(); y <= bottomRight.y(); ++y) {
>	   for (int x = topLeft.x(); x <= bottomRight.x(); ++x) {
> +	       numTiles++;
>	       TileIndex tileIndex(x, y);

You don't seem to be using numTiles.