Do not show image placeholders for CSS sprites

third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp

Index: third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp
diff --git a/third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp b/third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp
index 88e90ddbe49e012d943fa8dcc93992f35c1fbe27..6845e812959cf277ec2534d1617b9d56ee48a027 100644
--- a/third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp
+++ b/third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp
@@ -40,6 +40,8 @@
 #include "core/style/StyleInvalidImage.h"
 #include "core/style/StylePendingImage.h"
 #include "core/svg/SVGElementProxy.h"
+#include "platform/Length.h"
+#include "platform/loader/fetch/FetchParameters.h"
 #include "platform/loader/fetch/ResourceFetcher.h"
 
 namespace blink {
@@ -114,12 +116,32 @@ void ElementStyleResources::LoadPendingSVGDocuments(
   }
 }
 
+static bool ComputedStyleMayBeCSSSpriteBackgroundImage(
+    const ComputedStyle& style) {
+  // Simple heuristic to guess if CSS background image is used to create CSS
+  // sprites. For a legit background image it's very likely the height or the
+  // width will be set to auto. For CSS sprite image, height or width will
+  // probably be specified.
+  return style.HasBackgroundImage() &&
+         (!style.LogicalHeight().IsAuto() || !style.LogicalWidth().IsAuto());
+}
+
+static FetchParameters::PlaceholderImageRequestType
+GetPlaceholderImageRequestType(const ComputedStyle& style) {

[kouhei (in TOK), 2017/04/24 00:02:19]

I think we should simply inline the logic here. I found the name
"GetPlaceholderImageRequestType" too generic and hard to guess what the function
is doing without reading it.

[Raj, 2017/04/28 03:43:26]

Done.

+  return ComputedStyleMayBeCSSSpriteBackgroundImage(style)
+             ? FetchParameters::kDisallowPlaceholder
+             : FetchParameters::kAllowPlaceholder;
+}
+
 StyleImage* ElementStyleResources::LoadPendingImage(
     ComputedStyle* style,
     StylePendingImage* pending_image,
+    FetchParameters::PlaceholderImageRequestType placeholder_image_request_type,
     CrossOriginAttributeValue cross_origin) {
-  if (CSSImageValue* image_value = pending_image->CssImageValue())
-    return image_value->CacheImage(*document_, cross_origin);
+  if (CSSImageValue* image_value = pending_image->CssImageValue()) {
+    return image_value->CacheImage(*document_, cross_origin,
+                                   placeholder_image_request_type);
+  }
 
   if (CSSPaintValue* paint_value = pending_image->CssPaintValue()) {
     StyleGeneratedImage* image = StyleGeneratedImage::Create(*paint_value);
@@ -133,15 +155,20 @@ StyleImage* ElementStyleResources::LoadPendingImage(
     return StyleGeneratedImage::Create(*image_generator_value);
   }
 
-  if (CSSImageSetValue* image_set_value = pending_image->CssImageSetValue())
+  if (CSSImageSetValue* image_set_value = pending_image->CssImageSetValue()) {
     return image_set_value->CacheImage(*document_, device_scale_factor_,
-                                       cross_origin);
+                                       cross_origin,
+                                       placeholder_image_request_type);
+  }
 
   NOTREACHED();
   return nullptr;
 }
 
 void ElementStyleResources::LoadPendingImages(ComputedStyle* style) {
+  FetchParameters::PlaceholderImageRequestType placeholder_image_request_type =
+      GetPlaceholderImageRequestType(*style);

[kouhei (in TOK), 2017/04/24 00:02:19]

IIUC, this decides place_holder_image_request_type_ to be applied on all
image-holding CSS value on the "style", which will disable place_holder on
non-bg image if the style has css bg sprite.

I think what we want to achieve here is disable place_holder on bg image only.

[Raj, 2017/04/28 03:43:27]

Done.

+
   // We must loop over the properties and then look at the style to see if
   // a pending image exists, and only load that image. For example:
   //
@@ -166,9 +193,11 @@ void ElementStyleResources::LoadPendingImages(ComputedStyle* style) {
         for (FillLayer* background_layer = &style->AccessBackgroundLayers();
              background_layer; background_layer = background_layer->Next()) {
           if (background_layer->GetImage() &&
-              background_layer->GetImage()->IsPendingImage())
+              background_layer->GetImage()->IsPendingImage()) {
             background_layer->SetImage(LoadPendingImage(
-                style, ToStylePendingImage(background_layer->GetImage())));
+                style, ToStylePendingImage(background_layer->GetImage()),
+                placeholder_image_request_type));

[kouhei (in TOK), 2017/04/24 00:02:19]

Can we fill in place_holder_image_request_type here only

[Raj, 2017/04/28 03:43:26]

Done.

+          }
         }
         break;
       }
