Closed Bug 1503447 (clipping-rewrite2) Opened 6 years ago Closed 5 years ago

Tighten up the semantics of clip nodes versus scroll nodes in WebRender API

Categories

(Core :: Graphics: WebRender, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: kvark, Assigned: kvark)

References

Details

Attachments

(6 files)

The clip-scroll-tree in WebRender has seen a long path of evolution, and in the current state it makes WR API unsound with regards to the `ClipId` type, which can refer to either a clip node, a clip chain, or a scroll node.

If WR client provides a `ClipAndScrollInfo` without a clip node, then WR automatically tries to access the clip information associated with the `scroll_node_id`. This is unsound, difficult to reason about, not reflected at the type system, but apparently used by some of WR clients (namely, Gecko and Wrench).

This issue is about finishing the proper separation of clip semantics from the spatial/scroll semantics, which makes the WR API strict and easier to work with.
Assignee: nobody → kvark
Status: NEW → ASSIGNED
Marking as P2 since it affects the work of Andrew and Emilio, with thumbs up from Jeff.
Priority: -- → P2
Previously, the ASR overrides contained Maybe<ClipId>, where Nothing() corresponded to taking the top of the clip/scroll stack instead of overriding. This change removes the associated complexity by ensuring that we always provide the ClipId.
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7dc664cc985
Remove Nothing() semantic from ASR overrides r=kats
https://hg.mozilla.org/mozilla-central/rev/a7dc664cc985
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
This bug is not finished yet, reopening for the rest of the work.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Previously, WebRender ignored clip_node_id on the clip/scroll stack
when pushing clips or reference frames. This got fixed to be more consistent in:
https://github.com/servo/webrender/pull/3315

Now Gecko can use the clip chains generated for display items naturally,
instead of smuggling the last clip through the scroll_node_id, which is what
was happening in this hacky code branch being removed.
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa0c5bc2a99e
Remove the clip->ASR == item->ASR hacky code path r=kats
Alias: clipping-rewrite2
No longer blocks: 1499015
Previously, WebRender was getting a rectangle for reference frames
and stacking contexts, and it had to carefully treat the origin of this rectange:
  - by offseting all the items in a stacking context
  - by negatively compensating the sticky frame scroll port according to the
parent reference frame origin

With this change, we stop providing any non-zero origins. Instead we accomplish
the same behavior using existing API primitives, such as reference frames:
  1. when a stacking context has an origin, we push another reference frame for it
  2. when computing the sticky frame scroll port, we take this origin into account

This slightly simplifies Gecko-WR API, but more importantly it would allow WR to
get rid of this logic (of handling origins), which in turn would allow to switch
the reference frames from push()/pop() model to just define(), like we do for
scroll/sticky frames already.
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e214baf8fc1
Always use zero origin for WR reference frames and stacking contexts r=kats
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d4adeee5236
Follow-up to fix reftest annotation. r=kvark CLOSED TREE
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5ffebdcc014
Follow-up to fix generated FFI header. r=kats
Depends on: 1511024
Depends on: 1511042
Depends on: 1511087
I backed out cset 1e214baf8fc1 and the two followups here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9358169404d1

For introducing regressions (bug 1511042 and dupes for correctness, bug 1511024 for perf)
Port to separate SpatialId from ClipId in Webrender API

Landed in bug 1518605

Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.