1409856 - Clean up coordinate system units in WR-related code

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

Now that we've ditched layers-full WR, pretty much everything should be in LayoutDevice coordinate space. We should clean up the APIs and helpers to ensure this.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a88efd975e7fa287322d5d7e7d5fbddbf905f611
WR tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6cdda28d9acdfeb2134a1c304f9ba3b62587240

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Redid those pushes on autoland tip since central is pretty stale

Builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b81cf368e63a30c9564fabb2f0591e227de47aa8
Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=534c87ac8f2c6d4f9eed36ebbb02185ee5b79edb

Comment 3

[Aria Beingessner [:Gankra]]

Is there any background I could read on this?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I wrote a blog post a while back that might help: https://staktrace.com/spout/entry.php?id=800

Also, comments in layout/base/Units.h

Comment 5

[Aria Beingessner [:Gankra]]

Right so according to that post, LayoutDevice != Layer, but a bunch of the diff you're testing is just replacing one with the other? How does that work?

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Mostly because we did it wrong before :) Pretty much all of the values we are computing are actually in LayoutDevice space because we compute them from various layout properties, we just mislabeled them. Back when we had layers-full code we actually had things in layers space and so we made the APIs like StackingContextHelper::ToRelativeLayoutRect and wr::ToLayoutPoint take both. But then it looks like a bunch of layers conversions got cargo culted around into layout code which should have just kept things in LD coords. This undoes the mislabeling.

Comment 7

[Aria Beingessner [:Gankra]]

Ah perfect, that's exactly what I was hoping to hear!

I imagine we never noticed anything since the difference is (still?) only visible on mobile.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Alexis Beingessner [:Gankro] from comment #7)
> I imagine we never noticed anything since the difference is (still?) only
> visible on mobile.

Yeah. And in most cases we are actually providing the right values. There's just a couple of places in the APZ code where we might not be, and so I left those as having some ToUnknown()/FromUnknown() stuff.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1409856 - Update all the WebRender code to use LayoutDevice units instead of Layer units.

Review commit: https://reviewboard.mozilla.org/r/190892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190892/

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8919945 [details]
Bug 1409856 - Update all the WebRender code to use LayoutDevice units instead of Layer units.

https://reviewboard.mozilla.org/r/190892/#review196092

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8919945 [details]
Bug 1409856 - Update all the WebRender code to use LayoutDevice units instead of Layer units.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190892/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6600dc827ccf
Update all the WebRender code to use LayoutDevice units instead of Layer units. r=jrmuizel

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6600dc827ccf