[webkit-reviews] review granted: [Bug 225579] [GPU Process] Simplify DisplayList::Iterator part 8: Replace the guts of DisplayList::Iterator without affecting its public API : [Attachment 428128] Patch for review

[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 225579: [GPU Process] Simplify DisplayList::Iterator part 8: Replace the
guts of DisplayList::Iterator without affecting its public API
https://bugs.webkit.org/show_bug.cgi?id=225579

Attachment 428128: Patch for review

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




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

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

> Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:114
> +inline auto DisplayList::Iterator::currentItem() const ->
Optional<CurrentItemResult>

This inline is unexpected. What is the intent of it?

> Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:116
> +    using Sequence = brigand::make_sequence<brigand::ptrdiff_t<0>,
WTF::variant_size<DisplayListItem>::value>;

I think adding a helper type or renaming Sequence, would make it more clear
what this is attempting to do. 

I believe what this is doing is making an integer sequence from 0 to
WTF::variant_size<DisplayListItem>, but what you really want here is a type
list of the all the types in the DisplayListItem Variant.

I think we can do this more directly, and in a way that will be more useful for
other uses of iterating all the types here, by just using a type list directly.
I am not entirely sure, but I think you might be able to just use
DisplayListItem directly in for_each, since it has a type list as part of its
type. e.g.

    brigand::for_each<DisplayListItem>([&](auto&& type) {
	using DisplayListItemType = decltype(type);

       ...


Let me know if that works. If it doesn't, there are a few other ways we can
convert the Variant<...> into a working brigand::list<...>.

> Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:119
> +    Optional<CurrentItemResult> result;
> +    brigand::for_each<Sequence>([&](auto&& type) {

At some point I want to make a version of this that allows returning a value.

> Source/WebCore/platform/graphics/displaylists/DisplayListIterator.cpp:125
> +	   if (m_cursor + 1 > endOfCurrentBuffer)

In general, I think this code could use a few more comments and/or more
explicit named constants explaining what the specific lines are doing. Parsing
is hard, so explaining why a + 1, or sizeof(uint64_t) is there is going to help
future readers.