[webkit-reviews] review denied: [Bug 106061] [wk2] Remove offscreen tiles from the layer tree : [Attachment 181297] possibly a patch

[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 106061: [wk2] Remove offscreen tiles from the layer tree
https://bugs.webkit.org/show_bug.cgi?id=106061

Attachment 181297: possibly a patch
https://bugs.webkit.org/attachment.cgi?id=181297&action=review

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


> Source/WebCore/page/Settings.cpp:189
> +    , m_aggressiveTileRetentionEnabled(false)

Why isn't m_unparentOffscreenTilesEnabled initialized?

> Source/WebCore/platform/graphics/TiledBacking.h:78
> +    virtual void setAggressiveTileRetentionEnabled(bool) = 0;
> +    virtual bool aggressiveTileRetentionEnabled() const = 0;
> +    
> +    virtual void setUnparentOffscreenTilesEnabled(bool) = 0;
> +    virtual bool unparentOffscreenTilesEnabled() const = 0;
> +    

These sound like setting plumbing. I think they would read better as
setAggressivelyRetainsTiles()
setUnparentsOffscreenTiles() etc.

> Source/WebCore/platform/graphics/ca/mac/TileCache.h:143
> +	   RemoveTilesFromLayerTree = 1 << 2

I see no downside to always unparenting non TCR tiles. Maybe we should just
always do this?

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:320
> +    if (m_isInWindow) {
> +	   scheduleTileRevalidation(0);
> +    } else {
>	   const double tileRevalidationTimeout = 4;
>	   scheduleTileRevalidation(tileRevalidationTimeout);
>      }

This would be better as:
scheduleTileRevalidation(m_isInWindow ? 0 : tileRevalidationTimeout);

> Source/WebCore/platform/graphics/ca/mac/TileCache.mm:452
> +    if (!m_unparentOffscreenTilesEnabled || m_isInWindow)
> +	   revalidationPolicyFlags = CreatePrimaryTiles | PruneSecondaryTiles;
> +    else
> +	   revalidationPolicyFlags = PruneSecondaryTiles |
RemoveTilesFromLayerTree;

This gets simpler if we always remove tiles. CreatePrimaryTiles is only to
avoid making primary tiles in background tabs, right?