Bug 106198 - PreloadScanner preloads external CSS with non-matching media attribute
Summary: PreloadScanner preloads external CSS with non-matching media attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-06 14:12 PST by Yoav Weiss
Modified: 2013-08-03 06:42 PDT (History)
15 users (show)

See Also:


Attachments
A patch to avoid preloading of external stylesheets with a non-matching media attribute (3.76 KB, patch)
2013-01-06 14:13 PST, Yoav Weiss
abarth: review-
Details | Formatted Diff | Diff
Preload external stylesheet according to their media attribute (7.39 KB, patch)
2013-04-01 06:21 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Preload external stylesheets according to their media attribute (10.79 KB, patch)
2013-04-05 14:01 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Preload external stylesheets according to their media attribute (10.57 KB, patch)
2013-04-11 08:54 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Added ChangeLog (2.41 KB, patch)
2013-04-11 09:03 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (475.90 KB, application/zip)
2013-04-11 10:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (873.45 KB, application/zip)
2013-04-11 11:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (487.58 KB, application/zip)
2013-04-11 12:29 PDT, Build Bot
no flags Details
Fixed stylesheet preload tests (833 bytes, patch)
2013-04-11 12:50 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Preload external stylesheets according to their media attribute (13.81 KB, patch)
2013-04-11 13:25 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (14.07 KB, patch)
2013-08-03 05:36 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff
Patch (14.12 KB, patch)
2013-08-03 05:42 PDT, Yoav Weiss
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoav Weiss 2013-01-06 14:12:03 PST
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.
Comment 1 Yoav Weiss 2013-01-06 14:13:43 PST
Created attachment 181465 [details]
A patch to avoid preloading of external stylesheets with a non-matching media attribute
Comment 2 Adam Barth 2013-01-06 15:20:05 PST
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.
Comment 3 Adam Barth 2013-01-06 15:20:33 PST
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.
Comment 4 Yoav Weiss 2013-01-10 13:10:23 PST
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
Comment 5 Adam Barth 2013-01-10 13:22:30 PST
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.
Comment 6 Yoav Weiss 2013-03-28 13:04:43 PDT
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
Comment 7 Adam Barth 2013-03-30 11:01:28 PDT
> 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.
Comment 8 Yoav Weiss 2013-04-01 06:21:46 PDT
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.
Comment 9 Adam Barth 2013-04-01 10:54:58 PDT
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.
Comment 10 Yoav Weiss 2013-04-02 00:08:57 PDT
Thanks for your review & comments. I'll perform the required changes and re-submit.
Comment 11 Yoav Weiss 2013-04-05 14:01:40 PDT
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.
Comment 12 Eric Seidel (no email) 2013-04-06 15:11:51 PDT
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...
Comment 13 Yoav Weiss 2013-04-11 08:54:55 PDT
Created attachment 197594 [details]
Preload external stylesheets according to their media attribute

Fixed the comments on previous patches
Comment 14 Yoav Weiss 2013-04-11 09:03:56 PDT
Created attachment 197595 [details]
Added ChangeLog
Comment 15 Build Bot 2013-04-11 10:06:33 PDT
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
Comment 16 Build Bot 2013-04-11 10:06:36 PDT
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
Comment 17 Build Bot 2013-04-11 11:00:28 PDT
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
Comment 18 Build Bot 2013-04-11 11:00:31 PDT
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
Comment 19 Build Bot 2013-04-11 12:29:52 PDT
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
Comment 20 Build Bot 2013-04-11 12:29:55 PDT
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
Comment 21 Yoav Weiss 2013-04-11 12:50:20 PDT
Created attachment 197657 [details]
Fixed stylesheet preload tests
Comment 22 Darin Adler 2013-04-11 12:51:51 PDT
Comment on attachment 197657 [details]
Fixed stylesheet preload tests

Change looks OK. review- because a patch needs a change log.
Comment 23 Yoav Weiss 2013-04-11 13:25:37 PDT
Created attachment 197661 [details]
Preload external stylesheets according to their media attribute
Comment 24 Yoav Weiss 2013-04-11 22:48:45 PDT
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)
Comment 25 Yoav Weiss 2013-04-15 13:20:00 PDT
Can someone please review the latest patch?

Thanks,
Yoav
Comment 26 Yoav Weiss 2013-04-19 03:36:46 PDT
ping
Comment 27 Adam Barth 2013-04-19 12:42:16 PDT
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.
Comment 28 Yoav Weiss 2013-04-28 05:06:46 PDT
ping
Comment 29 Dean Jackson 2013-08-03 04:39:55 PDT
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 "".
Comment 30 Yoav Weiss 2013-08-03 05:36:31 PDT
Created attachment 208063 [details]
Patch
Comment 31 WebKit Commit Bot 2013-08-03 05:38:55 PDT
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.
Comment 32 Dean Jackson 2013-08-03 05:39:59 PDT
Comment on attachment 208063 [details]
Patch

TABS! :)
Comment 33 Yoav Weiss 2013-08-03 05:42:32 PDT
Created attachment 208064 [details]
Patch
Comment 34 WebKit Commit Bot 2013-08-03 06:42:14 PDT
Comment on attachment 208064 [details]
Patch

Clearing flags on attachment: 208064

Committed r153689: <http://trac.webkit.org/changeset/153689>
Comment 35 WebKit Commit Bot 2013-08-03 06:42:20 PDT
All reviewed patches have been landed.  Closing bug.