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..128f89ab1f0a19895745ee3cb501425fd9b2ea61 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,25 @@ 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());

[sclittle, 2017/04/28 23:55:54]

These LogicalWidth/Height().IsAuto() checks seem odd - have you tested these
rules on a lot of sites to see how well these rules target all sprites and only
sprites?

[Raj, 2017/04/30 18:35:28]

I tested all examples from below link and some other demo sites.
https://www.w3schools.com/css/css_image_sprites.asp
I will test with few more.

This heuristic is similar to:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Sma...

The problem with similar check from below CL is that xPosition().isZero() will
fail for the sprite image with background_x=0
https://codereview.chromium.org/2710113002/diff/1/third_party/WebKit/Source/c...
For example, first image will not be detected as sprite.
https://www.w3schools.com/css/tryit.asp?filename=trycss_sprites_img

background.xPosition().isFixed() || background.yPosition().isFixed() check may
be sufficient enough too.

[sclittle, 2017/05/08 20:34:44]

Any updates from testing these rules out on a few more sprites? I'm just worried
that width and height could be set on a lot of objects that aren't sprites.

[rajendrant, 2017/06/12 20:31:04]

As you suggested, I changed the check to (background.xPosition().isFixed() ||
background.yPosition().isFixed()), since that is more meaningful for sprite
images.

+}
+
 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_, placeholder_image_request_type,
+                                   cross_origin);
+  }
 
   if (CSSPaintValue* paint_value = pending_image->CssPaintValue()) {
     StyleGeneratedImage* image = StyleGeneratedImage::Create(*paint_value);
@@ -133,9 +148,11 @@ 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_,
+                                       placeholder_image_request_type,
                                        cross_origin);
+  }
 
   NOTREACHED();
   return nullptr;
@@ -166,9 +183,13 @@ 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()),
+                ComputedStyleMayBeCSSSpriteBackgroundImage(*style)
+                    ? FetchParameters::kDisallowPlaceholder
+                    : FetchParameters::kAllowPlaceholder));
+          }
         }
         break;
       }
@@ -178,10 +199,12 @@ 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)));
+                      LoadPendingImage(style, ToStylePendingImage(image),
+                                       FetchParameters::kAllowPlaceholder));
+            }
           }
         }
         break;
@@ -191,9 +214,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),
+                                     FetchParameters::kAllowPlaceholder));
+              }
             }
           }
         }
@@ -201,16 +226,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()),
+              FetchParameters::kAllowPlaceholder));
+        }
         break;
       }
       case CSSPropertyBorderImageSource: {
         if (style->BorderImageSource() &&
-            style->BorderImageSource()->IsPendingImage())
+            style->BorderImageSource()->IsPendingImage()) {
           style->SetBorderImageSource(LoadPendingImage(
-              style, ToStylePendingImage(style->BorderImageSource())));
+              style, ToStylePendingImage(style->BorderImageSource()),
+              FetchParameters::kAllowPlaceholder));
+        }
         break;
       }
       case CSSPropertyWebkitBoxReflect: {
@@ -219,7 +248,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 +260,33 @@ 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()),
+              FetchParameters::kAllowPlaceholder));
+        }
         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()),
+                FetchParameters::kAllowPlaceholder));
+          }
         }
         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()),
+              FetchParameters::kAllowPlaceholder,
               kCrossOriginAttributeAnonymous));
+        }
         break;
       default:
         NOTREACHED();