Bug 50758 – Defer loading print stylesheets

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

Currently stylesheets for media="print" are loaded with the same high priority as the rest. This can slow down first display. 

With Wireshark you can see that for example nytimes.com loads a print stylesheet very early.

[Antti Koivisto]

<rdar://problem/8722094>

[Antti Koivisto]

Created attachment 76104 [details]
patch

- Make preload scanner skip stylesheets with non-screen media types.
- Make link element skip stylesheets for media types other than screen or print.
- Make it possible to specify non-default load priority from the client.
- Use this mechanism to load print stylesheets using very low priority so they get loaded after everything else.
- Move default priority code from Loader to CachedResource.
- Move ResourceLoadScheduler::Priority enum to a separate file (as ResourceLoadPriority).

[Antti Koivisto]

Created attachment 76113 [details]
fixed a bug that was causing drt crashes

[Antti Koivisto]

Now I see that bug 22429 "document.styleSheets collection ignores media=presentation" deliberately enabled loading all stylesheets no matter how useless they are. 

I can rip that portion out from this patch but that change was not a very good idea in the first place.

[Alexey Proskuryakov]

If some stylesheets are omitted, then code like document.styleSheets[5] won't work, and that's almost certainly an issue.

[Alexey Proskuryakov]

I can also imagine a witty Web developer moving CSS rules from an otherwise unused stylesheet.

[Antti Koivisto]

