1470163 - Consider removing EnsureOnDemandBuiltinUASheet.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

It's not relevant for memory usage anymore, and perf of UA sheets in content should be good.

The on demand stuff has caused pain, for example, when the frontend people try
loading <svg>s from NAC (since that triggers sheet loading from frame
construction, which is not good).

I'm not concerned about loading mathml.css and svg.css everywhere, though xul.css may not be as doable since it adds a bunch of attribute-dependent selectors. Though on the other hand I asserted in the xul.css code and we don't load it in content with <video> / <input type="date/time/etc"> and such, afaict, so maybe now that legacy addons are gone we can remove that sheet from content processes altogether.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Turns out that xul.css is no longer loaded if not upfront. Horray!

Comment 2

[Phabricator Automation]

Attachment: Bug 1470163: Load mathml.css upfront, and remove the concept of on-demand builtin UA sheets. r=heycam

On top of the two depending bugs.

Funny how there's a comment referencing bug 77999.

Comment 3

[Phabricator Automation]

Comment on attachment 8986781 [details]
Bug 1470163: Load mathml.css upfront, and remove the concept of on-demand builtin UA sheets. r=heycam

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1750

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Don't load mathml/svg.css if MathML/SVG is disabled.

This fixes a MathML-disabled reftest with the previous patch.
    
The reftest assumes the sheet is not loaded, so let's just preserve that behavior.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

The test is https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7baaa6b3d283b8e253fd33b506dede20f44cfd7, fwiw.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8987025 [details] [diff] [review]
Don't load mathml/svg.css if MathML/SVG is disabled.

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

::: layout/base/nsDocumentViewer.cpp
@@ +2519,4 @@
>      styleSet->PrependStyleSheet(SheetType::Agent, sheet);
>    }
>  
>    sheet = cache->MathMLSheet();

Since we lazily load the MathML sheet, maybe wrap this in the MathMLEnabled() check?  (And maybe do it for the SVG sheet too just so the code looks symmetrical, even though we eagerly load that one.)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e97e7a605f
Load mathml.css upfront, and remove the concept of on-demand builtin UA sheets. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/e34a6bdac37a
Don't load mathml/svg.css if MathML/SVG is disabled. r=heycam

Comment 8

[Stefan Hindli [:stefan_hindli]]

Backed out 2 changesets (bug 1470163) for math failures in layout/mathml/tests/test_disabled.html on a CLOSED TREE

Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=9483bb48c57a4da7f9dfeba01d1573038a49d9c2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=184551334
Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6444985fb666178175be8b15c25349f8e333e034&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e972452d0ef7fb2878c25cda8e7ceedeec854537&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=184551334&repo=mozilla-inbound&lineNumber=2237

[task 2018-06-24T03:13:59.607Z] 03:13:59     INFO - TEST-UNEXPECTED-FAIL | layout/mathml/tests/test_disabled.html | undefined assertion name - got "<math>&lt;foo&gt;</math>", expected "<math:math>&lt;foo&gt;</math:math>"

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Move test_disabled.html tests to an iframe.

These tests rely on tweaking the pref before storing in the document whether it's actually enabled or not. The previous patch does check it early in the document's lifetime, so those pref toggles are ineffective. Move the actual test to an iframe so that it sees the pref change.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0905296423bf85a2cea4686f8f2e258e008e0968

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8987310 [details] [diff] [review]
Move test_disabled.html tests to an iframe.

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

r=me but let's rename the test_disabled_iframe.html files to file_disabled_iframe.html.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de319cd7dd2b
Load mathml.css upfront, and remove the concept of on-demand builtin UA sheets. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/edf707347ab7
Don't load mathml/svg.css if MathML/SVG is disabled. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b9f13ddee9
Move test_disabled to an iframe. r=heycam

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/de319cd7dd2b
https://hg.mozilla.org/mozilla-central/rev/edf707347ab7
https://hg.mozilla.org/mozilla-central/rev/25b9f13ddee9