Fix HTMLPreloadScanner handling of type in link preload.

third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp

Index: third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
diff --git a/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp b/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
index e224331a875c29409d023e158995ded88b9199b2..116644090e399c51f12bc3cd8cfebe87fc382785 100644
--- a/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
+++ b/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
@@ -120,7 +120,7 @@ static String initiatorFor(const StringImpl* tagImpl) {
     return scriptTag.localName();
   if (match(tagImpl, videoTag))
     return videoTag.localName();
-  ASSERT_NOT_REACHED();
+  NOTREACHED();
   return emptyString();
 }
 
@@ -205,13 +205,17 @@ class TokenPreloadScanner::StartTagScanner {
       const ReferrerPolicy documentReferrerPolicy) {
     PreloadRequest::RequestType requestType =
         PreloadRequest::RequestTypePreload;
+    Optional<Resource::Type> type;

[Charlie Harrison, 2017/01/10 14:35:06]

#include "wtf/Optional.h"

[Yoav Weiss, 2017/01/11 05:14:36]

Added

     if (shouldPreconnect()) {
       requestType = PreloadRequest::RequestTypePreconnect;
     } else {
       if (isLinkRelPreload()) {
         requestType = PreloadRequest::RequestTypeLinkRelPreload;
+        type = resourceTypeForLinkPreload();
+        if (type == WTF::nullopt)
+          return nullptr;
       }
-      if (!shouldPreload()) {
+      if (!shouldPreload(type)) {
         return nullptr;
       }
     }
@@ -230,9 +234,8 @@ class TokenPreloadScanner::StartTagScanner {
       resourceWidth.isSet = true;
     }
 
-    Resource::Type type;
-    if (!resourceType(type))
-      return nullptr;
+    if (type == WTF::nullopt)
+      type = resourceType();
 
     // The element's 'referrerpolicy' attribute (if present) takes precedence
     // over the document's referrer policy.
@@ -240,8 +243,9 @@ class TokenPreloadScanner::StartTagScanner {
                                         ? m_referrerPolicy
                                         : documentReferrerPolicy;
     auto request = PreloadRequest::createIfNeeded(
-        initiatorFor(m_tagImpl), position, m_urlToLoad, predictedBaseURL, type,
-        referrerPolicy, resourceWidth, clientHintsPreferences, requestType);
+        initiatorFor(m_tagImpl), position, m_urlToLoad, predictedBaseURL,
+        type.value(), referrerPolicy, resourceWidth, clientHintsPreferences,
+        requestType);
     if (!request)
       return nullptr;
 
@@ -349,8 +353,7 @@ class TokenPreloadScanner::StartTagScanner {
     } else if (match(attributeName, asAttr)) {
       m_asAttributeValue = attributeValue.lower();
     } else if (match(attributeName, typeAttr)) {
-      m_matched &= MIMETypeRegistry::isSupportedStyleSheetMIMEType(
-          ContentType(attributeValue).type());
+      m_typeAttributeValue = attributeValue;
     } else if (!m_referrerPolicySet &&
                match(attributeName, referrerpolicyAttr) &&
                !attributeValue.isNull()) {
@@ -443,25 +446,26 @@ class TokenPreloadScanner::StartTagScanner {
     return m_charset;
   }
 
-  bool resourceType(Resource::Type& type) const {
+  Optional<Resource::Type> resourceTypeForLinkPreload() const {
+    DCHECK(m_linkIsPreload);
+    return LinkLoader::getResourceTypeFromAsAttribute(m_asAttributeValue);
+  }
+
+  Resource::Type resourceType() const {
     if (match(m_tagImpl, scriptTag)) {
-      type = Resource::Script;
+      return Resource::Script;
     } else if (match(m_tagImpl, imgTag) || match(m_tagImpl, videoTag) ||
                (match(m_tagImpl, inputTag) && m_inputIsImage)) {
-      type = Resource::Image;
+      return Resource::Image;
     } else if (match(m_tagImpl, linkTag) && m_linkIsStyleSheet) {
-      type = Resource::CSSStyleSheet;
+      return Resource::CSSStyleSheet;
     } else if (m_linkIsPreconnect) {
-      type = Resource::Raw;
-    } else if (m_linkIsPreload) {
-      if (!LinkLoader::getResourceTypeFromAsAttribute(m_asAttributeValue, type))
-        return false;
+      return Resource::Raw;
     } else if (match(m_tagImpl, linkTag) && m_linkIsImport) {
-      type = Resource::ImportResource;
-    } else {
-      ASSERT_NOT_REACHED();
+      return Resource::ImportResource;
     }
-    return true;
+    NOTREACHED();
+    return Resource::Raw;
   }
 
   bool shouldPreconnect() const {
@@ -474,14 +478,44 @@ class TokenPreloadScanner::StartTagScanner {
            !m_urlToLoad.isEmpty();
   }
 
-  bool shouldPreload() const {
+  bool shouldPreloadLink(Optional<Resource::Type>& type) const {
+    if (m_linkIsStyleSheet) {
+      if (!m_typeAttributeValue.isEmpty() &&

[Charlie Harrison, 2017/01/10 14:35:06]

This inner if could be rewritten as

return m_typeAttributeValue.isEmpty() ||
MIMETypeRegistry::isSupportedStyleSheetMIMEType(ContentType(m_typeAttributeValue).type());

This way we avoid the implicit "return true" at the bottom of the method for
this condition. I also think it's more readable.

[Yoav Weiss, 2017/01/11 05:14:36]

done

+          !MIMETypeRegistry::isSupportedStyleSheetMIMEType(
+              ContentType(m_typeAttributeValue).type()))
+        return false;
+    } else if (m_linkIsPreload) {
+      if (!m_typeAttributeValue.isEmpty()) {

[Charlie Harrison, 2017/01/10 14:35:06]

optional: It might make sense to also have an early return for
m_typeAttributeValue.isEmpty() at the top of the method, so we don't so many
need nested conditionals.

[Yoav Weiss, 2017/01/11 05:14:36]

done

+        String typeFromAttribute = ContentType(m_typeAttributeValue).type();
+        if (type == Resource::Font &&

[Charlie Harrison, 2017/01/10 14:35:06]

These three conditions could be combined to:

if (type == Resource::Font || type == Resource::Image || type ==
Resource::CSSStyleSheet) {
  return
MIMETypeRegistry::isSupportedStyleSheetMIMEType(ContentType(m_typeAttributeValue).type());
}

[Yoav Weiss, 2017/01/11 05:14:36]

Not really, as the isSupported* function call in each is a different one. Still
I tried to make the condition more compact.

+            !MIMETypeRegistry::isSupportedFontMIMEType(typeFromAttribute)) {
+          return false;
+        }
+        if (type == Resource::Image &&
+            !MIMETypeRegistry::isSupportedImagePrefixedMIMEType(
+                typeFromAttribute)) {
+          return false;
+        }
+        if (type == Resource::CSSStyleSheet &&
+            !MIMETypeRegistry::isSupportedStyleSheetMIMEType(
+                typeFromAttribute)) {
+          return false;
+        }
+      }
+    } else if (!m_linkIsImport) {
+      return false;
+    }
+
+    return true;
+  }
+
+  bool shouldPreload(Optional<Resource::Type>& type) const {
     if (m_urlToLoad.isEmpty())
       return false;
     if (!m_matched)
       return false;
-    if (match(m_tagImpl, linkTag) && !m_linkIsStyleSheet && !m_linkIsImport &&
-        !m_linkIsPreload)
-      return false;
+    if (match(m_tagImpl, linkTag))
+      return shouldPreloadLink(type);
     if (match(m_tagImpl, inputTag) && !m_inputIsImage)
       return false;
     if (match(m_tagImpl, scriptTag) &&