Make ResourceLoadPriority calculation simpler

third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp

Index: third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
index 2a8f5d8b0e7051695131fc84db5839fbbbbcaf66..660d71494bd406633d6f63030d96020cd9e71e6b 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
@@ -134,16 +134,8 @@ static ResourceLoadPriority typeToPriority(Resource::Type type)
     return ResourceLoadPriorityUnresolved;
 }
 
-ResourceLoadPriority ResourceFetcher::loadPriority(Resource::Type type, const FetchRequest& request, ResourcePriority::VisibilityStatus visibility)
+ResourceLoadPriority ResourceFetcher::computeLoadPriority(Resource::Type type, const FetchRequest& request, ResourcePriority::VisibilityStatus visibility)
 {
-    // TODO(yoav): Change it here so that priority can be changed even after it was resolved.
-    if (request.priority() != ResourceLoadPriorityUnresolved)
-        return request.priority();
-
-    // Synchronous requests should always be max priority, lest they hang the renderer.
-    if (request.options().synchronousPolicy == RequestSynchronously)
-        return ResourceLoadPriorityHighest;
-
     ResourceLoadPriority priority = typeToPriority(type);
 
     // Visible resources (images in practice) get a boost to High priority.
@@ -166,9 +158,14 @@ ResourceLoadPriority ResourceFetcher::loadPriority(Resource::Type type, const Fe
             priority = ResourceLoadPriorityLow;
         else if (request.forPreload() && m_imageFetched)
             priority = ResourceLoadPriorityMedium;
+    } else if (FetchRequest::LazyLoad == request.defer()) {
+        priority = ResourceLoadPriorityVeryLow;

[Yoav Weiss, 2016/06/25 08:37:59]

If I understand it correctly, this would mean that deferred and async scripts
will now be downloaded with very low priority. Is that the case? If so, is this
intentional? Seems like a significant change from current behavior.

[Pat Meenan, 2016/06/27 14:56:55]

Scripts go through the first clause and shouldn't be changed.  This basically
just makes anything else that gets tagged as defer load at veryLow (AFAIK,
scripts previously were the only thing with the LazyLoad setting so this
shouldn't change any existing behavior.

[Nate Chapin, 2016/06/27 19:46:16]

Correct.

     }
 
-    return context().modifyPriorityForExperiments(priority);
+    // 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.
+    return std::max(context().modifyPriorityForExperiments(priority), request.resourceRequest().priority());

[Yoav Weiss, 2016/06/25 08:37:59]

In the image case, the "priority churn" can also have advantages, if the user
flicks through some images which we haven't yet downloaded, it might be better
to give higher priority to the ones that are in current viewport over ones in
past viewports. Is there a cost to said churn?

[Pat Meenan, 2016/06/27 14:56:55]

We had logic previously to prevent exactly this churn because it also applies to
the same image being reference in the viewport as well as late in the viewport. 
We want to make sure not to incorrectly decrease the priority of a resource that
is still in the viewport.  Dealing with the fast scroll case would actually be
new behavior (and should be done carefully if we want to bother).

[Nate Chapin, 2016/06/27 19:46:16]

I added a bit more detail to this comment, please review for clarity/correctness
:)

 }
 
 static void populateResourceTiming(ResourceTimingInfo* info, Resource* resource)
@@ -423,8 +420,7 @@ Resource* ResourceFetcher::requestResource(FetchRequest& request, const Resource
         return nullptr;
 
     unsigned long identifier = createUniqueIdentifier();
-    request.setPriority(loadPriority(factory.type(), request, ResourcePriority::NotVisible));
-    request.mutableResourceRequest().setPriority(request.priority());
+    request.mutableResourceRequest().setPriority(computeLoadPriority(factory.type(), request, ResourcePriority::NotVisible));
     initializeResourceRequest(request.mutableResourceRequest(), factory.type(), request.defer());
     context().willStartLoadingResource(identifier, request.mutableResourceRequest(), factory.type());
     if (!request.url().isValid())
@@ -500,8 +496,8 @@ Resource* ResourceFetcher::requestResource(FetchRequest& request, const Resource
         // 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 (request.priority() > resource->resourceRequest().priority())
-            resource->didChangePriority(request.priority(), 0);
+        if (request.resourceRequest().priority() > resource->resourceRequest().priority())
+            resource->didChangePriority(request.resourceRequest().priority(), 0);
     }
 
     // If only the fragment identifiers differ, it is the same resource.
@@ -1077,7 +1073,7 @@ void ResourceFetcher::updateAllImageResourcePriorities()
             continue;
 
         ResourcePriority resourcePriority = resource->priorityFromObservers();
-        ResourceLoadPriority resourceLoadPriority = loadPriority(Resource::Image, FetchRequest(resource->resourceRequest(), FetchInitiatorInfo()), resourcePriority.visibility);
+        ResourceLoadPriority resourceLoadPriority = computeLoadPriority(Resource::Image, FetchRequest(resource->resourceRequest(), FetchInitiatorInfo()), resourcePriority.visibility);
         if (resourceLoadPriority == resource->resourceRequest().priority())
             continue;