PreloadScanner preloads external CSS with non-matching media attribute
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 3 Aug 2013 13:43:24 +0000 (13:43 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 3 Aug 2013 13:43:24 +0000 (13:43 +0000)
https://bugs.webkit.org/show_bug.cgi?id=106198

Patch by Yoav Weiss <yoav@yoav.ws> on 2013-08-03
Reviewed by Dean Jackson.

Source/WebCore:

Test: http/tests/loading/preload-css-test.html

* html/parser/HTMLPreloadScanner.cpp:
Remove m_linkMediaAttributeIsScreen
Remove MediaQueryEvaluator calls
Add m_mediaAttribute that gets the value of the "media" attribute
Pass m_mediaAttribute to PreloadRequest
(WebCore::TokenPreloadScanner::StartTagScanner::StartTagScanner):
(WebCore::TokenPreloadScanner::StartTagScanner::createPreloadRequest):
(WebCore::TokenPreloadScanner::StartTagScanner::processAttribute):
(WebCore::TokenPreloadScanner::StartTagScanner::resourceType):
(WebCore::TokenPreloadScanner::StartTagScanner::shouldPreload):
* html/parser/HTMLResourcePreloader.cpp:
Add MediaQueryEvaluator calls to see if "media" matches
Perform preload only to resource with a matching media (if media exists)
(WebCore::PreloadRequest::isSafeToSendToAnotherThread):
(WebCore::mediaAttributeMatches):
(WebCore::HTMLResourcePreloader::preload):
* html/parser/HTMLResourcePreloader.h:
Add a constructor with a mediaAttribute value
Add m_mediaAttribute & its getter.
(WebCore::PreloadRequest::create):
(WebCore::PreloadRequest::media):
(WebCore::PreloadRequest::PreloadRequest):

LayoutTests:

* http/tests/loading/preload-css-test-expected.txt: Added.
* http/tests/loading/preload-css-test.html: Added.
* http/tests/loading/resources/big_mq.css: Added.
* http/tests/loading/resources/small_mq.css: Added.
* http/tests/local/link-stylesheet-load-order-preload-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@153689 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/loading/preload-css-test-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/loading/preload-css-test.html [new file with mode: 0644]
LayoutTests/http/tests/loading/resources/big_mq.css [new file with mode: 0644]
LayoutTests/http/tests/loading/resources/small_mq.css [new file with mode: 0644]
LayoutTests/http/tests/local/link-stylesheet-load-order-preload-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/html/parser/HTMLPreloadScanner.cpp
Source/WebCore/html/parser/HTMLResourcePreloader.cpp
Source/WebCore/html/parser/HTMLResourcePreloader.h

index 64f83b9..b0ea93d 100644 (file)
@@ -1,3 +1,16 @@
+2013-08-03  Yoav Weiss  <yoav@yoav.ws>
+
+        PreloadScanner preloads external CSS with non-matching media attribute
+        https://bugs.webkit.org/show_bug.cgi?id=106198
+
+        Reviewed by Dean Jackson.
+
+        * http/tests/loading/preload-css-test-expected.txt: Added.
+        * http/tests/loading/preload-css-test.html: Added.
+        * http/tests/loading/resources/big_mq.css: Added.
+        * http/tests/loading/resources/small_mq.css: Added.
+        * http/tests/local/link-stylesheet-load-order-preload-expected.txt:
+
 2013-08-02  Benjamin Poulain  <bpoulain@apple.com>
 
         REGRESSION (r153005): Crash in SpaceSplitString::spaceSplitStringContainsValue on Facebook
diff --git a/LayoutTests/http/tests/loading/preload-css-test-expected.txt b/LayoutTests/http/tests/loading/preload-css-test-expected.txt
new file mode 100644 (file)
index 0000000..f9d223d
--- /dev/null
@@ -0,0 +1,8 @@
+main frame - didStartProvisionalLoadForFrame
+main frame - didCommitLoadForFrame
+main frame - didFinishDocumentLoadForFrame
+main frame - didHandleOnloadEventsForFrame
+main frame - didFinishLoadForFrame
+Preload scanner should preload only the relevant MQ external CSS
+
+PASS PASS
diff --git a/LayoutTests/http/tests/loading/preload-css-test.html b/LayoutTests/http/tests/loading/preload-css-test.html
new file mode 100644 (file)
index 0000000..627cff6
--- /dev/null
@@ -0,0 +1,33 @@
+<head>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+var results=[];
+function checkForPreload(url, shouldbe) {
+    var preloaded = internals.isPreloaded(url);
+    if ((preloaded && shouldbe) || (!preloaded && !shouldbe))
+        results.push("PASS\n");
+    else
+        results.push("FAIL\n");
+}
+function printResults(){
+    for(var i = 0; i < results.length; i++)
+        document.getElementsByTagName("body")[0].appendChild(document.createTextNode(results[i]));
+}
+    
+</script>
+<script src="http://127.0.0.1:8000/resources/slow-script.pl?delay=1000"></script>
+<script>
+checkForPreload("resources/big_mq.css", true);
+checkForPreload("resources/small_mq.css", false);
+</script>
+<link href="resources/big_mq.css" rel="stylesheet" media="screen and (min-width: 800px)">
+<link href="resources/small_mq.css" rel="stylesheet" media="screen and (max-width: 799px)">
+<script src="http://127.0.0.1:8000/resources/slow-script.pl?delay=1000"></script>
+
+<body>
+<p>Preload scanner should preload only the relevant MQ external CSS</p>
+<script>
+printResults();
+</script>
diff --git a/LayoutTests/http/tests/loading/resources/big_mq.css b/LayoutTests/http/tests/loading/resources/big_mq.css
new file mode 100644 (file)
index 0000000..139597f
--- /dev/null
@@ -0,0 +1,2 @@
+
+
diff --git a/LayoutTests/http/tests/loading/resources/small_mq.css b/LayoutTests/http/tests/loading/resources/small_mq.css
new file mode 100644 (file)
index 0000000..139597f
--- /dev/null
@@ -0,0 +1,2 @@
+
+
index 5bd7aee..12b90d9 100644 (file)
@@ -7,14 +7,14 @@ no-media.css
 media-empty.css
 media-all.css
 media-screen.css
+media-screen-query-success.css
 media-braille-screen.css
 no-media-2.css
 media-empty-2.css
 media-all-2.css
 media-screen-2.css
-media-braille-screen-2.css
-media-screen-query-success.css
 media-screen-query-success-2.css
+media-braille-screen-2.css
 script-2.js
 image.jpg
 media-print.css
index 3492520..7296f33 100644 (file)
@@ -1,3 +1,35 @@
+2013-08-03  Yoav Weiss  <yoav@yoav.ws>
+
+        PreloadScanner preloads external CSS with non-matching media attribute
+        https://bugs.webkit.org/show_bug.cgi?id=106198
+
+        Reviewed by Dean Jackson.
+
+        Test: http/tests/loading/preload-css-test.html
+
+        * html/parser/HTMLPreloadScanner.cpp:
+        Remove m_linkMediaAttributeIsScreen
+        Remove MediaQueryEvaluator calls
+        Add m_mediaAttribute that gets the value of the "media" attribute
+        Pass m_mediaAttribute to PreloadRequest
+        (WebCore::TokenPreloadScanner::StartTagScanner::StartTagScanner):
+        (WebCore::TokenPreloadScanner::StartTagScanner::createPreloadRequest):
+        (WebCore::TokenPreloadScanner::StartTagScanner::processAttribute):
+        (WebCore::TokenPreloadScanner::StartTagScanner::resourceType):
+        (WebCore::TokenPreloadScanner::StartTagScanner::shouldPreload):
+        * html/parser/HTMLResourcePreloader.cpp:
+        Add MediaQueryEvaluator calls to see if "media" matches
+        Perform preload only to resource with a matching media (if media exists)
+        (WebCore::PreloadRequest::isSafeToSendToAnotherThread):
+        (WebCore::mediaAttributeMatches):
+        (WebCore::HTMLResourcePreloader::preload):
+        * html/parser/HTMLResourcePreloader.h:
+        Add a constructor with a mediaAttribute value
+        Add m_mediaAttribute & its getter.
+        (WebCore::PreloadRequest::create):
+        (WebCore::PreloadRequest::media):
+        (WebCore::PreloadRequest::PreloadRequest):
+
 2013-08-03  Andreas Kling  <akling@apple.com>
 
         RenderBoxModelObject::firstLetterRemainingText should be a RenderTextFragment*.
index 9daf743..fdba3bc 100644 (file)
@@ -34,8 +34,6 @@
 #include "HTMLTokenizer.h"
 #include "InputTypeNames.h"
 #include "LinkRelAttribute.h"
-#include "MediaList.h"
-#include "MediaQueryEvaluator.h"
 #include <wtf/MainThread.h>
 
 namespace WebCore {
@@ -110,7 +108,6 @@ public:
     explicit StartTagScanner(TagId tagId)
         : m_tagId(tagId)
         , m_linkIsStyleSheet(false)
-        , m_linkMediaAttributeIsScreen(true)
         , m_inputIsImage(false)
     {
     }
@@ -142,7 +139,7 @@ public:
         if (!shouldPreload())
             return nullptr;
 
-        OwnPtr<PreloadRequest> request = PreloadRequest::create(initiatorFor(m_tagId), m_urlToLoad, predictedBaseURL, resourceType());
+        OwnPtr<PreloadRequest> request = PreloadRequest::create(initiatorFor(m_tagId), m_urlToLoad, predictedBaseURL, resourceType(), m_mediaAttribute);
         request->setCrossOriginModeAllowsCookies(crossOriginModeAllowsCookies());
         request->setCharset(charset());
         return request.release();
@@ -179,7 +176,7 @@ private:
             else if (match(attributeName, relAttr))
                 m_linkIsStyleSheet = relAttributeIsStyleSheet(attributeValue);
             else if (match(attributeName, mediaAttr))
-                m_linkMediaAttributeIsScreen = linkMediaAttributeIsScreen(attributeValue);
+                m_mediaAttribute = attributeValue;
         } else if (m_tagId == InputTagId) {
             if (match(attributeName, srcAttr))
                 setUrlToLoad(attributeValue);
@@ -194,19 +191,6 @@ private:
         return rel.m_isStyleSheet && !rel.m_isAlternate && rel.m_iconType == InvalidIcon && !rel.m_isDNSPrefetch;
     }
 
-    static bool linkMediaAttributeIsScreen(const String& attributeValue)
-    {
-        if (attributeValue.isEmpty())
-            return true;
-        RefPtr<MediaQuerySet> mediaQueries = MediaQuerySet::createAllowingDescriptionSyntax(attributeValue);
-    
-        // Only preload screen media stylesheets. Used this way, the evaluator evaluates to true for any 
-        // rules containing complex queries (full evaluation is possible but it requires a frame and a style selector which
-        // may be problematic here).
-        MediaQueryEvaluator mediaQueryEvaluator("screen");
-        return mediaQueryEvaluator.eval(mediaQueries.get());
-    }
-
     void setUrlToLoad(const String& attributeValue)
     {
         // We only respect the first src/href, per HTML5:
@@ -230,7 +214,7 @@ private:
             return CachedResource::Script;
         if (m_tagId == ImgTagId || (m_tagId == InputTagId && m_inputIsImage))
             return CachedResource::ImageResource;
-        if (m_tagId == LinkTagId && m_linkIsStyleSheet && m_linkMediaAttributeIsScreen)
+        if (m_tagId == LinkTagId && m_linkIsStyleSheet)
             return CachedResource::CSSStyleSheet;
         ASSERT_NOT_REACHED();
         return CachedResource::RawResource;
@@ -241,7 +225,7 @@ private:
         if (m_urlToLoad.isEmpty())
             return false;
 
-        if (m_tagId == LinkTagId && (!m_linkIsStyleSheet || !m_linkMediaAttributeIsScreen))
+        if (m_tagId == LinkTagId && !m_linkIsStyleSheet)
             return false;
 
         if (m_tagId == InputTagId && !m_inputIsImage)
@@ -260,7 +244,7 @@ private:
     String m_charset;
     String m_crossOriginMode;
     bool m_linkIsStyleSheet;
-    bool m_linkMediaAttributeIsScreen;
+    String m_mediaAttribute;
     bool m_inputIsImage;
 };
 
index fa5adfb..22ebf21 100644 (file)
 #include "CachedResourceLoader.h"
 #include "Document.h"
 
+#include "MediaList.h"
+#include "MediaQueryEvaluator.h"
+#include "RenderObject.h"
+
 namespace WebCore {
 
 bool PreloadRequest::isSafeToSendToAnotherThread() const
@@ -36,6 +40,7 @@ bool PreloadRequest::isSafeToSendToAnotherThread() const
     return m_initiator.isSafeToSendToAnotherThread()
         && m_charset.isSafeToSendToAnotherThread()
         && m_resourceURL.isSafeToSendToAnotherThread()
+        && m_mediaAttribute.isSafeToSendToAnotherThread()
         && m_baseURL.isSafeToSendToAnotherThread();
 }
 
@@ -65,10 +70,24 @@ void HTMLResourcePreloader::takeAndPreload(PreloadRequestStream& r)
         preload(it->release());
 }
 
