[GPU Process] Simplify DisplayList::Iterator part 7: Migrate ItemBufferReadingClient from ItemHandle to a const Variant&
Created attachment 425469 [details] Patch
<rdar://problem/76678162>
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.
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; ?
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 });
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.
Created attachment 428119 [details] Patch for committing
Created attachment 428120 [details] Patch for committing
Created attachment 428122 [details] Patch for committing
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.
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.