Discard hidden articles when using fast path

javatests/org/chromium/distiller/DomUtilTest.java

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.distiller;
 
 import org.chromium.distiller.webdocument.WebTable;
 
 import com.google.gwt.core.client.JsArray;
 import com.google.gwt.dom.client.Document;
 import com.google.gwt.dom.client.Element;
 import com.google.gwt.dom.client.Node;
+import com.google.gwt.dom.client.NodeList;
 
 import java.util.Map;
 import java.util.List;
 
 public class DomUtilTest extends DomDistillerJsTestCase {
+    private static final String CONTENT_TEXT = "Lorem Ipsum Lorem Ipsum Lorem Ipsum.";
+
     public void testGetAttributes() {
         Element e = Document.get().createDivElement();
         e.setInnerHTML("<div style=\"width:50px; height:100px\" id=\"f\" class=\"sdf\"></div>");
         e = Element.as(e.getChildNodes().getItem(0));
         JsArray<Node> jsAttrs = DomUtil.getAttributes(e);
         assertEquals(3, jsAttrs.length());
         assertEquals("style", jsAttrs.get(0).getNodeName());
         assertEquals("width:50px; height:100px", jsAttrs.get(0).getNodeValue());
         assertEquals("id", jsAttrs.get(1).getNodeName());
         assertEquals("f", jsAttrs.get(1).getNodeValue());
@@ skipping 74 matching lines @@
         div.appendChild(div2);
 
         Element currDiv = TestUtil.createDiv(3);
         div2.appendChild(currDiv);
         Element finalDiv1 = currDiv;
 
         currDiv = TestUtil.createDiv(4);
         div2.appendChild(currDiv);
         currDiv.appendChild(TestUtil.createDiv(5));
 
-        assertEquals(div2, DomUtil.getNearestCommonAncestor(finalDiv1, currDiv.getChild(0)));
+        assertEquals(div2, DomUtil.getNearestCommonAncestor(finalDiv1,
+                currDiv.getChild(0)));
+
+        NodeList<Element> nodeList = DomUtil.querySelectorAll(mRoot,
+                "[id=\"3\"],[id=\"5\"]");
+
         assertEquals(div2, DomUtil.getNearestCommonAncestor(
-                DomUtil.querySelectorAll(mRoot, "[id=\"3\"],[id=\"5\"]")));
+                TestUtil.nodeListToList(nodeList)));
+
     }
 
     /**
      * The tree graph is:
      * 1 - 2 - 3
      */
     public void testNearestCommonAncestorIsRoot() {
         Element div = TestUtil.createDiv(1);
         mBody.appendChild(div);
 
         Element div2 = TestUtil.createDiv(2);
         div.appendChild(div2);
 
         Element div3 = TestUtil.createDiv(3);
         div2.appendChild(div3);
 
         assertEquals(div, DomUtil.getNearestCommonAncestor(div, div3));
+
+        NodeList<Element> nodeList = DomUtil.querySelectorAll(mRoot,
+                "[id=\"1\"],[id=\"3\"]");
+
         assertEquals(div, DomUtil.getNearestCommonAncestor(
-                DomUtil.querySelectorAll(mRoot, "[id=\"1\"],[id=\"3\"]")));
+                TestUtil.nodeListToList(nodeList)));
     }
 
     public void testNodeDepth() {
         Element div = TestUtil.createDiv(1);
 
         Element div2 = TestUtil.createDiv(2);
         div.appendChild(div2);
 
         Element div3 = TestUtil.createDiv(3);
         div2.appendChild(div3);
@@ skipping 227 matching lines @@
         for (int i = 0; i < mBody.getChildCount(); i++) {
             DomUtil.stripImageElements(mBody.getChild(i));
         }
         assertEquals(expected, mBody.getInnerHTML());
 
         mBody.setInnerHTML(html);
         DomUtil.stripImageElements(mBody);
         assertEquals(expected, mBody.getInnerHTML());
     }
 
