[webkit-reviews] review denied: [Bug 93305] Add optional debug logging for tiled scrolling : [Attachment 157298] style

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 93305: Add optional debug logging for tiled scrolling
https://bugs.webkit.org/show_bug.cgi?id=93305

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

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


> Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:299
> +void ScrollingTreeNodeMac::logExposedUnfilledArea()

This needs a big scarey comment (or an assertion) about being called on the
scrolling thread.

> Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:301
> +    Region paintedTileCoveredVisibleRegion;

Too many adjectives!

> Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:310
> +	   NSArray* sublayers = [[layer sublayers] copy];

You're leaking the array.

> Source/WebCore/page/scrolling/mac/ScrollingTreeNodeMac.mm:312
> +	   for (CALayer* sublayer in sublayers)
> +	       layerQueue.append(sublayer);

Not sure why you descend into sublayers here like this; can't you assume
something about layer tree structure?

> Source/WebCore/platform/graphics/TiledBacking.h:45
> +    bool scrollingPerformanceLoggingEnabled() { return
m_scrollingPerformanceLoggingEnabled; }

const please.

> Source/WebCore/platform/graphics/ca/mac/TileCache.h:72
> +    IntRect visibleRect() { return m_visibleRect; }
> +    unsigned blankPixelCount();

const please.

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:320
> +	   RetainPtr<WebTileLayer>& tileLayer = it->second;

Probably simpler as WebTileLayer* tileLayer = it->second.get()

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:332
> +	   IntRect visiblePart([tileLayer.get() frame]);
> +	   visiblePart.intersect(m_visibleRect);
> +
> +	   if(!visiblePart.isEmpty() && [tileLayer.get() repaintCount])
> +	       paintedTileCoveredVisibleRegion.unite(visiblePart);
> +    }
> +
> +    Region uncoveredRegion(m_visibleRect);
> +    uncoveredRegion.subtract(paintedTileCoveredVisibleRegion);
> +
> +    return uncoveredRegion.totalArea();

This seems similar to the logExposedUnfilledArea() code. Why do you need both?

missing space after if

> Source/WebCore/platform/graphics/ca/mac/WebTileLayer.mm:68
> +- (unsigned)repaintCount
> +{
> +    return _repaintCount;
> +}

Do we waste cycles maintaining _repaintCount even when not showing counters?

> Source/WebCore/platform/graphics/ca/mac/WebTileLayer.mm:75
> +    if([self repaintCount] == 1 && !visiblePart.isEmpty())

Space after if