Bug 1503447 (clipping-rewrite2)

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

RESOLVED FIXED in Firefox 65

Status

()

P2
normal
RESOLVED FIXED
3 months ago
15 days ago

People

(Reporter: kvark, Assigned: kvark)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

3 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

3 months ago
Assignee: nobody → kvark
Status: NEW → ASSIGNED
Blocks: 1499015
Blocks: 1504423
(Assignee)

Comment 1

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

Comment 2

2 months ago
Created attachment 9024852 [details]
Remove Nothing() semantic from ASR overrides

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

2 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

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

2 months ago
Created attachment 9025880 [details]
Remove the clip->ASR == item->ASR hacky code path

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

2 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

2 months ago
Created attachment 9027923 [details]
Always use zero origin for WR reference frames and stacking contexts

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

2 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
Created attachment 9028441 [details]
Bug 1503447 - Follow-up to fix reftest annotation. r?kvark

Comment 13

2 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
Created attachment 9028524 [details]
Bug 1503447 - Follow-up to fix generated FFI header. r?kats

Comment 15

2 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

a month ago
Created attachment 9033019 [details]
New WebRender ClipId/SpatialId API.

Port to separate SpatialId from ClipId in Webrender API

Landed in bug 1518605

Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago15 days ago
Resolution: --- → FIXED
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.