[webkit-reviews] review granted: [Bug 224314] [GPU Process] Simplify DisplayList::Iterator part 7: Migrate ItemBufferReadingClient from ItemHandle to a const Variant& : [Attachment 425469] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 224314: [GPU Process] Simplify DisplayList::Iterator part 7: Migrate
ItemBufferReadingClient from ItemHandle to a const Variant&
https://bugs.webkit.org/show_bug.cgi?id=224314

Attachment 425469: Patch

https://bugs.webkit.org/attachment.cgi?id=425469&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 425469
  --> https://bugs.webkit.org/attachment.cgi?id=425469
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:397
> +	   m_isValid = false;
> +	   if (auto decodedItem = client->decodeItem(startOfData, dataLength,
itemType)) {
> +	       if (safeCopy(decodedItem.value(), ItemHandle {
m_currentBufferForItem }))
> +		   m_isValid = true;
> +	   }

I often prefer the form *decodedItem to the form decodedItem.value().

Not sure I understand the meaning of the word "safe" in this function name.

Since we are setting a boolean, seems like we could stick with a single if
statement instead of two.

> Source/WebCore/platform/graphics/displaylists/DisplayListItemBuffer.h:85
> +bool safeCopy(const DisplayListItem& source, ItemHandle destination);

Copying from the left argument into the right argument goes against C tradition
and functions like memcpy and strcpy.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2285
> +using DisplayListItem = Variant<

It’s hard to keep this in "logical" order. I suggest alphabetical order instead
when the list is this long. Like includes, with conditional sections at the
bottom also in alphabetical order.