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 29 matching lines @@
 #include "platform/loader/fetch/ResourceLoaderOptions.h"
 #include "platform/loader/fetch/ResourceLoadingLog.h"
 #include "platform/network/HTTPParsers.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") ||
+         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
     : public GarbageCollectedFinalized<ImageResourceInfoImpl>,
       public ImageResourceInfo {
   USING_GARBAGE_COLLECTED_MIXIN(ImageResourceInfoImpl);
 
  public:
   ImageResourceInfoImpl(ImageResource* resource) : resource_(resource) {
     DCHECK(resource_);
@@ skipping 377 matching lines @@
       // 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;
     }
   }
+
+  // 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;
     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::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;
 
+  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 116 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