Closed Bug 1546783 Opened 1 year ago Closed 6 months ago

Consider to speculatively load css imports (@import) from inline styles

Categories

(Core :: DOM: HTML Parser, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: smaug, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

.

To be precise, the situation here is:

  <script src="really-slow-loading-script.js"></script>
  <style>
    @import url("some-style-sheet");
  </style>

Right now we block the parser on the script, and don't end up starting the import load until the script finishes loading and we parse the <style>.

In an ideal world, the HTML prescan we keep doing (which would kick off a <link rel="stylesheet" url="some-style-sheet"> load) would deal with this case and start the load for "some-style-sheet". The HTML parser can obviously grab the string inside the <style> element, but then we need to actually find the @imports in it.

While we're doing this, we should also consider doing the same for @font-face (maybe, at least, since I guess the font could not get used).

I know WebKit (and I guess Blink) does preload imports, though their analyzer was a bit crappy and failed to recognize @imports without quotes: https://bugs.webkit.org/show_bug.cgi?id=191466

maybe, at least, since I guess the font could not get used

Right, that is the issue with fonts...

It shouldn't be too terrible to add a CSS parser mode that stops after import rules, and triggers loads for them... I don't think it'd have many side effects, other than maybe allocating Servo NamespaceRule / ImportRule objects and such. We already parse a bunch off-main-thread.

Or alternatively we could go with the crappy "tokenizer" approach that WebKit / Blink have:

http://webkit.crisal.io/webkit/rev/6cc00baaf9835dc4abdf7b16e72cff60ecd7bb88/Source/WebCore/html/parser/CSSPreloadScanner.cpp

If we're just looking for @import that may be even simpler... Not sure.

Henri, do you know where could I get the stylesheet text content from the parser code? Also, would they be utf8? Or utf16?

Flags: needinfo?(hsivonen)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Henri, do you know where could I get the stylesheet text content from the parser code?

First, in https://searchfox.org/mozilla-central/source/parser/html/nsHtml5TreeBuilderHSupplement.h add a style nesting counter (below the comment with exclamation marks about mBuilder). (Counter instead of a boolean thanks to SVG being able to nest elements within style.)

In the below methods, changes should only go on the off-the-main-thread code path, which is the case where mBuilder is null.

In https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/parser/html/nsHtml5TreeBuilderCppSupplement.h#829 if the element is style in either the HTML or SVG namespace, increment the counter (in the existing blocks that now handle line numbers for HTML and SVG style). In https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/parser/html/nsHtml5TreeBuilderCppSupplement.h#900 decrement the counter.

In https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/parser/html/nsHtml5TreeBuilderCppSupplement.h#651 if the counter is nonzero, process the text. Note that a single text node could be split across multiple calls to this method.

For an example of how to ship the URLs to the main thread, see
https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/parser/html/nsHtml5TreeBuilderCppSupplement.h#236

Also, would they be utf8? Or utf16?

UTF-16, unfortunately.

Flags: needinfo?(hsivonen)
Summary: Consider to speculatively load css imports from inline styles → Consider to speculatively load css imports (@import) from inline styles
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Flags: needinfo?(emilio)

Doesn't recognize all the edge cases, but I think this should be good enough.

Let me know if you think something common is missing.

That doesn't make much sense to me.

We don't bother handling the nested element case amazingly. We'd instead stop
at the inner <style> element and drop the URLs from the outer.

But I think that's ok. Any good way to test this? I've verified it does the
right thing looking at the CSS loader logs, but... :)

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2678d2e55fb
Add a best-effort @import rule scanner for the parser. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/a8ac28c2bfc7
Stop speculatively trying to load <svg:style xlink:href>. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/ad3822105edc
Speculatively load @import rules inside <style>. r=bzbarsky,hsivonen
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78d0e221f6a4
Add a best-effort @import rule scanner for the parser. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/dfccd3155829
Stop speculatively trying to load <svg:style xlink:href>. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/56599f0a87a4
Speculatively load @import rules inside <style>. r=bzbarsky,hsivonen
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce851932daee
Test that we preload @import stylesheets. r=bzbarsky
Blocks: 1585674
You need to log in before you can comment on or make changes to this bug.