Add Blink support for showing image placeholders using range requests.

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 8e4b279b3fa0fdc786943babf6627663822abd35..82727f36a3e1200fbc748e075bf5ad78821c5510 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
@@ -480,6 +480,30 @@ void ResourceFetcher::updateMemoryCacheStats(Resource* resource,
   }
 }
 
+static void modifyRequestForPlaceholderImageRequest(FetchRequest& request,
+                                                    Resource::Type type) {
+  if (request.placeholderImageRequestType() != FetchRequest::AllowPlaceholder)
+    return;
+  DCHECK_EQ(Resource::Image, type);
+
+  if (!request.url().protocolIsInHTTPFamily() ||
+      request.resourceRequest().httpMethod() != "GET" ||
+      !request.resourceRequest().httpHeaderField("range").isNull()) {
+    request.setPlaceholderImageRequestType(FetchRequest::DisallowPlaceholder);

[Nate Chapin, 2016/10/18 18:23:17]

Everything in this function should probably all be done as part of
setPlaceholderImageRequestType()?

Also, this seems like the only place where setPlaceholderImageRequestType
changes an AllowPlaceholder to a DisallowPlaceholder. Can we change it to
setAllowImagePlaceholder() with no arguments, since the default will always be
DisallowPlaceholder?

[sclittle, 2016/10/18 23:53:03]

Done.

+    return;
+  }
+
+  // Fetch the first few bytes of the image. This number is tuned to both (a)
+  // likely capture the entire image for small images and (b) likely contain
+  // the dimensions for larger images.
+  // TODO(sclittle): Calculate the optimal value for this number.
+  request.mutableResourceRequest().setHTTPHeaderField("range", "bytes=0-2047");
+
+  // TODO(sclittle): Indicate somehow (e.g. through a new request bit) to the
+  // embedder that it should return the full resource if the entire resource is
+  // fresh in the cache.
+}
+
 Resource* ResourceFetcher::requestResource(
     FetchRequest& request,
     const ResourceFactory& factory,
@@ -516,6 +540,9 @@ Resource* ResourceFetcher::requestResource(
       factory.type(), request, ResourcePriority::NotVisible));
   initializeResourceRequest(request.mutableResourceRequest(), factory.type(),
                             request.defer());
+
+  modifyRequestForPlaceholderImageRequest(request, factory.type());
+
   context().willStartLoadingResource(
       identifier, request.mutableResourceRequest(), factory.type());
   if (!request.url().isValid())
@@ -1162,6 +1189,11 @@ void ResourceFetcher::didFinishLoading(Resource* resource,
   if (finishReason == DidFinishLoading)
     resource->finish(finishTime);
   context().didLoadResource(resource);
+
+  if (resource->isImage() &&
+      toImageResource(resource)->shouldReloadBrokenPlaceholder()) {
+    toImageResource(resource)->reloadIfLoFiOrPlaceholder(this);

[Nate Chapin, 2016/10/18 18:23:17]

I may have asked this in the design doc, but if I did, I can't find the answer
:(

Is it important that this happens after didLoadResource()? Could we do these
calls in ImageResource::error and ImageResource::finish instead?

[sclittle, 2016/10/18 23:53:03]

I don't think the ordering here is important with respect to didLoadResource.

You did ask this in the design doc, but I resolved the comment:
https://docs.google.com/document/d/1691W7yFDI1FJv69N2MEtaSzpnqO2EqkgGD3T0O-pQ...

The issue with doing the reload in ImageResource::finish/error is that we need a
ResourceFetcher in order to start the reload. To get that ResourceFetcher, we'd
either need to pass it into finish()/error(), or ImageResource would need to
hold on to a ResourceFetcher.

Doing the reload here seems a bit nicer.

+  }
 }
 
 void ResourceFetcher::didFailLoading(Resource* resource,
@@ -1174,6 +1206,11 @@ void ResourceFetcher::didFailLoading(Resource* resource,
   context().dispatchDidFail(resource->identifier(), error, isInternalRequest);
   resource->error(error);
   context().didLoadResource(resource);
+
+  if (resource->isImage() &&
+      toImageResource(resource)->shouldReloadBrokenPlaceholder()) {
+    toImageResource(resource)->reloadIfLoFiOrPlaceholder(this);
+  }
 }
 
 void ResourceFetcher::didReceiveResponse(Resource* resource,
@@ -1421,7 +1458,7 @@ void ResourceFetcher::reloadLoFiImages() {
     Resource* resource = documentResource.value.get();
     if (resource && resource->isImage()) {
       ImageResource* imageResource = toImageResource(resource);
-      imageResource->reloadIfLoFi(this);
+      imageResource->reloadIfLoFiOrPlaceholder(this);
     }
   }
 }