We(In reply to comment #5)
> If some stylesheets are omitted, then code like document.styleSheets[5] won't work, and that's almost certainly an issue.

Having empty sheets should work fine for that.

I'm not really worried about braille sheets, but the inability to optimize this:

http://broadcast.oreilly.com/2010/04/using-css-media-queries-ipad.html

[Alexey Proskuryakov]

> Having empty sheets should work fine for that.

Is giving unused stylesheets lowest priority insufficient? Breaking something that's mandated by CSSOM spec and works in every browser doesn't seem great.

[Antti Koivisto]

(In reply to comment #8)
> > Having empty sheets should work fine for that.
> 
> Is giving unused stylesheets lowest priority insufficient? Breaking something that's mandated by CSSOM spec and works in every browser doesn't seem great.

Not exactly the only place where we ignore CSSOM, or other browsers. Anyway I'm not convinced that this is anything more than a theoretical compatibility issue. The behavior was changed (based on "other browsers do this" only) very late, in 2009, which indicates that the case was actually not seen in the real world much or at all.

But yeah, I'll turn this one into a prioritization only patch.

[Antti Koivisto]

Created attachment 76378 [details]
updated patch

- Load all stylesheets, just use lower prioritization for non-screen sheets
- Implement load order serialization so we can test this reliably in DRT
- Add some resource load ordering tests

[WebKit Review Bot]

Attachment 76378 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7024090

[Build Bot]

Attachment 76378 [details] did not build on win:
Build output: http://queues.webkit.org/results/6989109

[Build Bot]

Attachment 76378 [details] did not build on win:
Build output: http://queues.webkit.org/results/7037040

[Antti Koivisto]

Created attachment 76390 [details]
fix drt build

[Alexey Proskuryakov]

Comment on attachment 76378 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76378&action=review

I've been reviewing the previous version (without build fixes). Looks good, and I'm especially happy about test coverage.

Please fix minor comments. I'm not quite sure what to do about the two bigger ones (possibility of printing problems, and breaking preloading for complex media). Is this something to discuss with Hyatt, or are you sure about this being fine?

> WebCore/ChangeLog:11
> +        - Use this mechanism to load print stylesheets using very low priority so they get loaded after everything else.

I'm slightly worried that this may increase problem rate when actually printing. It will be more likely that a page will not be printable yet when it looks complete on screen. The user would press Cmd+P, and observe broken layout.

> WebCore/ChangeLog:20
> +        * WebCore.xcodeproj/project.pbxproj:

Please modify other project files.

> WebCore/css/MediaQueryEvaluator.h:77
> -    bool eval(const MediaList*, CSSStyleSelector* = 0) const;
> +    bool eval(PassRefPtr<MediaList>, CSSStyleSelector* = 0) const;

This change is wrong. PassRefPtr should be used when passing ownership - but MediaQueryEvaluator doesn't take ownership of the MediaList.

> WebCore/html/HTMLLinkElement.cpp:227
> +    // FIXME: We shouldn't load unnecessary stylesheets at all.

Please remove this FIXME. There is no such thing as "unnecessary stylesheet".

> WebCore/html/HTMLLinkElement.cpp:261
> +        // Load print stylesheets with low priority so they don't affect normal page loading.
> +        ResourceLoadPriority priority = mediaIsScreen ? ResourceLoadPriorityUnresolved : ResourceLoadPriorityVeryLow;
> +        m_cachedSheet = document()->cachedResourceLoader()->requestCSSStyleSheet(m_url, charset, priority);

s/print/non-screen/?

> WebCore/html/parser/HTMLPreloadScanner.cpp:92
> +    bool linkMediaAttributeMatches(const String& attributeValue)

This function doesn't use class members, so it should be static. Also, the name implies matching something in class state, which is misleading. I suggest something like "linkMediaAttributeMatchesRenderingIntent" or linkMediaAttributeIsScreen".

> WebCore/html/parser/HTMLPreloadScanner.cpp:100
> +        // Only preload screen media stylesheets. Used this way, the evaluator evaluates to false for any 
> +        // rules containing complex queries (full evaluation is possible but it requires frame and style selector which
> +        // may be problematic here). Cases where picking these for preloading would help are probably rare.

Will this prevent preloading for screen size and resolution specific stylesheets? How bad is that for embedded devices?

> WebKitTools/ChangeLog:5
> +        Add setSerializeHTTPLoads function to allow testing resource load order on OS X.

Please consider also implementing this at least for WebKit2.

> WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm:1004
> +    [WebView _setLoadResourcesSerially:serialize forHost:@"127.0.0.1"];

What is the use case for passing a host here? We'll want at least localhost, too.

> LayoutTests/ChangeLog:1
> [phone]  Antti Koivisto  <[e-mail]>

Duplicated ChangeLog in LayoutTests.

[Alexey Proskuryakov]

Comment on attachment 76390 [details]
fix drt build

There's a bunch of minor comments and some questions to discuss, seems worth another round. But this looks technically sound, and very promising.

[Antti Koivisto]

Created attachment 76406 [details]
updated based on ap's comments

[Antti Koivisto]

(In reply to comment #15)

> > WebCore/ChangeLog:11
> > +        - Use this mechanism to load print stylesheets using very low priority so they get loaded after everything else.
> 
> I'm slightly worried that this may increase problem rate when actually printing. It will be more likely that a page will not be printable yet when it looks complete on screen. The user would press Cmd+P, and observe broken layout.

It shouldn't be a significant concern. People don't normally print before the page is visually complete. Parallel loading ensures that small number of hanging resources won't prevent the print sheet from getting loaded.

> Please modify other project files.

Done.

> This change is wrong. PassRefPtr should be used when passing ownership - but MediaQueryEvaluator doesn't take ownership of the MediaList.

Oh ok. Fixed.

> > WebCore/html/HTMLLinkElement.cpp:227
> > +    // FIXME: We shouldn't load unnecessary stylesheets at all.
> 
> Please remove this FIXME. There is no such thing as "unnecessary stylesheet".

Stylesheets that are never used are unnecessary to load. This is major performance concern. I have bug open to fix this too.

> This function doesn't use class members, so it should be static. Also, the name implies matching something in class state, which is misleading. I suggest something like "linkMediaAttributeMatchesRenderingIntent" or linkMediaAttributeIsScreen".

Fixed.

> Will this prevent preloading for screen size and resolution specific stylesheets? How bad is that for embedded devices?

Changed to load by default. This can be optimized later.

> > WebKitTools/ChangeLog:5
> > +        Add setSerializeHTTPLoads function to allow testing resource load order on OS X.
> 
> Please consider also implementing this at least for WebKit2.

Later.

> > WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm:1004
> > +    [WebView _setLoadResourcesSerially:serialize forHost:@"127.0.0.1"];
> 
> What is the use case for passing a host here? We'll want at least localhost, too.

Nothing really. I got rid of it and simplified the code in the process.

[Alexey Proskuryakov]

Comment on attachment 76406 [details]
updated based on ap's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=76406&action=review

>> Please modify other project files.
> Done.

Please also update the ChangeLog.

> > Please remove this FIXME. There is no such thing as "unnecessary stylesheet".
> Stylesheets that are never used are unnecessary to load. This is major performance concern. I have bug open to fix this too.

How do we know that they are never used? The only thing we could do is block on CSSOM access and load the stylesheet on demand, but that's even worse, as that would use a synchronous load. And even that could have compatibility implications - servers heavily rely on getting exactly one request for a Web font, for example.

Please do remove the FIXME - it's not really necessary if there's a bug, and we can continue the discussion in the bug.

> > Please consider also implementing this at least for WebKit2.
> Later.

OK. Generally, we should treat WebKit2 as first class citizen now, and if I were making this patch, implementing a WebKit2 DRT hook while I was still fully in the context of this work would be easiest for me.

> LayoutTests/platform/win/Skipped:1098
> +# For https://bugs.webkit.org/show_bug.cgi?id=50758
> +# These require DRT setSerializeHTTPLoads implementation to be reliable.

As a courtesy, we try to modify other platforms' Skipped lists, too (when it's straightforward).

[Antti Koivisto]

(In reply to comment #19)
> > Later.
> 
> OK. Generally, we should treat WebKit2 as first class citizen now, and if I were making this patch, implementing a WebKit2 DRT hook while I was still fully in the context of this work would be easiest for me.

This patch is alrady rather large. I just don't want it to rot while I figure out how to do that.

> > LayoutTests/platform/win/Skipped:1098
> > +# For https://bugs.webkit.org/show_bug.cgi?id=50758
> > +# These require DRT setSerializeHTTPLoads implementation to be reliable.
> 
> As a courtesy, we try to modify other platforms' Skipped lists, too (when it's straightforward).

If other platforms have more deterministic HTTP stacks than CFNetwork, they might not need this hack at all. I'll see what the build bots say before modifying the Skipped lists.

[Antti Koivisto]

Thanks for review!

[Antti Koivisto]

http://trac.webkit.org/changeset/73938

[WebKit Review Bot]

http://trac.webkit.org/changeset/73938 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
fast/dom/gc-11.html
http/tests/local/link-stylesheet-load-order-preload.html
http/tests/local/link-stylesheet-load-order.html

[Alexey Proskuryakov]

The load order tests need to be added to Skipped list, gc-11 is probably unrelated.

[Antti Koivisto]

      (In reply to comment #24)
> The load order tests need to be added to Skipped list, gc-11 is probably unrelated.

Gtk was skipped in http://trac.webkit.org/changeset/73954. I did add them to other skipped lists that I saw failing too.