Bug 225582 – [GPU Process] Simplify DisplayList::Iterator part 9: Migrate DisplayList::Iterator's API to use DisplayListItem rather than ItemHandle, and update callers

• Status: ASSIGNED
• Component: New Bugs

[Myles C. Maxfield]

Migrate DisplayList::Iterator's API to use DisplayListItem rather than ItemHandle, and update callers

[Myles C. Maxfield]

Created attachment 428142 [details]
WIP

[Myles C. Maxfield]

Created attachment 428145 [details]
Patch for reviewing

[Myles C. Maxfield]

Created attachment 428146 [details]
Patch for EWS

[Sam Weinig]

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

[Myles C. Maxfield]

Created attachment 428159 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 428160 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 428161 [details]
Updated patch for EWS

[Radar WebKit Bug Importer]

<rdar://problem/78084300>

[Sam Weinig]

Myles, you still planning on landing these? Would be great to get them in.

[Myles C. Maxfield]

(In reply to Sam Weinig from comment #9)
> Myles, you still planning on landing these? Would be great to get them in.

I am, but they cause a MotionMark regression. I need to figure out why.