Bug 1503447 (clipping-rewrite2)

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

RESOLVED FIXED in Firefox 65

Status

()

defect
P2
normal
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: kvark, Assigned: kvark)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

7 months ago
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

Updated

7 months ago
Assignee: nobody → kvark
Status: NEW → ASSIGNED
Blocks: 1499015
Blocks: 1504423
Assignee

Comment 1

7 months ago
Marking as P2 since it affects the work of Andrew and Emilio, with thumbs up from Jeff.
Priority: -- → P2
Assignee

Comment 2

6 months ago
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.

Comment 3

6 months ago
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7dc664cc985
Remove Nothing() semantic from ASR overrides r=kats

Comment 4

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a7dc664cc985
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months 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
Assignee

Comment 6

6 months ago
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.

Comment 7

6 months ago
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
Duplicate of this bug: 1499015
Assignee

Comment 10

6 months ago
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.

Comment 11

6 months ago
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

Comment 13

6 months ago
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d4adeee5236
Follow-up to fix reftest annotation. r=kvark CLOSED TREE

Comment 15

6 months ago
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: 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)
Assignee

Comment 19

5 months ago
Port to separate SpatialId from ClipId in Webrender API

Landed in bug 1518605

Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.