Fix for keeping lists structure

java/org/chromium/distiller/webdocument/filters/RelevantElements.java

 // Copyright 2015 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.webdocument.filters;
 
+import org.chromium.distiller.webdocument.PlaceHolder;
 import org.chromium.distiller.webdocument.WebDocument;
 import org.chromium.distiller.webdocument.WebElement;
 import org.chromium.distiller.webdocument.WebText;
 
+import java.util.List;
+import java.util.Stack;
+
 public class RelevantElements {
     public static boolean process(WebDocument document) {
         boolean changes = false;
         boolean inContent = false;
 
         for (WebElement e : document.getElements()) {
             if (e.getIsContent()) {
                 inContent = true;
             } else if (e instanceof WebText) {
                 inContent = false;
             } else {
                 if (inContent) {
                     e.setIsContent(true);
                     changes = true;
                 }
             }
         }
+        handlePlaceHolderElements(document.getElements());
         return changes;
     }
+
+    public static void handlePlaceHolderElements(

[wychen, 2015/08/01 01:00:20]

It makes sense to move the logic to a new file.

The test coverage of this function seems pretty low.

+            List<WebElement> elements) {
+        class StackEntry {
+            public StackEntry(WebElement start, boolean isContent) {
+                this.start = start;
+                this.isContent = isContent;
+            }
+
+            WebElement start;
+            boolean isContent;
+        }

[mdjones, 2015/08/03 16:57:55]

What if we use Set<PlaceHolder> and Stack<PlaceHolder> instead of this class?

[dalmirdasilva, 2015/08/03 17:13:18]

Usually, we use stack when parsing such kinds of thing. Here we (I'm working
with Marcelo) used stack because we thought it is the most appropriated
representation for the data structure we want. Every time we found a End Tag we
pop the stack to retrieve it's corresponding Start Tag and the isContent flag ,
which holds the bool value at the context when the StackEntry was pushed. Then
we set the isContent for the Start and End tags appropriately and restore the
flag. Why do you think Set would be better than Stack? Thanks.

[mdjones, 2015/08/03 18:10:54]

I don't, I think we should use both Set and Stack instead of creating this extra
class. The set would track if an element was content.

+        boolean isContent = false;
+        int stackMark = -1;
+        Stack<StackEntry> holderStack = new Stack<>();
+
+        for (WebElement e : elements) {
+            if (e instanceof WebText) {
+                if (!isContent) {
+                    isContent = e.getIsContent();
+                }
+            } else if (e instanceof PlaceHolder) {
+                PlaceHolder ph = (PlaceHolder) e;
+                if (ph.isStart()) {
+                    holderStack.push(new StackEntry(e, isContent));
+                    isContent = false;
+                } else {
+                    StackEntry stackEntry = holderStack.pop();

[dalmirdasilva, 2015/08/03 15:43:50]

This might raise EmptyStackException if the HTML isn't balanced (more tag end
than tag start). Shouldn't we catch it?

[wychen, 2015/08/04 02:37:00]

I think Chrome fixes that for you when you access the DOM tree.
Malformed tests should be able to catch it.

+                    boolean content = isContent || stackMark >= holderStack.size();
+                    if (content) {
+                        stackMark = holderStack.size() - 1;
+                    }
+                    stackEntry.start.setIsContent(content);
+                    e.setIsContent(content);
+                    isContent = stackEntry.isContent;
+                }
+            }
+        }
+    }
 }