Summary: | [GPU Process] Simplify DisplayList::Iterator part 9: Migrate DisplayList::Iterator's API to use DisplayListItem rather than ItemHandle, and update callers | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||
Status: | ASSIGNED --- | ||||||||||||||||
Severity: | Normal | CC: | sam, webkit-bug-importer, wenson_hsieh | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 225579 | ||||||||||||||||
Bug Blocks: | 223560 | ||||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2021-05-09 15:57:36 PDT
Created attachment 428142 [details]
WIP
Created attachment 428145 [details]
Patch for reviewing
Created attachment 428146 [details]
Patch for EWS
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. Created attachment 428159 [details]
Patch for committing
Created attachment 428160 [details]
Patch for committing
Created attachment 428161 [details]
Updated patch for EWS
Myles, you still planning on landing these? Would be great to get them in. (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. |