[webkit-reviews] review granted: [Bug 132667] [iOS WebKit2] Flush RemoteLayerBackingStore contexts on a secondary queue : [Attachment 231040] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Horton <thorton at apple.com>'s
request for review:
Bug 132667: [iOS WebKit2] Flush RemoteLayerBackingStore contexts on a secondary
queue
https://bugs.webkit.org/show_bug.cgi?id=132667

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231040&action=review


>> Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:360
>> +	m_frontContextPendingFlush = nullptr;
> 
> This can just be
> 
> RetainPtr<CGContextRef> frontContextPendingFlush =
std::move(m_frontContextPendingFlush);
> 
> or even
> 
> auto frontContextPendingFlush = std::move(m_frontContextPendingFlush);

Can it be this?

    return std::move(m_frontContextPendingFlush);

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h:108
> +	   BackingStoreFlusher(IPC::Connection*,
std::unique_ptr<IPC::MessageEncoder>, Vector<RetainPtr<CGContextRef>>);

I’m surprised to see both a public constructor and a create function.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h:118
> +	   std::atomic<bool> m_hasFlushed;

I don’t fully understand the use of std::atomic here. It doesn’t seem
sufficient for the cross-thread synchronization I expect would be needed. Maybe
it’s just helping the RELEASE_ASSERT work reliably?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:245
> +    RELEASE_ASSERT(!m_pendingBackingStoreFlusher ||
m_pendingBackingStoreFlusher->hasFlushed());

Wow, RELEASE_ASSERT. We are getting more liberal with those lately.

What guarantees that the backing store flusher has flushed at this point?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:322
> +    return adoptRef(new
RemoteLayerTreeDrawingArea::BackingStoreFlusher(connection, std::move(encoder),
contextsToFlush));

Would a std::move(contextsToFlush) help efficiency here?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:328
> +    , m_contextsToFlush(contextsToFlush)

Would a std::move(contextsToFlush) here help efficiency?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:329
> +    , m_hasFlushed(false)

Is this needed? Doesn’t an std::atomic<bool> automatically get initialized to
false?

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:339
> +    for (auto& context : m_contextsToFlush)
> +	   CGContextFlush(context.get());
> +
> +    m_connection->sendMessage(std::move(m_commitEncoder));
> +    m_hasFlushed = true;

This should either have:

    ASSERT(!m_hasFlushed);

or

    if (m_hasFlushed)
	return;

Or maybe there’s some good reason neither is needed.