Collapse images disallowed by the Safe Browsing Subresource Filter.

third_party/WebKit/Source/core/html/HTMLImageElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLImageElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLImageElement.cpp b/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
index 01fc2ac62b9849ede2e4d98b0d73447fdedc8672..4e48e68a4b1b42e72cf7d97185fb11fe1a23a2fa 100644
--- a/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
@@ -93,9 +93,9 @@ HTMLImageElement::HTMLImageElement(Document& document,
       m_imageLoader(HTMLImageLoader::create(this)),
       m_imageDevicePixelRatio(1.0f),
       m_source(nullptr),
+      m_layoutDisposition(LayoutDisposition::PrimaryContent),
       m_formWasSetByParser(false),
       m_elementCreatedByParser(createdByParser),
-      m_useFallbackContent(false),
       m_isFallbackImage(false),
       m_referrerPolicy(ReferrerPolicyDefault) {
   setHasCustomStyleCallbacks();
@@ -339,6 +339,11 @@ ImageCandidate HTMLImageElement::findBestFitImageFromPictureParent() {
   return ImageCandidate();
 }
 
+bool HTMLImageElement::layoutObjectIsNeeded(const ComputedStyle& style) {
+  return m_layoutDisposition != LayoutDisposition::Collapsed &&
+         HTMLElement::layoutObjectIsNeeded(style);
+}
+
 LayoutObject* HTMLImageElement::createLayoutObject(const ComputedStyle& style) {
   const ContentData* contentData = style.contentData();
   if (contentData && contentData->isImage()) {
@@ -349,9 +354,10 @@ LayoutObject* HTMLImageElement::createLayoutObject(const ComputedStyle& style) {
       return LayoutObject::createObject(this, style);
   }
 
-  if (m_useFallbackContent)

[dominicc (has gone to gerrit), 2016/12/05 03:30:55]

What do you think about using a switch here? Then it's crystal clear you cover
three cases:

Fallback -> block flow
Primary -> image
Collapsed -> dcheck

[engedy, 2016/12/06 15:27:57]

Done.

+  if (m_layoutDisposition == LayoutDisposition::FallbackContent)
     return new LayoutBlockFlow(this);
 
+  DCHECK_EQ(m_layoutDisposition, LayoutDisposition::PrimaryContent);
   LayoutImage* image = new LayoutImage(this);
   image->setImageResource(LayoutImageResource::create());
   image->setImageDevicePixelRatio(m_imageDevicePixelRatio);
@@ -801,7 +807,7 @@ void HTMLImageElement::selectSourceURL(
   if ((imageHasLoaded && imageHasImage) || imageStillLoading || imageIsDocument)
     ensurePrimaryContent();
   else
-    ensureFallbackContent();
+    ensureCollapsedOrFallbackContent();
 }
 
 const KURL& HTMLImageElement::sourceURL() const {
@@ -814,24 +820,32 @@ void HTMLImageElement::didAddUserAgentShadowRoot(ShadowRoot&) {
 
 void HTMLImageElement::ensureFallbackForGeneratedContent() {
   setUseFallbackContent();
-  reattachFallbackContent();
+  reattachContent();
 }
 
-void HTMLImageElement::ensureFallbackContent() {
-  if (m_useFallbackContent || m_isFallbackImage)
+void HTMLImageElement::ensureCollapsedOrFallbackContent() {
+  if (m_isFallbackImage)
     return;
-  setUseFallbackContent();
-  reattachFallbackContent();
+  if (imageLoader().image() &&
+      imageLoader().image()->resourceError().shouldCollapseInitiator()) {
+    if (m_layoutDisposition != LayoutDisposition::Collapsed) {
+      m_layoutDisposition = LayoutDisposition::Collapsed;

[dominicc (has gone to gerrit), 2016/12/05 03:30:55]

This code smells a tiny bit, it looks like when you set m_layoutDisposition you
should call reattachContent right after/soon after. Can we make that a setter
which ties those operations together into unit?

If that's not *always* the case, can we add some unit tests and integration
tests exercising when it's different?

[engedy, 2016/12/06 15:27:57]

It's always the case, done some refactoring. WDYT?

+      reattachContent();
+    }
+  } else if (m_layoutDisposition != LayoutDisposition::FallbackContent) {
+    setUseFallbackContent();

[dominicc (has gone to gerrit), 2016/12/05 03:30:55]

Isn't this just ensureFallbackForGeneratedContent?

[engedy, 2016/12/06 15:27:57]

Refactored this a bit too.  For now I don't want to introduce the collapsed
state to the code path from ensureFallbackForGeneratedContent(), I'll look into
that later.

+    reattachContent();
+  }
 }
 
 void HTMLImageElement::ensurePrimaryContent() {
-  if (!m_useFallbackContent)
+  if (m_layoutDisposition == LayoutDisposition::PrimaryContent)
     return;
-  m_useFallbackContent = false;
-  reattachFallbackContent();
+  m_layoutDisposition = LayoutDisposition::PrimaryContent;
+  reattachContent();
 }
 
-void HTMLImageElement::reattachFallbackContent() {
+void HTMLImageElement::reattachContent() {
   // This can happen inside of attachLayoutTree() in the middle of a recalcStyle
   // so we need to reattach synchronously here.
   if (document().inStyleRecalc())
@@ -841,17 +855,16 @@ void HTMLImageElement::reattachFallbackContent() {
 }
 
 PassRefPtr<ComputedStyle> HTMLImageElement::customStyleForLayoutObject() {
-  RefPtr<ComputedStyle> newStyle = originalStyleForLayoutObject();
-
-  if (!m_useFallbackContent)
-    return newStyle;
-
-  RefPtr<ComputedStyle> style = ComputedStyle::clone(*newStyle);
-  return HTMLImageFallbackHelper::customStyleForAltText(*this, style);
+  if (m_layoutDisposition == LayoutDisposition::PrimaryContent ||

[dominicc (has gone to gerrit), 2016/12/05 03:30:55]

Again, I wonder if a switch with fallthrough would be easier to read? It teaches
the reader what the three exhaustive cases are even if they don't have the
definition of LayoutDisposition at the front of their mind.

[engedy, 2016/12/06 15:27:57]

Done.

+      m_layoutDisposition == LayoutDisposition::Collapsed) {
+    return originalStyleForLayoutObject();
+  }
+  return HTMLImageFallbackHelper::customStyleForAltText(
+      *this, ComputedStyle::clone(*originalStyleForLayoutObject()));
 }
 
 void HTMLImageElement::setUseFallbackContent() {
-  m_useFallbackContent = true;
+  m_layoutDisposition = LayoutDisposition::FallbackContent;
   if (document().inStyleRecalc())
     return;
   EventDispatchForbiddenScope::AllowUserAgentEvents allowEvents;