+static bool mediaAttributeMatches(Frame* frame, RenderStyle* renderStyle, const String& attributeValue)
+{
+    RefPtr<MediaQuerySet> mediaQueries = MediaQuerySet::createAllowingDescriptionSyntax(attributeValue);
+    MediaQueryEvaluator mediaQueryEvaluator("screen", frame, renderStyle);
+    return mediaQueryEvaluator.eval(mediaQueries.get());
+}
+
 void HTMLResourcePreloader::preload(PassOwnPtr<PreloadRequest> preload)
 {
+    ASSERT(m_document->frame());
+    ASSERT(m_document->renderer());
+    ASSERT(m_document->renderer()->style());
+    if (!preload->media().isEmpty() && !mediaAttributeMatches(m_document->frame(), m_document->renderer()->style(), preload->media()))
+        return;
+
     CachedResourceRequest request = preload->resourceRequest(m_document);
     m_document->cachedResourceLoader()->preload(preload->resourceType(), request, preload->charset());
 }
 
+
 }
index 22eaa3a..aec78cd 100644 (file)
@@ -33,9 +33,14 @@ namespace WebCore {
 
 class PreloadRequest {
 public:
+    static PassOwnPtr<PreloadRequest> create(const String& initiator, const String& resourceURL, const KURL& baseURL, CachedResource::Type resourceType, const String& mediaAttribute)
+    {
+        return adoptPtr(new PreloadRequest(initiator, resourceURL, baseURL, resourceType, mediaAttribute));
+    }
+
     static PassOwnPtr<PreloadRequest> create(const String& initiator, const String& resourceURL, const KURL& baseURL, CachedResource::Type resourceType)
     {
-        return adoptPtr(new PreloadRequest(initiator, resourceURL, baseURL, resourceType));
+        return adoptPtr(new PreloadRequest(initiator, resourceURL, baseURL, resourceType, String()));
     }
 
     bool isSafeToSendToAnotherThread() const;
@@ -43,16 +48,18 @@ public:
     CachedResourceRequest resourceRequest(Document*);
 
     const String& charset() const { return m_charset; }
+    const String& media() const { return m_mediaAttribute; }
     void setCharset(const String& charset) { m_charset = charset.isolatedCopy(); }
     void setCrossOriginModeAllowsCookies(bool allowsCookies) { m_crossOriginModeAllowsCookies = allowsCookies; }
     CachedResource::Type resourceType() const { return m_resourceType; }
 
 private:
-    PreloadRequest(const String& initiator, const String& resourceURL, const KURL& baseURL, CachedResource::Type resourceType)
+    PreloadRequest(const String& initiator, const String& resourceURL, const KURL& baseURL, CachedResource::Type resourceType, const String& mediaAttribute)
         : m_initiator(initiator)
         , m_resourceURL(resourceURL.isolatedCopy())
         , m_baseURL(baseURL.copy())
         , m_resourceType(resourceType)
+        , m_mediaAttribute(mediaAttribute.isolatedCopy())
         , m_crossOriginModeAllowsCookies(false)
     {
     }
@@ -64,6 +71,7 @@ private:
     KURL m_baseURL;
     String m_charset;
     CachedResource::Type m_resourceType;
+    String m_mediaAttribute;
     bool m_crossOriginModeAllowsCookies;
 };