Fix HTMLPreloadScanner handling of type in link preload.

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

 /*
  * Copyright (C) 2008 Apple Inc. All Rights Reserved.
  * Copyright (C) 2009 Torch Mobile, Inc. http://www.torchmobile.com/
  * Copyright (C) 2010 Google Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
@@ skipping 102 matching lines @@
   if (match(tagImpl, imgTag))
     return imgTag.localName();
   if (match(tagImpl, inputTag))
     return inputTag.localName();
   if (match(tagImpl, linkTag))
     return linkTag.localName();
   if (match(tagImpl, scriptTag))
     return scriptTag.localName();
   if (match(tagImpl, videoTag))
     return videoTag.localName();
-  ASSERT_NOT_REACHED();
+  NOTREACHED();
   return emptyString();
 }
 
 static bool mediaAttributeMatches(const MediaValuesCached& mediaValues,
                                   const String& attributeValue) {
   MediaQuerySet* mediaQueries = MediaQuerySet::create(attributeValue);
   MediaQueryEvaluator mediaQueryEvaluator(mediaValues);
   return mediaQueryEvaluator.eval(mediaQueries);
 }
 
@@ skipping 64 matching lines @@
   }
 
   std::unique_ptr<PreloadRequest> createPreloadRequest(
       const KURL& predictedBaseURL,
       const SegmentedString& source,
       const ClientHintsPreferences& clientHintsPreferences,
       const PictureData& pictureData,
       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;
       }
     }
 
     TextPosition position =
         TextPosition(source.currentLine(), source.currentColumn());
     FetchRequest::ResourceWidth resourceWidth;
     float sourceSize = m_sourceSize;
     bool sourceSizeSet = m_sourceSizeSet;
     if (pictureData.picked) {
       sourceSizeSet = pictureData.sourceSizeSet;
       sourceSize = pictureData.sourceSize;
     }
     if (sourceSizeSet) {
       resourceWidth.width = sourceSize;
       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.
     ReferrerPolicy referrerPolicy = (m_referrerPolicy != ReferrerPolicyDefault)
                                         ? 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;
 
     request->setCrossOrigin(m_crossOrigin);
     request->setNonce(m_nonce);
     request->setCharset(charset());
     request->setDefer(m_defer);
     request->setIntegrityMetadata(m_integrityMetadata);
 
     return request;
@@ skipping 87 matching lines @@
       m_linkIsImport = rel.isImport();
     } else if (match(attributeName, mediaAttr)) {
       m_matched &= mediaAttributeMatches(*m_mediaValues, attributeValue);
     } else if (match(attributeName, crossoriginAttr)) {
       setCrossOrigin(attributeValue);
     } else if (match(attributeName, nonceAttr)) {
       setNonce(attributeValue);
     } 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()) {
       m_referrerPolicySet = true;
       SecurityPolicy::referrerPolicyFromString(attributeValue,
                                                &m_referrerPolicy);
     }
   }
 
   template <typename NameType>
@@ skipping 72 matching lines @@
   }
 
   const String& charset() const {
     // FIXME: Its not clear that this if is needed, the loader probably ignores
     // charset for image requests anyway.
     if (match(m_tagImpl, imgTag) || match(m_tagImpl, videoTag))
       return emptyString();
     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 {
     return match(m_tagImpl, linkTag) && m_linkIsPreconnect &&
            !m_urlToLoad.isEmpty();
   }
 
   bool isLinkRelPreload() const {
     return match(m_tagImpl, linkTag) && m_linkIsPreload &&
            !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) &&
         !ScriptLoader::isValidScriptTypeAndLanguage(
             m_typeAttributeValue, m_languageAttributeValue,
             ScriptLoader::AllowLegacyTypeInTypeAttribute)) {
       return false;
     }
     return true;
   }
@@ skipping 397 matching lines @@
   defaultViewportMinWidth = document->viewportDefaultMinWidth();
   viewportMetaZeroValuesQuirk =
       document->settings() &&
       document->settings()->getViewportMetaZeroValuesQuirk();
   viewportMetaEnabled =
       document->settings() && document->settings()->getViewportMetaEnabled();
   referrerPolicy = document->getReferrerPolicy();
 }
 
 }  // namespace blink