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..47f9ada068b98f74aa8f6e4d085aaccf1462a238 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.

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

I did some blaming to trace this logic. It stems from pmeenans priority work. I
think the problem was that the preload scanner would scan too far ahead in some
cases, treating script way ahead in the document as more important than it
should be. Arguably we could apply that logic to *all* scripts late in the
document, but the preload bit is an OK signal that we can probably lower
priority.

[Yoav Weiss, 2017/02/06 23:15:49]

Changed to a comment stating the above

       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.
+  if (resource->getType() == Resource::Font && !request.isLinkPreload())
     return false;
   if (resource->isImage() && shouldDeferImageLoad(resource->url()))
     return false;
@@ -415,15 +418,15 @@ Resource* ResourceFetcher::resourceForBlockedRequest(
   return resource;
 }
 
-void ResourceFetcher::moveCachedNonBlockingResourceToBlocking(
+void ResourceFetcher::makePreloadedResourceBlockOnloadIfNeeded(
     Resource* resource,
     const FetchRequest& request) {
   // TODO(yoav): Test that non-blocking resources (video/audio/track) continue
   // to not-block even after being preloaded and discovered.
   if (resource && resource->loader() &&
       resource->isLoadEventBlockingResourceType() &&
-      m_nonBlockingLoaders.contains(resource->loader()) &&
-      resource->isLinkPreload() && !request.forPreload()) {
+      resource->isLinkPreload() && !request.isLinkPreload() &&
+      m_nonBlockingLoaders.contains(resource->loader())) {
     m_nonBlockingLoaders.remove(resource->loader());
     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()) {

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

nit: seeing these together brings out the inconsistency in naming. Can we rename
to isSpeculativePreload? That better adheres to blink style.

[Yoav Weiss, 2017/02/06 23:15:49]

changed

     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,10 @@ ResourceFetcher::PrepareRequestResult ResourceFetcher::prepareRequest(
   blockedReason = context().canRequest(
       factory.type(), resourceRequest,
       MemoryCache::removeFragmentIdentifierIfNeeded(request.url()),
-      request.options(), request.forPreload(), request.getOriginRestriction());
+      request.options(), request.speculativePreload()
+                             ? FetchContext::ReportingPolicy::SuppressReporting
+                             : FetchContext::ReportingPolicy::Report,
+      request.getOriginRestriction());
   if (blockedReason != ResourceRequestBlockedReason::None) {
     DCHECK(!substituteData.forceSynchronousLoad());
     return Block;
@@ -490,7 +496,10 @@ ResourceFetcher::PrepareRequestResult ResourceFetcher::prepareRequest(
 
   context().willStartLoadingResource(
       identifier, resourceRequest, factory.type(),
-      request.options().initiatorInfo.name, request.forPreload());
+      request.options().initiatorInfo.name,
+      (request.speculativePreload()
+           ? FetchContext::LoggingPolicy::SuppressLogging
+           : FetchContext::LoggingPolicy::Log));
   if (!request.url().isValid())
     return Abort;
 
@@ -539,9 +548,9 @@ Resource* ResourceFetcher::requestResource(
         memoryCache()->resourceForURL(request.url(), getCacheIdentifier());
   }
 
-  // See if we can use an existing resource from the cache. If so, we need to
-  // move it to be load blocking.
-  moveCachedNonBlockingResourceToBlocking(resource, request);
+  // If we got a preloaded resource from the cache for a non-preload request,
+  // we may need to make it block the onload event.
+  makePreloadedResourceBlockOnloadIfNeeded(resource, request);
 
   const RevalidationPolicy policy = determineRevalidationPolicy(
       factory.type(), request, resource, isStaticData);
@@ -568,7 +577,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?

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

I can't think/see of any and this code is decently old (~June 2015). I would
support removing this block and adding CHECK_EQ(resource->getType(),
factory.type()). Possibly CQ will complain if this actually gets hit.

[Yoav Weiss, 2017/02/06 23:15:49]

I'll do that in a followup

     return nullptr;
   }
 
@@ -578,7 +588,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

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

This logic is very old [1], and priority calculations based on preloads should
not live here. Let's try changing it in a follow-up?

[1]
https://chromium.googlesource.com/chromium/src/+/79ff7ef8ea33fa66ff2792a7c432...

[Yoav Weiss, 2017/02/06 23:15:49]

sure

+  // 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 +599,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 +722,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 +825,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.
@@ -1479,7 +1493,8 @@ void ResourceFetcher::emulateLoadStartedForInspector(
   FetchRequest request(resourceRequest, initiatorName, resource->options());
   context().canRequest(resource->getType(), resource->lastResourceRequest(),
                        resource->lastResourceRequest().url(), request.options(),
-                       false, request.getOriginRestriction());
+                       FetchContext::ReportingPolicy::Report,
+                       request.getOriginRestriction());
   requestLoadStarted(resource->identifier(), resource, request,
                      ResourceLoadingFromCache);
 }