Bug 224314 – [GPU Process] Simplify DisplayList::Iterator part 7: Migrate ItemBufferReadingClient from ItemHandle to a const Variant&

• Component: New Bugs
• Status: NEW

[Myles C. Maxfield]

[GPU Process] Simplify DisplayList::Iterator part 7: Migrate ItemBufferReadingClient from ItemHandle to a const Variant&

[Myles C. Maxfield]

Created attachment 425469 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/76678162>

[Darin Adler]

Comment on attachment 425469 [details]
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.

[Myles C. Maxfield]

Comment on attachment 425469 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:397
>> +        }
> 
> 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.

Do you mean like

if (auto decodedItem = client->decodeItem(startOfData, dataLength, itemType); safeCopy(decodedItem.value(), ItemHandle { m_currentBufferForItem }))
    m_isValid = true;

?

[Darin Adler]

Comment on attachment 425469 [details]
Patch

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

>>> Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:397
>>> +        }
>> 
>> 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.
> 
> Do you mean like
> 
> if (auto decodedItem = client->decodeItem(startOfData, dataLength, itemType); safeCopy(decodedItem.value(), ItemHandle { m_currentBufferForItem }))
>     m_isValid = true;
> 
> ?

I actually meant this:

    if (auto decodedItem = client->decodeItem(startOfData, dataLength, itemType))
        m_isValid = safeCopy(*decodedItem, ItemHandle { m_currentBufferForItem });

[Myles C. Maxfield]

Comment on attachment 425469 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:395
> +            if (safeCopy(decodedItem.value(), ItemHandle { m_currentBufferForItem }))

I need to add a comment around this copy operation saying that it's temporary and will be removed in a later patch.

[Myles C. Maxfield]

Created attachment 428119 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 428120 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 428122 [details]
Patch for committing

[Myles C. Maxfield]

I'm going to wait to land this until https://bugs.webkit.org/show_bug.cgi?id=225579 is ready, so I can land them both together. This patch introduces an extra copy that will end up getting deleted in that patch, and I don't want to cause a regression.

[Myles C. Maxfield]

Comment on attachment 428122 [details]
Patch for committing

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

> Source/WebCore/platform/graphics/displaylists/DisplayListItemBuffer.cpp:629
> +        using I = typename WTF::RemoveCVAndReference<decltype(type)>::type::type;

We can apply the treatment from https://bugs.webkit.org/show_bug.cgi?id=225579#c8 here.