Bug 219007

Created attachment 414293 [details]
Patch

Comment on attachment 414293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414293&action=review

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:187
> +    void commitFlushDisplayList(WebCore::DisplayList::FlushIdentifier flushIdentifier) override

We need to stop using "commit" and "flush" in ways we don't all understand.

IMO (but obv. up for discussion):

"submit" for "send display list/work to GPUP", like Wenson fixed elsewhere
"flush" is "wait till all submitted work has actually been completed"
"commit" should be just for the layer tree transactions (but is effectively the same as "submit" for a transaction)

and no combination of the above means anything so we shouldn't see them adjacent to each other :)

Created attachment 414861 [details]
Patch

Created attachment 414879 [details]
Patch

Created attachment 414880 [details]
Patch

Created attachment 414885 [details]
Patch

<rdar://problem/71747762>

Created attachment 414912 [details]
Patch

Created attachment 414935 [details]
Patch

Comment on attachment 414935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414935&action=review

> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:297
> +    std::unique_ptr<ImageBufferBackend::Parameters> m_parameters;

Why not just store a copy of the tiny struct? Much simpler.

> Source/WebCore/platform/graphics/ImageBufferBackend.h:166
> +    const Parameters& m_parameters;

Who owns the original data? Would be safer to just store by value.

Created attachment 415043 [details]
Patch

Created attachment 415045 [details]
Patch

Created attachment 415046 [details]
Patch

Created attachment 415050 [details]
Patch

ImageBufferBackend::Parameters are saved by value in ImageBufferBackend and ConcreteImageBuffer. Renaming the flush/commit/flush functions was removed from this patch and will be addressed in a different patch.

Created attachment 415395 [details]
Patch

Comment on attachment 415395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415395&action=review

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:124
> +    auto backendSize = this->backendSize();

Doesn't seem worthwhile storing this in a local.

> Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.cpp:108
> +    return { static_cast<int>(CGBitmapContextGetWidth(context().platformContext())), static_cast<int>(CGBitmapContextGetHeight(context().platformContext())) };

Let's not fetch context().platformContext() twice.

Created attachment 415434 [details]
Patch

Created attachment 415445 [details]
Patch

Comment on attachment 415395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415395&action=review

>> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:124
>> +    auto backendSize = this->backendSize();
> 
> Doesn't seem worthwhile storing this in a local.

Actually it is worthy. sinkIntoNativeImage() destroys the backend. So we can't call backendSize() after calling sinkIntoNativeImage(). I put it back and I added a comment.

Committed r270458: <https://trac.webkit.org/changeset/270458>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415445 [details].