Closed Bug 1581448 Opened 7 months ago Closed 7 months ago

Add support for specifying extra per-primitive flags in WR display lists

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: gw, Assigned: gw)

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → gwatson

This patch replaces the is_backface_visible bool in the common
per-primitive data in the display list with a PrimitiveFlags
enumeration. This will allow Gecko to specify extra information
about certain primitive, such as tagging scroll bars.

It would be very useful for the WR picture caching code to know which primitives are part of scrollbars. This patch adds a bitfield to the common information about a primitive, to allow marking a primitive as part of a scrollbar.

I briefly discussed this on IRC with Matt, who said that Gecko has the information needed to provide these annotations and pointed me to where to look in the Gecko code. But I got a bit lost, I don't know the Gecko code very well at all.

Questions:

  • Does this API seem suitable for allowing Gecko to provide these annotations?
  • If so, and once this lands, would someone who knows the Gecko DL code be able to add the Gecko-side parts to set the flag on scrollbar primitives appropriately?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(jmuizelaar)

I'm going to redirect my ni to mstange as he can probably give a better answer with less investigation. If we don't here from him or Matt (it's TPAC this week) than I'll look closer.

Flags: needinfo?(jmuizelaar) → needinfo?(mstange)

Glenn, just to make sure, you want this piece of information on the scrollbar thumb, not the scrollbar track, right? Also, can you describe why it is useful for WR to be able to treat the scrollbar thumb differently from something that has an async transform animation? (The scrollbar thumb already has an animated transform property ID.)

The nsDisplayOwnLayer item we create for scrollbar thumbs returns true from nsDisplayOwnLayer::IsScrollThumbLayer(). I think the information would need to be propagated from this section in nsDisplayOwnLayer::CreateWebRenderCommands to where we create the WR primitive for the stacking context. Maybe by storing the return value of IsScrollThumbLayer() in a new field in the wr::StackingContextParams (and a corresponding new field in StackingContextHelper), and then passing it as parameters through DisplayListBuilder::PushStackingContext and wr_dp_push_stacking_context.

Flags: needinfo?(mstange)
Flags: needinfo?(matt.woodrow)

I was thinking of having it for both the thumb and the track, but just the thumb would probably also be good enough.

There's a couple of reasons:

  1. In the current picture caching code, it'd make it easy for WR to identify that these items shouldn't be part of the cached content slice. This would avoid invalidations occurring in content tiles due to the scroll bar moving. It's currently hard to reliably distinguish these from other fixed position elements in the content at the beginning / end of the display list for an iframe.

  2. In future, I think it might make sense to place the root scroll bar elements into their own OS compositor layer.

If those don't make sense, or you can think of other possible ways to achieve those outcomes, I'd be open to any other suggestions too!

Ok, sounds good. We also have nsDisplayOwnLayer::IsScrollbarContainer() for the entire scrollbar. So I think we need two WR flags, PrimitiveFlags::IS_SCROLLBAR_CONTAINER and PrimitiveFlags::IS_SCROLLBAR_THUMB.

I updated the patch to have those two flags. Thanks for taking a look!

Pushed by gwatson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/af9417bd8105
Add support for specifying extra per-primitive flags in WR display lists r=kvark,nical

Fixed up the patch and updated in phab. Just doing a try run rebased on current autoland to make sure there are no more conflicts:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=deb8e9c6561b3ad6b0d9b3ae2243473da6b2b1b3

Flags: needinfo?(gwatson)
Pushed by gwatson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8bec7810fc09
Add support for specifying extra per-primitive flags in WR display lists r=kvark,nical
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.