PreloadScanner preloads external CSS with non-matching media attribute

• Component: Page Loading
• Status: RESOLVED FIXED

[Yoav Weiss]

The PreloadScanner currently loads external style sheets with a non-matching media query, as long as the media type is "screen".
This can result in unnecessary delay to the page's first paint. 
Since these resources are labeled as "very low priority" in HTMLLinkElement (even though this is not currently acted upon in Chrome. See https://bugs.webkit.org/show_bug.cgi?id=105919), I don't think preloading them is useful.

I attach a patch that avoids preloading non-matching external stylesheets.

[Yoav Weiss]

Created attachment 181465 [details]
A patch to avoid preloading of external stylesheets with a non-matching media attribute

[Adam Barth]

Comment on attachment 181465 [details]
A patch to avoid preloading of external stylesheets with a non-matching media attribute

We don't want to add a Frame dependency to the preload scanner.  We're likely to move the preload scanner and/or the entire HTML parser onto a background thread where it won't have access to the frame.

[Adam Barth]

Comment on attachment 181465 [details]
A patch to avoid preloading of external stylesheets with a non-matching media attribute

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

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:207
> +    RefPtr<RenderStyle> documentStyle = StyleResolver::styleForDocument(m_document, 0);

Also, I'd be surprised if we wanted the preload scanner to resolve styles.

[Yoav Weiss]

In an attempt to prove this issue to be significant in real life, I've ran an analysis of ~35K top Alexa HTMLs from webdevdata.org.
What I have seen is that only 30% of external stylesheet are supposed to be fetched using the PreloadScanner (i.e. they appear after an external script in the HTML).
I've also seen that only 0.5% of those had a media attribute that referred to a complex media query (e.g. width, device-width, orientation, device-pixel-ratio).

So I guess that at least for now, this is not an important issue.

Never the less, I'd love to discuss ways to improve the patch, to make it more compliant with current and future WebKit design.
Is this bug the right place to do so? Are other channels preferable? (IRC, mailing list, etc)

Thanks,
Yoav

[Adam Barth]

We're in the process of deleting the PreloadScanner in favor of a speculative parser.  We should revisit this issue after we've switched over to the new model.

[Yoav Weiss]

Revisiting the PreloadScanner's code after 3 months, I see that there are many changes supporting the threaded parser, but the basic architecture doesn't seem very different, at least not at first sight.

Are there significant changes planned for the PreloadScanner in the near future, or can I assume that it is now relatively stable?

I'd like to continue exploring this issue. 

Thanks,
Yoav

[Adam Barth]

> Are there significant changes planned for the PreloadScanner in the near future, or can I assume that it is now relatively stable?

It should be relatively stable now.

> I'd like to continue exploring this issue. 

Go for it.  Thanks for waiting.

[Yoav Weiss]

Created attachment 195963 [details]
Preload external stylesheet according to their media attribute

Here's my new attempt at resolving this issue.
The patch is based on the new multi-threaded PreloadScanner architecture.
The PreloadScanner now passed the media attribute string to the ResourcePreloader, and the ResourcePreloader evaluates the MQs using the document's existing RenderStyle and Frame.
I've moved the MQ resolution bits to HTMLResourcePreloader.cpp, so I've removed this dependency from the HTMLPreloadScanner.cpp and added it to HTMLResourcePreloader.cpp. I also added RenderObject dependency to HTMLResourcePreloader.cpp as well. I hope that's OK.
I've tested it locally and it seems to work fine, but I didn't yet write proper Layout tests.

[Adam Barth]

Comment on attachment 195963 [details]
Preload external stylesheet according to their media attribute

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

The general approach looks reasonable.

You're also missing a ChangeLog entry.  :)

> Source/WebCore/html/parser/HTMLResourcePreloader.h:41
>      bool isSafeToSendToAnotherThread() const;

Please add m_mediaAttr to this function so that we can detect threading bug.

> Source/WebCore/html/parser/HTMLResourcePreloader.h:57
> +        , m_mediaAttr(mediaAttr)

You need to make a copy of this string here because this object is sent across threads.  See m_resourceURL above.

> Source/WebCore/html/parser/HTMLResourcePreloader.h:69
> +    String m_mediaAttr;

m_mediaAttr -> m_mediaAttribute (please use complete words in variable names)

> Source/WebCore/html/parser/HTMLResourcePreloader.h:92
> +    static bool mediaAttributeMatches(Frame* frame, RenderStyle* renderStyle, const String& attributeValue);

There's no reason to expose this function in the header.  It can just be a static function in the implementation.

[Yoav Weiss]

Thanks for your review & comments. I'll perform the required changes and re-submit.

[Yoav Weiss]

Created attachment 196676 [details]
Preload external stylesheets according to their media attribute

This patch is similar in architecture to the previous one (https://bugs.webkit.org/show_bug.cgi?id=106198#c8 )
I've applied the review comments and added a ChangeLog entry as well as tests.

[Eric Seidel (no email)]

Comment on attachment 196676 [details]
Preload external stylesheets according to their media attribute

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

> Source/WebCore/html/parser/HTMLResourcePreloader.cpp:84
> +    if(!preload->media().isEmpty() && !mediaAttributeMatches(m_document->frame(), m_document->renderer()->style(), preload->media()))

missing a space betwen if and (

> Source/WebCore/html/parser/HTMLResourcePreloader.h:36
> +    static PassOwnPtr<PreloadRequest> create(const String& initiator, const String& resourceURL, const KURL& baseURL, CachedResource::Type resourceType, const String& mediaAttr="")

I've not seen const String& used with defaut values in WebKit before...

[Yoav Weiss]

Created attachment 197594 [details]
Preload external stylesheets according to their media attribute

Fixed the comments on previous patches

[Yoav Weiss]

Created attachment 197595 [details]
Added ChangeLog

[Build Bot]

Comment on attachment 197594 [details]
Preload external stylesheets according to their media attribute

Attachment 197594 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17722215

New failing tests:
http/tests/local/link-stylesheet-load-order-preload.html

[Build Bot]

Created attachment 197634 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 197594 [details]
Preload external stylesheets according to their media attribute

Attachment 197594 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17711098

New failing tests:
http/tests/local/link-stylesheet-load-order-preload.html

[Build Bot]

Created attachment 197642 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 197594 [details]
Preload external stylesheets according to their media attribute

Attachment 197594 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/89143

New failing tests:
http/tests/local/link-stylesheet-load-order-preload.html

[Build Bot]

Created attachment 197653 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Yoav Weiss]

Created attachment 197657 [details]
Fixed stylesheet preload tests

[Darin Adler]

Comment on attachment 197657 [details]
Fixed stylesheet preload tests

Change looks OK. review- because a patch needs a change log.

[Yoav Weiss]

Created attachment 197661 [details]
Preload external stylesheets according to their media attribute

[Yoav Weiss]

I've added a new patch that includes a ChangeLog entry, as well as a fix to the failing tests expected result (since stylesheet preloading order have changed with this patch)

[Yoav Weiss]

Can someone please review the latest patch?

Thanks,
Yoav

[Yoav Weiss]

ping

[Adam Barth]

Comment on attachment 197661 [details]
Preload external stylesheets according to their media attribute

This patch LGTM, but I'm not sure I should mark it r=me.  It's probably better if a more active reviewer does the official review.

[Yoav Weiss]

ping

[Dean Jackson]

Comment on attachment 197661 [details]
Preload external stylesheets according to their media attribute

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

Looks ok. Just minor comments.

> Source/WebCore/ChangeLog:16
> +        * html/parser/HTMLPreloadScanner.cpp:
> +        (WebCore::TokenPreloadScanner::StartTagScanner::StartTagScanner):
> +        (WebCore::TokenPreloadScanner::StartTagScanner::createPreloadRequest):
> +        (WebCore::TokenPreloadScanner::StartTagScanner::processAttribute):
> +        (WebCore::TokenPreloadScanner::StartTagScanner::resourceType):
> +        (WebCore::TokenPreloadScanner::StartTagScanner::shouldPreload):
> +        (TokenPreloadScanner::StartTagScanner):

I'd really like some per-function comments here to make it clear what you're changing.

> Source/WebCore/html/parser/HTMLResourcePreloader.h:43
> +        return adoptPtr(new PreloadRequest(initiator, resourceURL, baseURL, resourceType, ""));

Use String() rather than "".

[Yoav Weiss]

Created attachment 208063 [details]
Patch

[WebKit Commit Bot]

Attachment 208063 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/loading/preload-css-test-expected.txt', u'LayoutTests/http/tests/loading/preload-css-test.html', u'LayoutTests/http/tests/loading/resources/big_mq.css', u'LayoutTests/http/tests/loading/resources/small_mq.css', u'LayoutTests/http/tests/local/link-stylesheet-load-order-preload-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/html/parser/HTMLPreloadScanner.cpp', u'Source/WebCore/html/parser/HTMLResourcePreloader.cpp', u'Source/WebCore/html/parser/HTMLResourcePreloader.h']" exit_code: 1
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:21:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:22:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:27:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:28:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 8 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Comment on attachment 208063 [details]
Patch

TABS! :)

[Yoav Weiss]

Created attachment 208064 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 208064 [details]
Patch

Clearing flags on attachment: 208064

Committed r153689: <http://trac.webkit.org/changeset/153689>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.