[webkit-reviews] review denied: [Bug 133020] [wk2] Enable aggressive tile retention and purgeable front buffers on iOS : [Attachment 231755] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Tim Horton
<thorton at apple.com>'s request for review:
Bug 133020: [wk2] Enable aggressive tile retention and purgeable front buffers
on iOS
https://bugs.webkit.org/show_bug.cgi?id=133020

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

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


> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:389
> +	       if (isVolatile)
> +		   m_frontBuffer.surface->clearGraphicsContext();

I was confused briefly by the "clearGraphicsContext" terminology, interpreting
it as "paint the clear color into the graphics context", but it's really
removeGraphicsContext() right?

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:61
> +    if (!m_unparentedBackingStore.contains(backingStore))
> +	   return;
> +    m_liveBackingStore.add(backingStore);
> +    m_unparentedBackingStore.remove(backingStore);

Can't this be done with just one hash lookup in
m_unparentedBackingStore.contains?

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:72
> +    if
(!backingStore.setBufferVolatility(RemoteLayerBackingStore::BufferType::Seconda
ryBack, true))
> +	   successfullyMadeBackingStoreVolatile = false;
> +    if
(!backingStore.setBufferVolatility(RemoteLayerBackingStore::BufferType::Back,
true))
> +	   successfullyMadeBackingStoreVolatile = false;
> +    if (!backingStore.layer()->superlayer()) {

Would prefer blank lines between these statements.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:100
> +    if (m_unparentedBackingStore.contains(backingStore))
> +	   return;
> +    m_unparentedBackingStore.add(backingStore);

Same.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:118
> +	   successfullyMadeBackingStoreVolatile |=
markBackingStoreVolatile(*backingStore, now);

Don't you want &= ?  successfullyMadeBackingStoreVolatile starts off true so |=
won't do anything.

> Source/WebKit2/Shared/mac/RemoteLayerBackingStoreCollection.mm:121
> +	   successfullyMadeBackingStoreVolatile |=
markBackingStoreVolatileImmediately(*backingStore);

Same.

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:106
> +    // Drop the contents of any layers which were unparented; the Web
process will re-send
> +    // the backing store in the commit that reparents them.
> +    for (auto& layerOrView : m_layers.values()) {
> +	   CALayer *layer = asLayer(layerOrView.get());
> +	   if (layer.superlayer)
> +	       continue;
> +	   layer.contents = nullptr;
> +    }

How should we handle disconnected subtrees? Or do we only care about this for
tiles? Seems odd to consider all layers here when it's really just about tiles.