1933181 - Stop using nsView for popups.

Status: RESOLVED FIXED

(Core :: Layout, task)

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1933181 - Stop using nsView for popups. r=#layout

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9439692 [details]
Bug 1933181 - Stop using nsView for popups. r=#layout

WDYT about something like this? This seems to work great (only tested on Linux tho), and would be the first step to get rid of views.

Comment 3

[Timothy Nikkel (:tnikkel)]

Had a quick look through, so far nothing stands out as a hard blocker. I want to think a bit more on this though.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

(In reply to Timothy Nikkel (:tnikkel) from comment #3)

Had a quick look through, so far nothing stands out as a hard blocker. I want to think a bit more on this though.

Yeah didn't mean to make it a proper review request, mostly a quick "am I too off" thing. I need to fix handling of <browser>s inside popups or so, for example which might make me kill the nsSubDocumentFrame outer view as well perhaps, or do something else. Once I figure that out I'll send it for review proper.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: WIP: Bug 1933181 - Don't use views for subdocs.

Main issue being that the browsing context tree is mutated before
display lists are created, so this triggers RDL asserts.

Works other than that tho.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1933181 - Minor simplification to XULPopupElement::GetOuterScreenRect. r=#layout

No behavior change.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1933181 - Introduce nsIWidget::Get{,Popup}Frame. r=stransky,#layout

And use it instead of manually using views.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1933181 - Simplify nsWindow::GetTopmostWindow(). r=stransky

The code already doesn't jump across toplevel boundaries so this is
simpler.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1933181 - Make painting code not use views directly. r=#layout

The idea is that by passing (frame, renderer) we'll accommodate popups
not having views (and eventually kill views directly).

Comment 10

[Phabricator Automation]

Comment on attachment 9519342 [details]
Bug 1933181 - Minor simplification to XULPopupElement::GetOuterScreenRect. r=#layout

Revision D268205 was moved to bug 1993570. Setting attachment 9519342 [details] to obsolete.

Comment 11

[Phabricator Automation]

Comment on attachment 9519343 [details]
Bug 1933181 - Introduce nsIWidget::Get{,Popup}Frame. r=stransky,#layout

Revision D268206 was moved to bug 1993570. Setting attachment 9519343 [details] to obsolete.

Comment 12

[Phabricator Automation]

Comment on attachment 9519344 [details]
Bug 1933181 - Simplify nsWindow::GetTopmostWindow(). r=stransky

Revision D268207 was moved to bug 1993570. Setting attachment 9519344 [details] to obsolete.

Comment 13

[Phabricator Automation]

Comment on attachment 9519348 [details]
Bug 1933181 - Make painting code not use views directly. r=#layout

Revision D268208 was moved to bug 1993570. Setting attachment 9519348 [details] to obsolete.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/93b4c09f7e85
https://hg.mozilla.org/integration/autoland/rev/832b10027e3b
Stop using nsView for popups. r=layout-reviewers,tnikkel

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3babafbc8b63
https://hg.mozilla.org/integration/autoland/rev/78a64826662b
Skip two translations tests that fail on wayland opt.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/dc244b89d062
https://hg.mozilla.org/integration/autoland/rev/111e68144f88
Same for other two tests I missed.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/61bcdfc7acf3
https://hg.mozilla.org/mozilla-central/rev/068ddb3652ea
Same for other two tests I missed.

Comment 18

[amarc]

https://hg.mozilla.org/mozilla-central/rev/832b10027e3b
https://hg.mozilla.org/mozilla-central/rev/78a64826662b

Comment 19

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/111e68144f88

Comment 20

[Phabricator Automation]

Comment on attachment 9443435 [details]
WIP: Bug 1933181 - Don't use views for subdocs.

Revision D232057 was moved to bug 1994942. Setting attachment 9443435 [details] to obsolete.

Comment 21

[Florin Bilt]

(In reply to Pulsebot from comment #14)

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/93b4c09f7e85
https://hg.mozilla.org/integration/autoland/rev/832b10027e3b
Stop using nsView for popups. r=layout-reviewers,tnikkel

Perfherder has detected a devtools performance change from push 832b10027e3b1c44f0a22891903f657bf2a97064.

If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
19%	damp console.typing	linux1804-64-shippable-qr	e10s fission stylo webrender	527.48 -> 426.70
12%	damp console.autocomplete	linux1804-64-shippable-qr	e10s fission stylo webrender	665.67 -> 587.60
8%	damp console.autocomplete	windows11-64-24h2-shippable	e10s fission stylo webrender	395.45 -> 362.34

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 47169

The following documentation link provides more information about this command.