Refactor the forPreload flag to mean speculative preload.

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

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All
     rights reserved.
     Copyright (C) 2009 Torch Mobile Inc. http://www.torchmobile.com/
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
@@ skipping 139 matching lines @@
   return ResourceLoadPriorityUnresolved;
 }
 
 }  // namespace
 
 ResourceLoadPriority ResourceFetcher::computeLoadPriority(
     Resource::Type type,
     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.
   if (visibility == ResourcePriority::Visible)
     priority = ResourceLoadPriorityHigh;
 
   // Resources before the first image are considered "early" in the document and
   // resources after the first image are "late" in the document.  Important to
   // note that this is based on when the preload scanner discovers a resource
   // for the most part so the main parser may not have reached the image element
   // yet.
   if (type == Resource::Image)
     m_imageFetched = true;
 
   if (FetchRequest::IdleLoad == deferOption) {
     priority = ResourceLoadPriorityVeryLow;
   } else if (type == Resource::Script) {
     // Special handling for scripts.
     // Default/Parser-Blocking/Preload early in document: High (set in
     // 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;
   }
 
   // A manually set priority acts as a floor. This is used to ensure that
   // synchronous requests are always given the highest possible priority, as
   // well as to ensure that there isn't priority churn if images move in and out
   // of the viewport, or is displayed more than once, both in and out of the
   // viewport.
   return std::max(context().modifyPriorityForExperiments(priority),
@@ skipping 72 matching lines @@
   return resource.get();
 }
 
 bool ResourceFetcher::isControlledByServiceWorker() const {
   return context().isControlledByServiceWorker();
 }
 
 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;
   return policy != Use || resource->stillNeedsLoad();
 }
 
 // Limit the number of URLs in m_validatedURLs to avoid memory bloat.
 // http://crbug.com/52411
 static const int kMaxValidatedURLsSize = 10000;
 
@@ skipping 125 matching lines @@
 }
 
 void ResourceFetcher::moveCachedNonBlockingResourceToBlocking(
     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()) {

[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());
   }
 }
 
 void ResourceFetcher::updateMemoryCacheStats(Resource* resource,
                                              RevalidationPolicy policy,
                                              const FetchRequest& request,
                                              const ResourceFactory& factory,
                                              bool isStaticData) const {
   if (isStaticData)
     return;
 
-  if (request.forPreload()) {
+  if (request.speculativePreload() || request.isLinkPreload()) {
     DEFINE_RESOURCE_HISTOGRAM("Preload.");
   } else {
     DEFINE_RESOURCE_HISTOGRAM("");
   }
 
   // Aims to count Resource only referenced from MemoryCache (i.e. what would be
   // dead if MemoryCache holds weak references to Resource). Currently we check
   // references to Resource from ResourceClient and |m_preloads| only, because
   // they are major sources of references.
   if (resource && !resource->isAlive() &&
@@ skipping 16 matching lines @@
 
   context().populateResourceRequest(
       factory.type(), request.clientHintsPreferences(),
       request.getResourceWidth(), resourceRequest);
 
   if (!request.url().isValid())
     return Abort;
 
   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());
 
   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;
   }
 
   context().willStartLoadingResource(
       identifier, resourceRequest, factory.type(),
-      request.options().initiatorInfo.name, request.forPreload());
+      request.options().initiatorInfo.name, request.speculativePreload());
   if (!request.url().isValid())
     return Abort;
 
   resourceRequest.setAllowStoredCredentials(
       request.options().allowCredentials == AllowStoredCredentials);
   return Continue;
 }
 
 Resource* ResourceFetcher::requestResource(
     FetchRequest& request,
@@ skipping 57 matching lines @@
       initializeRevalidation(resourceRequest, resource);
       break;
     case Use:
       if (resource->isLinkPreload() && !request.isLinkPreload())
         resource->setLinkPreload(false);
       break;
   }
   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;
   }
 
   if (!resource->isAlive())
     m_deadStatsRecorder.update(policy);
 
   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
     // promotions. This can happen when a visible image's priority is increased
     // and then another reference to the image is parsed (which would be at a
     // 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.
   DCHECK(equalIgnoringFragmentIdentifier(resource->url(), request.url()));
   requestLoadStarted(
       identifier, resource, request,
       policy == Use ? ResourceLoadingFromCache : ResourceLoadingFromNetwork,
       isStaticData);
   m_documentResources.set(
       MemoryCache::removeFragmentIdentifierIfNeeded(request.url()), resource);
@@ skipping 101 matching lines @@
   const String cacheIdentifier = getCacheIdentifier();
   DCHECK(!memoryCache()->resourceForURL(request.resourceRequest().url(),
                                         cacheIdentifier));
 
   RESOURCE_LOADING_DVLOG(1) << "Loading Resource for "
                             << request.resourceRequest().url().elidedString();
 
   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);
 
   // - Don't add main resource to cache to prevent reuse.
   // - Don't add the resource if its body will not be stored.
   if (factory.type() != Resource::MainResource &&
       request.options().dataBufferingPolicy != DoNotBufferData) {
     memoryCache()->add(resource);
   }
@@ skipping 82 matching lines @@
   if (existingResource->mustRefetchDueToIntegrityMetadata(fetchRequest)) {
     RecordSriResourceIntegrityMismatchEvent(RefetchDueToIntegrityMismatch);
     return Reload;
   }
 
   // Service Worker's CORS fallback message must not be cached.
   if (existingResource->response().wasFallbackRequiredByServiceWorker())
     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.
   if (existingResource->getType() != type) {
     // FIXME: If existingResource is a Preload and the new type is LinkPrefetch
     // We really should discard the new prefetch since the preload has more
     // specific type information! crbug.com/379893
     // fast/dom/HTMLLinkElement/link-and-subresource-test hits this case.
     RESOURCE_LOADING_DVLOG(1) << "ResourceFetcher::determineRevalidationPolicy "
                                  "reloading due to type mismatch.";
@@ skipping 700 matching lines @@
   visitor->trace(m_context);
   visitor->trace(m_archive);
   visitor->trace(m_loaders);
   visitor->trace(m_nonBlockingLoaders);
   visitor->trace(m_documentResources);
   visitor->trace(m_preloads);
   visitor->trace(m_resourceTimingInfoMap);
 }
 
 }  // namespace blink