+    public void testIsVisibleByItsOffsetParentDisplayNone() {
+        String html =
+            "<div style=\"display: none;\">" +
+                "<div>Some Text</div>" +
+            "</div>";
+        mBody.setInnerHTML(html);
+        Element child = mBody.getFirstChildElement().getFirstChildElement();
+        assertFalse(DomUtil.isVisibleByItsOffset(child));
+    }
+
+    public void testIsVisibleByItsOffsetChildDisplayNone() {
+        String html =
+            "<div>" +
+                "<div style=\"display: none;\">Some Text</div>" +
+            "</div>";
+        mBody.setInnerHTML(html);
+        Element child = mBody.getFirstChildElement().getFirstChildElement();
+        assertFalse(DomUtil.isVisibleByItsOffset(child));
+    }
+
+    public void testIsVisibleByItsOffsetDisplayBlock() {
+        String html =
+            "<div>" +
+                "<div>Some Text</div>" +
+            "</div>";
+        mBody.setInnerHTML(html);
+        Element child = mBody.getFirstChildElement().getFirstChildElement();
+        assertTrue(DomUtil.isVisibleByItsOffset(child));
+    }
+
+    public void testOnlyProcessArticleElement() {
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +

[wychen, 2016/06/02 05:56:01]

All these tests should work without putting CONTENT_TEXT in the tags.

[marcelorcorrea, 2016/06/03 16:21:57]

We tried using only empty elements but didn't work because DomUtil.isVisible()
method doesn't consider these elements as visible.
That's why we kept CONTENT_TEXT inside of the elements.

[wychen, 2016/06/03 16:49:53]

Ah. So it's about test again.

Let's use the content of isVisible() below and put into our isVisibleByOffset().
https://codereview.chromium.org/1508963003/
You can read the comment there for more details.

[marcelorcorrea, 2016/06/06 13:17:57]

Done.

+            "<article>" + CONTENT_TEXT + "</article>";
+
+        String expected = "<article>" + CONTENT_TEXT + "</article>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessArticleElementWithHiddenArticleElement() {
+        final String paragraph = "<p>" + CONTENT_TEXT + "</p>" +
+                "<p>" + CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<article>" + paragraph + "</article>" +
+            "<article style=\"display:none\">" + paragraph +"</article>";
+
+        String expected = "<article>" + paragraph + "</article>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessArticleElementMultiple() {
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<article>" + CONTENT_TEXT + "</article>" +
+            "<article>" + CONTENT_TEXT + "</article>";
+
+        // The existence of multiple articles disables the fast path.
+        assertNull(getArticleElement(htmlArticle));
+    }
+
+    public void testOnlyProcessArticleElementMultipleWithHiddenArticleElement() {

[wychen, 2016/06/02 05:56:01]

This seems a bit redundant.

[marcelorcorrea, 2016/06/03 16:21:57]

Done.

+        final String paragraph = "<p>" + CONTENT_TEXT + "</p><p>" +
+                CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<article>" + paragraph + "</article>" +
+            "<article style=\"display:none\">" + paragraph + "</article>" +
+            "<article>" + paragraph + "</article>";
+
+        // The existence of multiple articles disables the fast path.
+        assertNull(getArticleElement(htmlArticle));
+    }
+
+    public void testOnlyProcessOGArticle() {

[wychen, 2016/06/02 05:56:01]

My bad. I mixed up open graph with schema.org back then.
testOnlyProcessSchemaOrgArticle()

Apply to all the schema.org tests below.

[marcelorcorrea, 2016/06/03 16:21:57]

Done.

+        final String paragraph = "<p>" + CONTENT_TEXT + "</p><p>" +
+                CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<div itemscope itemtype=\"http://schema.org/Article\">" +
+                paragraph +
+            "</div>";
+
+        final String expected =
+            "<div itemscope=\"\" " +
+                "itemtype=\"http://schema.org/Article\">" + paragraph +
+            "</div>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessOGArticleWithHiddenArticleElement() {
+        final String paragraph = "<p>" + CONTENT_TEXT + "</p>" +
+                "<p>" + CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<div itemscope itemtype=\"http://schema.org/Article\">" +
+                paragraph + "</div>" +
+            "<div itemscope itemtype=\"http://schema.org/Article\" " +
+                "style=\"display:none\">" + paragraph +
+            "</div>";
+
+        String expected =
+            "<div itemscope=\"\" itemtype=\"http://schema.org/Article\">" +
+                paragraph +
+            "</div>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessOGArticleNews() {
+        final String paragraph = "<p>" + CONTENT_TEXT + "</p>" +
+                "<p>" + CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<div itemscope itemtype=\"http://schema.org/NewsArticle\">" +
+                paragraph +
+            "</div>";
+
+        final String expected =
+            "<div itemscope=\"\" " +
+                "itemtype=\"http://schema.org/NewsArticle\">" + paragraph +
+            "</div>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessOGArticleNewsWithHiddenArticleElement() {

[wychen, 2016/06/02 05:56:01]

I think we only need to test one variation of schema.org articles with hidden
articles.

[marcelorcorrea, 2016/06/03 16:21:57]

Done.

+        final String paragraph = "<p>" + CONTENT_TEXT + "</p>" +
+                "<p>" + CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<div itemscope itemtype=\"http://schema.org/NewsArticle\">" +
+                paragraph +
+            "</div>" +
+            "<div itemscope itemtype=\"http://schema.org/NewsArticle\" " +
+                "style=\"display:none\">" + paragraph +
+            "</div>";
+
+        String expected =
+            "<div itemscope=\"\" " +
+                "itemtype=\"http://schema.org/NewsArticle\">" + paragraph +
+            "</div>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessOGArticleBlog() {
+        final String paragraph = "<p>" + CONTENT_TEXT + "</p>" +
+                "<p>" + CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<div itemscope itemtype=\"http://schema.org/BlogPosting\">" +
+                paragraph +
+            "</div>";
+
+        final String expected =
+            "<div itemscope=\"\" " +
+                "itemtype=\"http://schema.org/BlogPosting\">" +
+                paragraph +
+            "</div>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessOGArticleBlogWithHiddenArticleElement() {

[wychen, 2016/06/02 05:56:01]

ditto

[marcelorcorrea, 2016/06/03 16:21:57]

Done.

+        final String paragraph = "<p>" + CONTENT_TEXT + "</p>" +
+                "<p>" + CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<div itemscope itemtype=\"http://schema.org/BlogPosting\">" +
+                paragraph +
+            "</div>" +
+            "<div itemscope itemtype=\"http://schema.org/BlogPosting\" " +
+                "style=\"display:none\">" + paragraph +
+            "</div>";
+
+        final String expected =
+            "<div itemscope=\"\" " +
+                "itemtype=\"http://schema.org/BlogPosting\">" +
+                paragraph +
+            "</div>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessOGArticleNested() {
+        final String paragraph = "<p>" + CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<div itemscope itemtype=\"http://schema.org/Article\">" +
+                paragraph +
+                "<div itemscope itemtype=\"http://schema.org/Article\">" +
+                    paragraph +
+                "</div>" +
+            "</div>";
+
+        final String expected =
+            "<div itemscope=\"\" itemtype=\"http://schema.org/Article\">" +
+                paragraph +
+                "<div itemscope=\"\" itemtype=\"http://schema.org/Article\">" +
+                    paragraph +
+               "</div>" +
+            "</div>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessOGArticleNestedWithNestedHiddenArticleElement() {

[wychen, 2016/06/02 05:56:01]

Keep this, since this seems complicated enough.

[marcelorcorrea, 2016/06/03 16:21:57]

Done.

+        final String paragraph = "<p>" + CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<div itemscope itemtype=\"http://schema.org/Article\">" +
+                    paragraph +
+                "<div itemscope itemtype=\"http://schema.org/Article\">" +
+                    paragraph +
+                "</div>" +
+                "<div itemscope itemtype=\"http://schema.org/Article\" " +
+                    "style=\"display:none\">" + paragraph +
+                "</div>" +
+            "</div>";
+
+        final String expected =
+            "<div itemscope=\"\" itemtype=\"http://schema.org/Article\">" +
+                paragraph +
+            "<div itemscope=\"\" itemtype=\"http://schema.org/Article\">" +
+                paragraph + "</div>" +
+            "<div itemscope=\"\" itemtype=\"http://schema.org/Article\" " +
+                "style=\"display:none\">" + paragraph + "</div>" +
+            "</div>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessOGArticleNestedWithHiddenArticleElement() {

[wychen, 2016/06/02 05:56:01]

Also keep this.

[marcelorcorrea, 2016/06/03 16:21:57]

Done.

+        final String paragraph = "<p>" + CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<div itemscope itemtype=\"http://schema.org/Article\">" +
+                    paragraph +
+                "<div itemscope itemtype=\"http://schema.org/Article\">" +
+                    paragraph +
+                "</div>" +
+            "</div>" +
+            "<div itemscope itemtype=\"http://schema.org/Article\" " +
+                "style=\"display:none\">" + paragraph + "</div>";
+
+        final String expected =
+            "<div itemscope=\"\" itemtype=\"http://schema.org/Article\">" +
+                paragraph +
+                "<div itemscope=\"\" itemtype=\"http://schema.org/Article\">" +
+                    paragraph +
+                "</div>" +
+            "</div>";
+
+        Element result = getArticleElement(htmlArticle);
+        assertEquals(expected, result.getString());
+    }
+
+    public void testOnlyProcessOGNonArticleMovie() {
+        final String paragraph = "<p>" + CONTENT_TEXT + "</p>" +
+                "<p>" + CONTENT_TEXT + "</p>";
+
+        final String htmlArticle =
+            "<h1>" + CONTENT_TEXT + "</h1>" +
+            "<div itemscope itemtype=\"http://schema.org/Movie\">" +
+                paragraph +
+            "</div>";
+
+        // Non-article schema.org types should not use the fast path.
+        Element result = getArticleElement(htmlArticle);
+        assertNull(result);
+    }
+
+    private Element getArticleElement(String html) {
+        mBody.setInnerHTML(html);
+        return DomUtil.getArticleElement(mRoot);
+    }
+
     public void testGetArea() {
         String elements =
             "<div style=\"width: 200px; height: 100px\">w</div>" +
             "<div style=\"width: 300px;\">" +
                 "<div style=\"width: 300px; height: 200px\"></div>" +
             "</div>" +
             "<div style=\"width: 400px; height: 100px\">" +
                 "<div style=\"height: 100%\"></div>" +
             "</div>";
         mBody.setInnerHTML(elements);
 
         Element element = mBody.getFirstChildElement();
         assertEquals(200*100, DomUtil.getArea(element));
 
         element = element.getNextSiblingElement();
         assertEquals(300*200, DomUtil.getArea(element));
 
         element = element.getNextSiblingElement();
         assertEquals(400*100, DomUtil.getArea(element));
 
         element = element.getFirstChildElement();
         assertEquals(400*100, DomUtil.getArea(element));
     }
 }