what's the semantic difference between "intern" and "reuse" here?

we only intern background color items and nothing else?

in what case would an item be re-used but not cached?

what are the guaranteed assumptions about the state of mCachedItemState versus aBuilder?

if aBuilder is required to actually hold the cached items, and it is aware of their indices (parameter here), did you consider making it responsible for knowing which indices are cached (as opposed to having mCachedItemState at all here)?

are we handling a case where this overflows?

this is bumped before the use, should it be initialized to 0 then?

nit: should be a verb

nit: use map_or

should this be merged with InternedCollection by making the space optional?

nit: we could just else {..}

nit: use map_or

nit: rename to with_space_and_clip

doesn't this cache size always end up being 50k?

how does the concept of "collection" apply here? the current_key is set to a concrete key that (as far as I see) can only be associated with one cached item

why do we need this?

should be able to avoid explicit lifetimes here

why are we switching back and forth between the data and the extra data, as opposed to writing everything into the same place?

Good catch, it returns the count of bools.

I had a bit of trouble coming up with consistent naming. Intern here refers to something that can be stored and later reused. Will add comments.

It was an easy item to implement for MVP. I am hoping to make Text item interning work before landing.

isReusedItem here is set if Gecko display list merging reused the item. It is not related to caching, which is why we track the cache state separately. Will add comments.

This was an early optimization to avoid sending SpaceAndClipInfo unnecessarily. Depending on how options are serialized, it might be possible.

If the value of 50k will be hard coded, yes.
This supports growing the cache dynamically. At some point we probably want to set it based on the display list length.

The reduce the cache size. DisplayItem enum is around twice as large as CachedDisplayItem.
Hopefully they can be merged at some point.

I am not a fan of this approach myself, but I think it was the easiest and probably the fastest way to implement two-part display list.
The interned display item data is gathered in a separate buffer, that is appended to actual display item buffer at the end, and later accessed using the extra_data_offset.
Will add comments.

Warning: Constructor does not initialize these fields: mPreviousPipelineId, mCachedItems, mReusedItems, mTotalItems [clang-tidy: cppcoreguidelines-pro-type-member-init]
Checker reliability is medium, meaning that the false positive ratio is medium.

Checker reliability is high, meaning that the false positive ratio is low.
Non-static class member "mTotalItems" is not initialized in this constructor nor in any functions that it calls.
The path that leads to this defect is:

• obj-x86_64-pc-linux-gnu/gfx/webrender_bindings/webrender_ffi_generated.h:625:
  • member_decl: Class member declaration for "mNamespace"..

• gfx/layers/wr/WebRenderCommandBuilder.h:82:
  • uninit_member: Non-static class member field "mPreviousPipelineId.mNamespace" is not initialized in this constructor nor in any functions that it calls..

• obj-x86_64-pc-linux-gnu/gfx/webrender_bindings/webrender_ffi_generated.h:626:
  • member_decl: Class member declaration for "mHandle"..

• gfx/layers/wr/WebRenderCommandBuilder.h:82:
  • uninit_member: Non-static class member field "mPreviousPipelineId.mHandle" is not initialized in this constructor nor in any functions that it calls..

• gfx/layers/wr/WebRenderCommandBuilder.h:150:
  • member_decl: Class member declaration for "mCachedItems"..

• gfx/layers/wr/WebRenderCommandBuilder.h:82:
  • uninit_member: Non-static class member "mCachedItems" is not initialized in this constructor nor in any functions that it calls..

• gfx/layers/wr/WebRenderCommandBuilder.h:150:
  • member_decl: Class member declaration for "mReusedItems"..

• gfx/layers/wr/WebRenderCommandBuilder.h:82:
  • uninit_member: Non-static class member "mReusedItems" is not initialized in this constructor nor in any functions that it calls..

• gfx/layers/wr/WebRenderCommandBuilder.h:150:
  • member_decl: Class member declaration for "mTotalItems"..

• gfx/layers/wr/WebRenderCommandBuilder.h:82:
  • uninit_member: Non-static class member "mTotalItems" is not initialized in this constructor nor in any functions that it calls..

How do you find the new approach? This introduces some extra layering, but I believe it makes things easier to follow.

Warning: Constructor does not initialize these fields: mCachedItems, mReusedItems, mTotalItems [clang-tidy: cppcoreguidelines-pro-type-member-init]
Checker reliability is medium, meaning that the false positive ratio is medium.

Yes, it avoids the possibly costly processing of the display list extra data (in scene::set_display_list), when it would not be needed. I am not aware of any sites where this would happen, but it felt like a trivial and safe optimization to make.

Yes. And I believe that the code is generic enough so that it can also be used to speed up scene building.

I don't believe it is necessary since gecko is the source of truth for the display list, and it decided to send an item with that cache index.