Refactor the forPreload flag to mean speculative preload.

third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp

Index: third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
diff --git a/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp b/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
index da7d65d99f6abaa313a92c605a8a13a52b99c444..11cba5cb882888ab842ed80530d73324a2233ce8 100644
--- a/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
@@ -157,7 +157,7 @@ ResourceLoadPriority ResourceFetcher::computeLoadPriority(
     const ResourceRequest& resourceRequest,
     ResourcePriority::VisibilityStatus visibility,
     FetchRequest::DeferOption deferOption,
-    bool forPreload) {
+    bool speculativePreload) {
   ResourceLoadPriority priority = typeToPriority(type);
 
   // Visible resources (images in practice) get a boost to High priority.
@@ -180,10 +180,12 @@ ResourceLoadPriority ResourceFetcher::computeLoadPriority(
     // typeToPriority)
     // Async/Defer: Low Priority (applies to both preload and parser-inserted)
     // Preload late in document: Medium
-    if (FetchRequest::LazyLoad == deferOption)
+    if (FetchRequest::LazyLoad == deferOption) {
       priority = ResourceLoadPriorityLow;
-    else if (forPreload && m_imageFetched)
+    } else if (speculativePreload && m_imageFetched) {
+      // TODO(yoav): It is not clear why this applies only to preloaded scripts.
       priority = ResourceLoadPriorityMedium;
+    }
   } else if (FetchRequest::LazyLoad == deferOption) {
     priority = ResourceLoadPriorityVeryLow;
   }
@@ -276,8 +278,9 @@ bool ResourceFetcher::isControlledByServiceWorker() const {
 bool ResourceFetcher::resourceNeedsLoad(Resource* resource,
                                         const FetchRequest& request,
                                         RevalidationPolicy policy) {
-  // Defer a font load until it is actually needed unless this is a preload.
-  if (resource->getType() == Resource::Font && !request.forPreload())
+  // Defer a font load until it is actually needed unless this is a link
+  // preload.

[Charlie Harrison, 2017/02/05 18:39:12]

I think this should still be for all preloads, not just link preloads. AFAIK we
don't have a way currently to speculatively preload fonts, but in principle we
should be able to if we had a robust enough speculation.

[Yoav Weiss, 2017/02/06 09:58:35]

I don't know that it's worth-while to pay for an extra condition here just in
case we would start preloading fonts in the future. Wouldn't it be better to add
that condition once it's relevant?
Can we perhaps add a comment somewhere in the preloadScanner code to point folks
that may attempt it here? (Not sure where would be the ideal place for such a
comment)

[Charlie Harrison, 2017/02/06 19:54:15]

Your call, it isn't a huge deal. Probably the best place for an extra comment
would be in the css preload scanner maybe?

+  if (resource->getType() == Resource::Font && !request.isLinkPreload())
     return false;
   if (resource->isImage() && shouldDeferImageLoad(resource->url()))
     return false;
@@ -423,7 +426,7 @@ void ResourceFetcher::moveCachedNonBlockingResourceToBlocking(
   if (resource && resource->loader() &&
       resource->isLoadEventBlockingResourceType() &&
       m_nonBlockingLoaders.contains(resource->loader()) &&
-      resource->isLinkPreload() && !request.forPreload()) {
+      resource->isLinkPreload() && !request.isLinkPreload()) {

[Charlie Harrison, 2017/02/05 18:39:12]

nit: can we move the isLinkPreload and !isLinkPreload before the contains() call
for efficiency?

[Yoav Weiss, 2017/02/06 09:58:35]

Good call

     m_nonBlockingLoaders.remove(resource->loader());

[Charlie Harrison, 2017/02/05 18:39:12]

I am slightly confused what this method does, do you mind adding some
documentation?

[Yoav Weiss, 2017/02/06 09:58:35]

renamed the method to a hopefully clearer name, and improved comment on the call
site. Let me know if you think further documentation is needed :)

     m_loaders.insert(resource->loader());
   }
@@ -437,7 +440,7 @@ void ResourceFetcher::updateMemoryCacheStats(Resource* resource,
   if (isStaticData)
     return;
 
-  if (request.forPreload()) {
+  if (request.speculativePreload() || request.isLinkPreload()) {
     DEFINE_RESOURCE_HISTOGRAM("Preload.");
   } else {
     DEFINE_RESOURCE_HISTOGRAM("");
@@ -474,7 +477,7 @@ ResourceFetcher::PrepareRequestResult ResourceFetcher::prepareRequest(
 
   resourceRequest.setPriority(computeLoadPriority(
       factory.type(), request.resourceRequest(), ResourcePriority::NotVisible,
-      request.defer(), request.forPreload()));
+      request.defer(), request.speculativePreload()));
   initializeResourceRequest(resourceRequest, factory.type(), request.defer());
   network_instrumentation::resourcePrioritySet(identifier,
                                                resourceRequest.priority());
@@ -482,7 +485,8 @@ ResourceFetcher::PrepareRequestResult ResourceFetcher::prepareRequest(
   blockedReason = context().canRequest(
       factory.type(), resourceRequest,
       MemoryCache::removeFragmentIdentifierIfNeeded(request.url()),
-      request.options(), request.forPreload(), request.getOriginRestriction());
+      request.options(), request.speculativePreload(),
+      request.getOriginRestriction());
   if (blockedReason != ResourceRequestBlockedReason::None) {
     DCHECK(!substituteData.forceSynchronousLoad());
     return Block;
@@ -490,7 +494,7 @@ ResourceFetcher::PrepareRequestResult ResourceFetcher::prepareRequest(
 
   context().willStartLoadingResource(
       identifier, resourceRequest, factory.type(),
-      request.options().initiatorInfo.name, request.forPreload());
+      request.options().initiatorInfo.name, request.speculativePreload());
   if (!request.url().isValid())
     return Abort;
 
@@ -568,7 +572,8 @@ Resource* ResourceFetcher::requestResource(
   if (!resource)
     return nullptr;
   if (resource->getType() != factory.type()) {
-    DCHECK(request.forPreload());
+    DCHECK(request.speculativePreload() || request.isLinkPreload());
+    // TODO(yoav): What's the scenario where this can actually happen?
     return nullptr;
   }
 
@@ -578,7 +583,9 @@ Resource* ResourceFetcher::requestResource(
   if (policy != Use)
     resource->setIdentifier(identifier);
 
-  if (!request.forPreload() || policy != Use) {
+  // TODO(yoav): It is not clear why preloads are exempt from this check. Can we
+  // remove the exemption?
+  if (!request.speculativePreload() || policy != Use) {
     // When issuing another request for a resource that is already in-flight
     // make sure to not demote the priority of the in-flight request. If the new
     // request isn't at the same priority as the in-flight request, only allow
@@ -587,6 +594,8 @@ Resource* ResourceFetcher::requestResource(
     // lower priority).
     if (resourceRequest.priority() > resource->resourceRequest().priority())
       resource->didChangePriority(resourceRequest.priority(), 0);
+    // TODO(yoav): I'd expect the stated scenario to not go here, as its policy
+    // would be Use.
   }
 
   // If only the fragment identifiers differ, it is the same resource.
@@ -708,7 +717,7 @@ Resource* ResourceFetcher::createResourceForLoading(
   Resource* resource =
       factory.create(request.resourceRequest(), request.options(), charset);
   resource->setLinkPreload(request.isLinkPreload());
-  if (request.forPreload()) {
+  if (request.speculativePreload()) {
     resource->setPreloadDiscoveryTime(request.preloadDiscoveryTime());
   }
   resource->setCacheIdentifier(cacheIdentifier);
@@ -811,7 +820,7 @@ ResourceFetcher::determineRevalidationPolicy(Resource::Type type,
     return Reload;
 
   // We already have a preload going for this URL.
-  if (fetchRequest.forPreload() && existingResource->isPreloaded())
+  if (fetchRequest.speculativePreload() && existingResource->isPreloaded())
     return Use;
 
   // If the same URL has been loaded as a different type, we need to reload.