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 2a19d222e787f6c3f67a161f9452d754c963fe4a..9d974da881afe37192b2d950e4aaf1d137d60235 100644
--- a/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
+++ b/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
@@ -47,11 +47,20 @@
 #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") ||
+         response.HttpHeaderField("chrome-proxy").Contains("q=low");

[megjablon, 2017/06/13 19:26:14]

Can you add a comment that we still check q=low because of the cache? I started
commenting asking why we still have this, so it'd be good to comment so that it
doesn't get accidentally removed.

[sclittle, 2017/06/13 20:53:23]

Done.

+}
+
 }  // namespace
 
 class ImageResource::ImageResourceInfoImpl final
@@ -449,9 +458,32 @@ void ImageResource::ResponseReceived(
       placeholder_option_ = PlaceholderOption::kReloadPlaceholderOnDecodeError;
     }
   }
+
+  // Ensure that the PreviewsState bit for Server Lo-Fi is set iff we received
+  // the appropriate Server Lo-Fi response headers for this image.
+  if (HasServerLoFiResponseHeaders(this->GetResponse())) {

[megjablon, 2017/06/13 19:26:14]

Shouldn't this already be set? We wouldn't have gotten a lo-fi response if we
didn't have the server lo-fi bit set.

[sclittle, 2017/06/13 20:53:23]

Normally the bit should be set if we're getting the headers, but the legacy
q=low LoFi images didn't use Vary in the way that the newer LoFi images do, so
the legacy images could still show up even if Chrome isn't in LoFi mode.

Also, if there's ever a server or client bug somewhere or something, it's nice
to have it such that the user is still able to reload the original images if
Server LoFi images are still served somehow.

[megjablon, 2017/06/13 21:27:05]

Ah ok. Can you add a comment of this effect?

[sclittle, 2017/06/13 21:42:31]

Done.

+    SetPreviewsState(GetResourceRequest().GetPreviewsState() |
+                     WebURLRequest::kServerLoFiOn);
+  } else if (GetResourceRequest().GetPreviewsState() &
+             WebURLRequest::kServerLoFiOn) {
+    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 {

[megjablon, 2017/06/13 19:26:14]

Is this method only called if you're in the client lo-fi experiment? 

[sclittle, 2017/06/13 20:53:23]

It's called by ImageResourceContent::UpdateImage when it decides if it should
show a placeholder image or not, for all images regardless of what experiments
are active.

So this causes client placeholders to be shown for Server LoFi images even if
the user isn't in the Client LoFi experiment.

[megjablon, 2017/06/13 21:27:05]

Is this approved to roll out for server lo-fi without any sort of flag? Does the
server team know that we essentially won't be using the server placeholders M61+
for all lo-fi users?

[sclittle, 2017/06/13 21:42:31]

Good point - I'll confirm and clarify with server folks before landing this.

+  // 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.
+  if (GetResourceRequest().GetPreviewsState() & WebURLRequest::kServerLoFiOn)
+    return true;
+
   switch (placeholder_option_) {
     case PlaceholderOption::kShowAndReloadPlaceholderAlways:
       return true;
@@ -476,27 +508,19 @@ 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;
 
+  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