Do not show image placeholders for CSS sprites

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc.
  * All rights reserved.
  * Copyright (C) 2013 Google 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
  * version 2 of the License, or (at your option) any later version.
@@ skipping 10 matching lines @@
  *
  */
 
 #include "core/css/resolver/ElementStyleResources.h"
 
 #include "core/CSSPropertyNames.h"
 #include "core/css/CSSGradientValue.h"
 #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"
 #include "core/style/FillLayer.h"
 #include "core/style/FilterOperation.h"
 #include "core/style/StyleFetchedImage.h"
 #include "core/style/StyleFetchedImageSet.h"
 #include "core/style/StyleGeneratedImage.h"
 #include "core/style/StyleImage.h"
 #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 {
 
 ElementStyleResources::ElementStyleResources(Document& document,
                                              float deviceScaleFactor)
     : m_document(&document), m_deviceScaleFactor(deviceScaleFactor) {}
 
 StyleImage* ElementStyleResources::styleImage(CSSPropertyID property,
                                               const CSSValue& value) {
@@ skipping 54 matching lines @@
       computedStyle->mutableFilter().operations();
   for (auto& filterOperation : filterOperations) {
     if (filterOperation->type() != FilterOperation::REFERENCE)
       continue;
     ReferenceFilterOperation& referenceOperation =
         toReferenceFilterOperation(*filterOperation);
     referenceOperation.elementProxy().resolve(*m_document);
   }
 }
 
+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);
     style->addPaintImage(image);
     return image;
   }
 
   if (CSSImageGeneratorValue* imageGeneratorValue =
           pendingImage->cssImageGeneratorValue()) {
     imageGeneratorValue->loadSubimages(*m_document);
     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;
 }
 
 void ElementStyleResources::loadPendingImages(ComputedStyle* style) {
   // 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:
   //
   // <style>
@@ skipping 106 matching lines @@
     }
   }
 }
 
 void ElementStyleResources::loadPendingResources(ComputedStyle* computedStyle) {
   loadPendingImages(computedStyle);
   loadPendingSVGDocuments(computedStyle);
 }
 
 }  // namespace blink