[webkit-reviews] review granted: [Bug 225582] [GPU Process] Simplify DisplayList::Iterator part 9: Migrate DisplayList::Iterator's API to use DisplayListItem rather than ItemHandle, and update callers : [Attachment 428145] Patch for reviewing

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 225582: [GPU Process] Simplify DisplayList::Iterator part 9: Migrate
DisplayList::Iterator's API to use DisplayListItem rather than ItemHandle, and
update callers
https://bugs.webkit.org/show_bug.cgi?id=225582

Attachment 428145: Patch for reviewing

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




--- Comment #4 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 428145
  --> https://bugs.webkit.org/attachment.cgi?id=428145
Patch for reviewing

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

> Source/WebCore/ChangeLog:12
> +	   - DisplayList::append() goes from 134 lines to 3

Feels good, right?

> Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:90
> +    auto visitor = WTF::makeVisitor([&](const SetState& stateItem) -> bool {

I've been encouraging use of the helper WTF::switchOn(variant, ...) which wraps
makeVisitor() and just reads a bit nicer in my opinion.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:2385
> +inline constexpr bool isDrawingItem(const DisplayListItem& displayListItem)
> +{
> +    return WTF::visit([](const auto& displayListItem) {
> +	   return displayListItem.isDrawingItem;
> +    }, displayListItem);
> +}

This will end up being a bit of a big function due to all the cases it needs to
handle so unless it is particularly performance sensitive, I would recommend
moving the implementation out of line. I don't think the constexpr will apply
in most cases as you usually don't have a constexpr DisplayListItem.

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:115
> +template<typename Item>
> +static inline typename std::enable_if_t<!HasApply<Item>, void> apply(const
Item&, GraphicsContext&)

These should go on the same line.

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:120
> +template<typename Item>
> +static inline typename std::enable_if_t<HasApply<Item>, void> apply(const
Item& item, GraphicsContext& graphicsContext)

These should go on the same line.