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 f435cbe4692f0309714aa60b441dafeaafdf6369..ae9d8eb39b4108d7b2491cd85739f1b92c861595 100644
--- a/third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp
+++ b/third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp
@@ -28,6 +28,8 @@
 #include "core/css/CSSImageValue.h"
 #include "core/css/CSSURIValue.h"
 #include "core/dom/Document.h"
+#include "core/frame/Settings.h"
+#include "core/loader/resource/ImageResource.h"
 #include "core/style/ComputedStyle.h"
 #include "core/style/ContentData.h"
 #include "core/style/CursorData.h"
@@ -40,6 +42,8 @@
 #include "core/style/StyleInvalidImage.h"
 #include "core/style/StylePendingImage.h"
 #include "core/svg/SVGElementProxy.h"
+#include "platform/Length.h"
+#include "platform/LengthPoint.h"
 #include "platform/loader/fetch/ResourceFetcher.h"
 
 namespace blink {
@@ -114,12 +118,30 @@ void ElementStyleResources::loadPendingSVGDocuments(
   }
 }
 
+static bool isSprite(const ComputedStyle& style) {

[kouhei (in TOK), 2017/04/11 07:45:59]

I'd prefer to have a more descriptive name.
ComputedStyleMayBeCSSSpriteBackgroundImage?

[Raj, 2017/04/20 19:01:17]

Done.

+  // 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());

[kouhei (in TOK), 2017/04/11 07:45:59]

Would you provide more info about this heuristic?
Should we also check background-clip, background-position?
Also, checking on style here would mean we will disallow imagePlaceHolders on
non-bg css images (see L189) like WebkitBoxReflect if CSS sprite bg image is
maybe set. Is this intended?

[Raj, 2017/04/20 19:01:17]

The heuristic is to catch only the following case:
 background image is set, height or width is specified.
It captures all the examples from
https://www.w3schools.com/css/css_image_sprites.asp

I have added check to enable imagePlaceholder for WebkitBoxReflect case

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

Would you add test case for this?

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

Added test.

+}
+
+static bool isImagePlaceholderAllowed(const Document& document,
+                                      const ComputedStyle& style) {
+  return document.settings() &&
+         document.settings()->getFetchImagePlaceholders() && !isSprite(style);

[kouhei (in TOK), 2017/04/11 07:45:59]

It looks like we are introducing another check so that we check
document.settings() twice. Would you explain why?

[Raj, 2017/04/20 19:01:17]

Check not needed here. Removed.

+}
+
 StyleImage* ElementStyleResources::loadPendingImage(
     ComputedStyle* style,
     StylePendingImage* pendingImage,
     CrossOriginAttributeValue crossOrigin) {
-  if (CSSImageValue* imageValue = pendingImage->cssImageValue())
-    return imageValue->cacheImage(*m_document, crossOrigin);
+  if (CSSImageValue* imageValue = pendingImage->cssImageValue()) {
+    return imageValue->cacheImage(
+        *m_document, crossOrigin,
+        isImagePlaceholderAllowed(*m_document, *style));
+  }
 
   if (CSSPaintValue* paintValue = pendingImage->cssPaintValue()) {
     StyleGeneratedImage* image = StyleGeneratedImage::create(*paintValue);
@@ -133,9 +155,11 @@ StyleImage* ElementStyleResources::loadPendingImage(
     return StyleGeneratedImage::create(*imageGeneratorValue);
   }
 
-  if (CSSImageSetValue* imageSetValue = pendingImage->cssImageSetValue())
-    return imageSetValue->cacheImage(*m_document, m_deviceScaleFactor,
-                                     crossOrigin);
+  if (CSSImageSetValue* imageSetValue = pendingImage->cssImageSetValue()) {
+    return imageSetValue->cacheImage(
+        *m_document, m_deviceScaleFactor, crossOrigin,
+        isImagePlaceholderAllowed(*m_document, *style));
+  }
 
   NOTREACHED();
   return nullptr;