Show client placeholders for Server LoFi images.

third_party/WebKit/Source/core/loader/resource/ImageResource.cpp

Index: third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
diff --git a/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp b/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
index 0b72716693f8e4c0ba85a02f40a81e30b9fa009a..afda85e713328e82dc9fad56a8c887d9368040a6 100644
--- a/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
+++ b/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
@@ -48,11 +48,22 @@
 #include "v8/include/v8.h"
 
 namespace blink {
+
 namespace {
+
 // The amount of time to wait before informing the clients that the image has
 // been updated (in seconds). This effectively throttles invalidations that
 // result from new data arriving for this image.
 constexpr double kFlushDelaySeconds = 1.;
+
+bool HasServerLoFiResponseHeaders(const ResourceResponse& response) {
+  return response.HttpHeaderField("chrome-proxy-content-transform")
+             .Contains("empty-image") ||
+         // Check for the legacy Server Lo-Fi response headers, since it's
+         // possible that an old Lo-Fi image could be served from the cache.
+         response.HttpHeaderField("chrome-proxy").Contains("q=low");
+}
+
 }  // namespace
 
 class ImageResource::ImageResourceInfoImpl final
@@ -456,9 +467,44 @@ void ImageResource::ResponseReceived(
       placeholder_option_ = PlaceholderOption::kReloadPlaceholderOnDecodeError;
     }
   }
+
+  if (HasServerLoFiResponseHeaders(this->GetResponse())) {

[kouhei (in TOK), 2017/07/07 01:38:03]

Can we omit "this->"?

[sclittle, 2017/07/07 02:46:41]

Done.

+    // Ensure that the PreviewsState bit for Server Lo-Fi is set iff Chrome
+    // received the appropriate Server Lo-Fi response headers for this image.
+    //
+    // Normally, the |kServerLoFiOn| bit should already be set if Server Lo-Fi
+    // response headers are coming back, but it's possible for legacy Lo-Fi
+    // images to be served from the cache even if Chrome isn't in Lo-Fi mode.
+    // This also serves as a nice last line of defence to ensure that Server
+    // Lo-Fi images can be reloaded to show the original even if e.g. a server
+    // bug causes Lo-Fi images to be sent when they aren't expected.
+    SetPreviewsState(GetResourceRequest().GetPreviewsState() |

[hiroshige, 2017/07/07 02:52:49]

Just for clarification:
SetPreviewsState() calls here and at Line 494 modifies ResourceRequest, but this
looks like a part of response.
Do we need to modify ResourceRequest? Does the following alternative work?
1. Store a separate PreviewState as (Image)Resource::response_preview_state_
that is initialized to ResourceRequest's preview state.
2. Update |response_preview_state_| here, leave ResourceRequest's preview state
unmodified.
3. Make the lofi-related code in ImageResource.cpp use response_preview_state_
instead of ResourceRequest's PreviewState.

This doesn't block this CL, no need to do the changes; I want to track the
intention of modifications to ResourceRequest.

[sclittle, 2017/07/07 21:25:28]

I think that alternative would work, and it might be useful to keep the original
PreviewsState from the request intact if we wanted to e.g. record metrics about
it in the future. I think it's a bit simpler to update the bits in the
ResourceRequest for now (at least for this CL), since e.g. the reloading code
already keeps those bits up to date.

I also like the idea of splitting the LoFi/Previews logic out into a separate
helper or state class like kinuko@ and kouhei@ mentioned, but that seems better
suited to a separate CL.

+                     WebURLRequest::kServerLoFiOn);
+  } else if (GetResourceRequest().GetPreviewsState() &
+             WebURLRequest::kServerLoFiOn) {
+    // If Chrome expects a Lo-Fi response, but the server decided to send the
+    // full image, then clear the Server Lo-Fi Previews state bit.
+    WebURLRequest::PreviewsState new_previews_state =
+        GetResourceRequest().GetPreviewsState();
+
+    new_previews_state &= ~WebURLRequest::kServerLoFiOn;
+    if (new_previews_state == WebURLRequest::kPreviewsUnspecified)
+      new_previews_state = WebURLRequest::kPreviewsOff;
+
+    SetPreviewsState(new_previews_state);
+  }
 }
 
 bool ImageResource::ShouldShowPlaceholder() const {
+  if (RuntimeEnabledFeatures::ClientPlaceholdersForServerLoFiEnabled() &&
+      (GetResourceRequest().GetPreviewsState() &
+       WebURLRequest::kServerLoFiOn)) {
+    // If the runtime feature is enabled, show Client Lo-Fi placeholder images
+    // in place of Server Lo-Fi responses. This is done so that all Lo-Fi images
+    // have a consistent appearance.
+    return true;
+  }
+
   switch (placeholder_option_) {
     case PlaceholderOption::kShowAndReloadPlaceholderAlways:
     case PlaceholderOption::kShowAndDoNotReloadPlaceholder:
@@ -485,27 +531,21 @@ bool ImageResource::ShouldReloadBrokenPlaceholder() const {
   return false;
 }
 
-static bool IsLoFiImage(const ImageResource& resource) {
-  if (resource.IsLoaded()) {
-    return resource.GetResponse()
-               .HttpHeaderField("chrome-proxy-content-transform")
-               .Contains("empty-image") ||
-           resource.GetResponse()
-               .HttpHeaderField("chrome-proxy")
-               .Contains("q=low");
-  }
-  return resource.GetResourceRequest().GetPreviewsState() &
-         WebURLRequest::kServerLoFiOn;
-}
-
 void ImageResource::ReloadIfLoFiOrPlaceholderImage(
     ResourceFetcher* fetcher,
     ReloadLoFiOrPlaceholderPolicy policy) {
   if (policy == kReloadIfNeeded && !ShouldReloadBrokenPlaceholder())
     return;
 
+  // If the image is loaded, then the |PreviewsState::kServerLoFiOn| bit should
+  // be set iff the image has Server Lo-Fi response headers.
+  DCHECK(!IsLoaded() ||
+         HasServerLoFiResponseHeaders(GetResponse()) ==
+             static_cast<bool>(GetResourceRequest().GetPreviewsState() &
+                               WebURLRequest::kServerLoFiOn));
+
   if (placeholder_option_ == PlaceholderOption::kDoNotReloadPlaceholder &&
-      !IsLoFiImage(*this))
+      !(GetResourceRequest().GetPreviewsState() & WebURLRequest::kServerLoFiOn))
     return;
 
   // Prevent clients and observers from being notified of completion while the