@@ -178,10 +207,11 @@ void ElementStyleResources::LoadPendingImages(ComputedStyle* style) {
              content_data; content_data = content_data->Next()) {
           if (content_data->IsImage()) {
             StyleImage* image = ToImageContentData(content_data)->GetImage();
-            if (image->IsPendingImage())
+            if (image->IsPendingImage()) {
               ToImageContentData(content_data)
-                  ->SetImage(
-                      LoadPendingImage(style, ToStylePendingImage(image)));
+                  ->SetImage(LoadPendingImage(style, ToStylePendingImage(image),
+                                              placeholder_image_request_type));

[kouhei (in TOK), 2017/04/24 00:02:19]

and use FetchParameters::kAllowPlaceholder here and below?

[Raj, 2017/04/28 03:43:27]

Done.

+            }
           }
         }
         break;
@@ -191,9 +221,11 @@ void ElementStyleResources::LoadPendingImages(ComputedStyle* style) {
           for (size_t i = 0; i < cursor_list->size(); ++i) {
             CursorData& current_cursor = cursor_list->at(i);
             if (StyleImage* image = current_cursor.GetImage()) {
-              if (image->IsPendingImage())
+              if (image->IsPendingImage()) {
                 current_cursor.SetImage(
-                    LoadPendingImage(style, ToStylePendingImage(image)));
+                    LoadPendingImage(style, ToStylePendingImage(image),
+                                     placeholder_image_request_type));
+              }
             }
           }
         }
@@ -201,16 +233,20 @@ void ElementStyleResources::LoadPendingImages(ComputedStyle* style) {
       }
       case CSSPropertyListStyleImage: {
         if (style->ListStyleImage() &&
-            style->ListStyleImage()->IsPendingImage())
+            style->ListStyleImage()->IsPendingImage()) {
           style->SetListStyleImage(LoadPendingImage(
-              style, ToStylePendingImage(style->ListStyleImage())));
+              style, ToStylePendingImage(style->ListStyleImage()),
+              placeholder_image_request_type));
+        }
         break;
       }
       case CSSPropertyBorderImageSource: {
         if (style->BorderImageSource() &&
-            style->BorderImageSource()->IsPendingImage())
+            style->BorderImageSource()->IsPendingImage()) {
           style->SetBorderImageSource(LoadPendingImage(
-              style, ToStylePendingImage(style->BorderImageSource())));
+              style, ToStylePendingImage(style->BorderImageSource()),
+              placeholder_image_request_type));
+        }
         break;
       }
       case CSSPropertyWebkitBoxReflect: {
@@ -219,7 +255,8 @@ void ElementStyleResources::LoadPendingImages(ComputedStyle* style) {
           if (mask_image.GetImage() &&
               mask_image.GetImage()->IsPendingImage()) {
             StyleImage* loaded_image = LoadPendingImage(
-                style, ToStylePendingImage(mask_image.GetImage()));
+                style, ToStylePendingImage(mask_image.GetImage()),
+                FetchParameters::kAllowPlaceholder);
             reflection->SetMask(NinePieceImage(
                 loaded_image, mask_image.ImageSlices(), mask_image.Fill(),
                 mask_image.BorderSlices(), mask_image.Outset(),
@@ -230,27 +267,32 @@ void ElementStyleResources::LoadPendingImages(ComputedStyle* style) {
       }
       case CSSPropertyWebkitMaskBoxImageSource: {
         if (style->MaskBoxImageSource() &&
-            style->MaskBoxImageSource()->IsPendingImage())
+            style->MaskBoxImageSource()->IsPendingImage()) {
           style->SetMaskBoxImageSource(LoadPendingImage(
-              style, ToStylePendingImage(style->MaskBoxImageSource())));
+              style, ToStylePendingImage(style->MaskBoxImageSource()),
+              placeholder_image_request_type));
+        }
         break;
       }
       case CSSPropertyWebkitMaskImage: {
         for (FillLayer* mask_layer = &style->AccessMaskLayers(); mask_layer;
              mask_layer = mask_layer->Next()) {
           if (mask_layer->GetImage() &&
-              mask_layer->GetImage()->IsPendingImage())
+              mask_layer->GetImage()->IsPendingImage()) {
             mask_layer->SetImage(LoadPendingImage(
-                style, ToStylePendingImage(mask_layer->GetImage())));
+                style, ToStylePendingImage(mask_layer->GetImage()),
+                placeholder_image_request_type));
+          }
         }
         break;
       }
       case CSSPropertyShapeOutside:
         if (style->ShapeOutside() && style->ShapeOutside()->GetImage() &&
-            style->ShapeOutside()->GetImage()->IsPendingImage())
+            style->ShapeOutside()->GetImage()->IsPendingImage()) {
           style->ShapeOutside()->SetImage(LoadPendingImage(
               style, ToStylePendingImage(style->ShapeOutside()->GetImage()),
-              kCrossOriginAttributeAnonymous));
+              placeholder_image_request_type, kCrossOriginAttributeAnonymous));
+        }
         break;
       default:
         NOTREACHED();