[webkit-reviews] review granted: [Bug 131213] [iOS WebKit2] Mark back buffers purgeable when they're not being painted frequently : [Attachment 228581] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 131213: [iOS WebKit2] Mark back buffers purgeable when they're not being
painted frequently
https://bugs.webkit.org/show_bug.cgi?id=131213

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

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


> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.h:126
> +    std::chrono::steady_clock::time_point m_lastDisplayTime;

std::so::verbose

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:373
> +    if (markVolatile)
> +	   surface->setIsVolatile(true);
> +    else if (surface->setIsVolatile(false) ==
IOSurface::SurfaceState::Empty)
> +	   return false;

if (markVolatile) {
  surface...
  return true;
}
...

Or do the "return false" test first.

Also the two booleans (markVolatile and the argument to setIsVolatile) make
this code very confusing.

How about:

bool result = surface->setIsVolatile(markVolatile);
if (!markVolatile && !result)
  return false;

It would be clearer if you used an enum markVolatile/markNonVolatile.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:402
> +	   if (!applyVolatilityToSurface(m_frontSurface.get(), false))
> +	       setNeedsDisplay();
> +	   applyVolatilityToSurface(m_backSurface.get(), false);
> +	   break;
> +    case Volatility::BackBufferVolatile:
> +	   if (m_backSurface && m_backSurface->isInUse())
> +	       return false;
> +	   if (!applyVolatilityToSurface(m_frontSurface.get(), false))
> +	       setNeedsDisplay();
> +	   applyVolatilityToSurface(m_backSurface.get(), true);
> +	   break;
> +    case Volatility::AllBuffersVolatile:
> +	   if (m_frontSurface && m_frontSurface->isInUse())
> +	       return false;
> +	   if (m_backSurface && m_backSurface->isInUse())
> +	       return false;
> +	   applyVolatilityToSurface(m_frontSurface.get(), true);
> +	   applyVolatilityToSurface(m_backSurface.get(), true);

false, false, false, true, true!

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.h:59
> +private:
> +    HashSet<RemoteLayerBackingStore*> m_liveBackingStore;
> +
> +    void markInactiveBackingStorePurgeable();
> +
> +    RemoteLayerTreeContext* m_context;
> +
> +    WebCore::Timer<RemoteLayerBackingStoreCollection> m_purgeabilityTimer;

Would prefer the member variables be grouped together.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeContext.h:82
> +    std::unique_ptr<RemoteLayerBackingStoreCollection>
m_backingStoreCollection;

Why not just have it by value?