Show client placeholders for Server LoFi images.

third_party/WebKit/Source/core/loader/resource/ImageResource.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) 2006 Samuel Weinig (sam.weinig@gmail.com)
     Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
     License as published by the Free Software Foundation; either
@@ skipping 30 matching lines @@
 #include "platform/loader/fetch/ResourceLoadingLog.h"
 #include "platform/network/HTTPParsers.h"
 #include "platform/weborigin/KURL.h"
 #include "platform/weborigin/SecurityViolationReportingPolicy.h"
 #include "platform/wtf/CurrentTime.h"
 #include "platform/wtf/StdLibExtras.h"
 #include "public/platform/Platform.h"
 #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
     : public GarbageCollectedFinalized<ImageResourceInfoImpl>,
       public ImageResourceInfo {
   USING_GARBAGE_COLLECTED_MIXIN(ImageResourceInfoImpl);
 
  public:
   ImageResourceInfoImpl(ImageResource* resource) : resource_(resource) {
     DCHECK(resource_);
@@ skipping 370 matching lines @@
             .ToFloat(&has_device_pixel_ratio_header_value_);
     if (!has_device_pixel_ratio_header_value_ ||
         device_pixel_ratio_header_value_ <= 0.0) {
       device_pixel_ratio_header_value_ = 1.0;
       has_device_pixel_ratio_header_value_ = false;
     }
   }
 
   if (placeholder_option_ ==
           PlaceholderOption::kShowAndReloadPlaceholderAlways &&
       IsEntireResource(this->GetResponse())) {

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

Sorry I forgot why we are using this->GetResponse() instead of function param
"response".
Can we simply use response here? or can we add a comment why?
(feel free to split CL for this)
My guess is to avoid reacting to revalidation req responses?

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

Looking at the code, it looks like you're correct in that it's for handling
revalidation responses. I've added a comment.

     if (this->GetResponse().HttpStatusCode() < 400 ||
         this->GetResponse().HttpStatusCode() >= 600) {
       // Don't treat a complete and broken image as a placeholder if the
       // response code is something other than a 4xx or 5xx error.
       // This is done to prevent reissuing the request in cases like
       // "204 No Content" responses to tracking requests triggered by <img>
       // tags, and <img> tags used to preload non-image resources.
       placeholder_option_ = PlaceholderOption::kDoNotReloadPlaceholder;
     } else {
       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:
       return true;
     case PlaceholderOption::kReloadPlaceholderOnDecodeError:
     case PlaceholderOption::kDoNotReloadPlaceholder:
       return false;
   }
   NOTREACHED();
   return false;
 }
 
 bool ImageResource::ShouldReloadBrokenPlaceholder() const {
   switch (placeholder_option_) {
     case PlaceholderOption::kShowAndReloadPlaceholderAlways:
       return ErrorOccurred();
     case PlaceholderOption::kReloadPlaceholderOnDecodeError:
       return GetStatus() == ResourceStatus::kDecodeError;
     case PlaceholderOption::kShowAndDoNotReloadPlaceholder:
     case PlaceholderOption::kDoNotReloadPlaceholder:
       return false;
   }
   NOTREACHED();
   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
   // reload is being scheduled, so that e.g. canceling an existing load in
   // progress doesn't cause clients and observers to be notified of completion
   // prematurely.
   DCHECK(!is_scheduling_reload_);
   is_scheduling_reload_ = true;
 
   SetCachePolicyBypassingCache();
@@ skipping 126 matching lines @@
     //    reloading in Step 3 due to notifyObservers()'s
     //    schedulingReloadOrShouldReloadBrokenPlaceholder() check.
     // 3. reloadIfLoFiOrPlaceholderImage() is called via ResourceFetcher
     //    (a) via didFinishLoading() called in decodeError(), or
     //    (b) after returning ImageResource::updateImage().
     DecodeError(all_data_received);
   }
 }
 
 }  // namespace blink