Hookup proper scale selection (ChooseScaleAndSetTransform)

RESOLVED FIXED in Firefox 66

Status

()

P3
normal
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: jrmuizel, Assigned: tnikkel)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 unaffected, firefox66 fixed)

Details

(Whiteboard: [wr-reserve[retest])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
Once we have https://github.com/servo/webrender/issues/1542 we'll need to the transforms properly. We'll probably want to reuse: ChooseScaleAndSetTransform from FrameLayerBuilder.cpp
(Reporter)

Comment 1

a year ago
Having this in place would also help with bug 1415326
(Reporter)

Updated

a year ago
Blocks: 1415326
Whiteboard: [wr-mvp] [triage]
Blocks: 1386669
status-firefox57: --- → unaffected
status-firefox58: --- → unaffected
Priority: -- → P2
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
Priority: P2 → P3
Whiteboard: [wr-mvp] → [wr-reserve]
(Reporter)

Comment 2

7 months ago
We need to determine how much of an issue this is still.
Flags: needinfo?(jmuizelaar)
Assigning to Jeff to reflect the NI.
Assignee: nobody → jmuizelaar
Whiteboard: [wr-reserve] → [wr-reserve[retest]
Flags: needinfo?(jmuizelaar)
Hey Jeff, just giving you a fresh needinfo so I get bugmail when you reply.  Thanks!
Flags: needinfo?(jmuizelaar)
(Reporter)

Comment 5

5 months ago
This is definitely still an issue.

We want to be doing the more elaborate thing that ChooseScaleAndSetTransform does here: https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/gfx/layers/wr/StackingContextHelper.cpp#55
Flags: needinfo?(jmuizelaar)
Summary: Hookup proper scale selection → Hookup proper scale selection (ChooseScaleAndSetTransform)
(Reporter)

Comment 6

5 months ago
We could add a scale parameter to StackingContextHelper that the nsDisplayTransform would set.
(Reporter)

Updated

5 months ago
Assignee: jmuizelaar → nobody
Priority: P2 → P3
(Reporter)

Updated

4 months ago
Assignee: nobody → jmuizelaar
(Reporter)

Comment 7

4 months ago
This should be reusable by WebRender
Attachment #9026226 - Attachment mime type: application/octet-stream → text/plain
Attachment #9026226 - Attachment is patch: true
Hmm, does this bug also cover using ChooseScaleAndSetTransform in the blob code? There's a comment about it here: https://searchfox.org/mozilla-central/rev/876022232b15425bb9efde189caf747823b39567/gfx/layers/wr/WebRenderCommandBuilder.cpp#1906-1907
(Reporter)

Comment 9

4 months ago
It can be adapted to yes.
(Reporter)

Updated

4 months ago
Assignee: jmuizelaar → aosmond
(Reporter)

Updated

4 months ago
Assignee: aosmond → tnikkel
(Assignee)

Comment 11

3 months ago
Posted patch Use ChooseScale in wr (obsolete) — Splinter Review
This at least fixes bug 1492859 and passes try server on linux, but still need to understand the webrender bits a bit more to make sure they are correct (an obvious mistake in an earlier patch only failed one reftest).
Blocks: 1492859
(Assignee)

Comment 12

3 months ago
Attachment #9030165 - Attachment is obsolete: true
Attachment #9031944 - Flags: feedback?(jmuizelaar)
(Reporter)

Comment 13

3 months ago
Comment on attachment 9031944 [details] [diff] [review]
Use ChooseScale in wr

Review of attachment 9031944 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder if it makes more sense to move the call to ChooseScale out into the caller so that we're only calling it for transform items and not for everything that pushes a stacking context. Other than that this seems reasonable.
Attachment #9031944 - Flags: feedback?(jmuizelaar) → feedback+
(Assignee)

Comment 14

3 months ago
Not sure if this is better, I guess we could compute mInheritedTransform and mSnappingSurfaceTransform in the caller, but that's just more things to pass.
Attachment #9032103 - Flags: feedback?(jmuizelaar)
(Reporter)

Comment 15

3 months ago
Comment on attachment 9032103 [details] [diff] [review]
ChooseScale in caller

Review of attachment 9032103 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, why don't we go with the first one. I think the better solution here is to add a second constructor for StackingContextHelper but we can do that in a follow up.
Attachment #9032103 - Flags: feedback?(jmuizelaar) → feedback+
(Assignee)

Updated

3 months ago
Attachment #9031944 - Flags: review?(jmuizelaar)
(Assignee)

Comment 16

3 months ago
Comment on attachment 9030164 [details] [diff] [review]
Factor out the ChooseScale part of ChooseScacleAndSetTransform (rebased)

This is Jeff's patch so I can review it.
Attachment #9030164 - Flags: review+
(Reporter)

Updated

3 months ago
Attachment #9031944 - Flags: review?(jmuizelaar) → review+

Comment 17

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/640e15aa79b9
https://hg.mozilla.org/mozilla-central/rev/996bf60e0fce
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Duplicate of this bug: 1492859

Updated

3 months ago
Blocks: 1448926

Updated

3 months ago
Blocks: 1493359

Comment 19

3 months ago

This bug massively improved bug 1448926 and bug 1493359 with WR enabled.

You need to log in before you can comment on or make changes to this bug.