Fix for keeping lists structure

Fix for keeping lists structure

Created the WebTag class to represent the tags we want to preserve over
the distillation process. A WebTag instance acts like a placeholder
which is injected to the WebDocument when walking the DOM and is used to
track the UL, OL and LI element positions.

This mechanism could be used in the future for all the tags that we want
to preserve.

When the generateOuput() is called for each WebTag, it returns the html
tag that the instance is representing.  A new filter was created to
process these WebTags and mark as content considering its content
inside. This filter runs for the last since it relies on content found
by all the others filters.

BUG=502524
R=mdjones@chromium.org, wychen@chromium.org

Committed: a4e94dd067684796435ca45d2f88e063b5af20af

[Marcelo Correa, 2015-07-09 16:08:23]

marcelorcorrea@gmail.com changed reviewers:
+ cjhopman@chromium.org, mdjones@chromium.org, wychen@chromium.org

[Marcelo Correa, 2015-07-09 16:10:29]

This is a new approach for issue about ordered lists:
https://codereview.chromium.org/1190343004/

[wychen, 2015-07-10 01:42:42]

Thanks for the patch.

We might need to keep LI elements separate, or the filter algorithms would
generate different results. I gave it a try, and the results were usually worse.

This CL only handles OL, but we might also want to handle UL as well, since LI
without UL is considered invalid. The rendered result of orphan LI is slightly
different from LI within UL, probably because each LI is regarded as being in
separate ULs.

One alternative is to inject placeholder objects to the WebDocument to label the
start and end of (nested) OL and UL, make sure the filtering algorithms are not
affected, and clean up the placeholder pairs if the contents within turn out to
be empty after filtering.

[Marcelo Correa, 2015-07-13 13:41:37]

On 2015/07/10 01:42:42, wychen wrote:
> Thanks for the patch.
> 
> We might need to keep LI elements separate, or the filter algorithms would
> generate different results. I gave it a try, and the results were usually
worse.
> 
> This CL only handles OL, but we might also want to handle UL as well, since LI
> without UL is considered invalid. The rendered result of orphan LI is slightly
> different from LI within UL, probably because each LI is regarded as being in
> separate ULs.
> 
> One alternative is to inject placeholder objects to the WebDocument to label
the
> start and end of (nested) OL and UL, make sure the filtering algorithms are
not
> affected, and clean up the placeholder pairs if the contents within turn out
to
> be empty after filtering.

Hello wychen!

Could you give more details to this alternative that you suggested? Not sure if
I get it clearly. :)

I agree with you about handling OLs AND ULs, I've just focused on what CL was
described, my bad.
About this solution: I've tried to keep the whole structure in a single webtext
because I was assuming that the whole content inside an OL is relevant. Maybe
that wasn't a good approach after all. Using this approach to keep elements
separated wouldn't make the filters to miss some content? I mean, The Heuristics
might ignore an element with just one word or something. That was the reason I
used the single webtext approach.

Anyway, if you could provide some more details about this placeholder approach,
I'll try to implement as soon as I can :).
Also, could you provide the Data Set that you use to test the Dom Distiller?

Thanks!

[dalmirdasilva, 2015-07-13 16:55:03]

On 2015/07/13 13:41:37, Marcelo Correa wrote:
> On 2015/07/10 01:42:42, wychen wrote:
> > Thanks for the patch.
> > 
> > We might need to keep LI elements separate, or the filter algorithms would
> > generate different results. I gave it a try, and the results were usually
> worse.
> > 
> > This CL only handles OL, but we might also want to handle UL as well, since
LI
> > without UL is considered invalid. The rendered result of orphan LI is
slightly
> > different from LI within UL, probably because each LI is regarded as being
in
> > separate ULs.
> > 
> > One alternative is to inject placeholder objects to the WebDocument to label
> the
> > start and end of (nested) OL and UL, make sure the filtering algorithms are
> not
> > affected, and clean up the placeholder pairs if the contents within turn out
> to
> > be empty after filtering.
> 
> Hello wychen!
> 
> Could you give more details to this alternative that you suggested? Not sure
if
> I get it clearly. :)
> 
> I agree with you about handling OLs AND ULs, I've just focused on what CL was
> described, my bad.
> About this solution: I've tried to keep the whole structure in a single
webtext
> because I was assuming that the whole content inside an OL is relevant. Maybe
> that wasn't a good approach after all. Using this approach to keep elements
> separated wouldn't make the filters to miss some content? I mean, The
Heuristics
> might ignore an element with just one word or something. That was the reason I
> used the single webtext approach.
> 
> Anyway, if you could provide some more details about this placeholder
approach,
> I'll try to implement as soon as I can :).
> Also, could you provide the Data Set that you use to test the Dom Distiller?
> 
> Thanks!

If we keep everything inside the list, we will eventually define as content
something that could not be. I don't think it is something too bad, since the
definition of a list is something like: "A group of related content". In the
other hand, enabling the filter to work inside list elements as it is, will
certainly remove <li>s which are definitively content, let say if you have a
list of ingredients (that are not too big in terms of words), it will remove
most of the elements and the distilled list will be irrelevant information
(partial information). Maybe there is a third solution: how about, instead of
having only hard coded thresholds when deciding which content is relevant based
on the #of words, use weights associated to these values, and once inside "key
content" - like recipe, we relax a little bit those values (using smaller
weight) since the data inside these areas are more likely to be content? Is it
an acceptable solution?

[wychen, 2015-07-20 22:45:32]

On 2015/07/13 16:55:03, ds wrote:
> On 2015/07/13 13:41:37, Marcelo Correa wrote:
> > Hello wychen!
> > 
> > Could you give more details to this alternative that you suggested? Not sure
> if
> > I get it clearly. :)
> > 
> > I agree with you about handling OLs AND ULs, I've just focused on what CL
was
> > described, my bad.
> > About this solution: I've tried to keep the whole structure in a single
> webtext
> > because I was assuming that the whole content inside an OL is relevant.
Maybe
> > that wasn't a good approach after all. Using this approach to keep elements
> > separated wouldn't make the filters to miss some content? I mean, The
> Heuristics
> > might ignore an element with just one word or something. That was the reason
I
> > used the single webtext approach.
> > 
> > Anyway, if you could provide some more details about this placeholder
> approach,
> > I'll try to implement as soon as I can :).
> > Also, could you provide the Data Set that you use to test the Dom Distiller?
> > 
> > Thanks!
> 
> If we keep everything inside the list, we will eventually define as content
> something that could not be. I don't think it is something too bad, since the
> definition of a list is something like: "A group of related content". In the
> other hand, enabling the filter to work inside list elements as it is, will
> certainly remove <li>s which are definitively content, let say if you have a
> list of ingredients (that are not too big in terms of words), it will remove
> most of the elements and the distilled list will be irrelevant information
> (partial information). Maybe there is a third solution: how about, instead of
> having only hard coded thresholds when deciding which content is relevant
based
> on the #of words, use weights associated to these values, and once inside "key
> content" - like recipe, we relax a little bit those values (using smaller
> weight) since the data inside these areas are more likely to be content? Is it
> an acceptable solution?

It makes sense that a list should be treated as a single entity.

The tricky part is: it would take quite some time until making the data set
public helpful, because the driving, analyzing, and display logic for benchmark
have internal dependencies. In the short term, it would be difficult for
external contributors to evaluate how their changes might impact the
distillation
quality. I'm sorry it is this way.

The filtering logic was written assuming each item in the list are separated,
so aggregating the items as a single element would certainly change the outcome.
Wrongly removing part of the content in a list is not really an issue according
to the benchmark on our data sets, so it might be easier to keep items separated
as is.

The placeholder idea is to inject markers to represent <UL>, <OL>, </UL>, and
</OL>s. For example, the following html code would be translated like this:

<p> (as WebText)
<ul> (as ULStart)
<li> (as WebText)
<ol> (as OLStart)
<li> (as WebText)
</ol> (as OLEnd)
</ul> (as ULEnd)

[Marcelo Correa, 2015-07-29 14:36:26]

Using new approach to keep list structure.

[mdjones, 2015-07-29 17:12:16]

Some initial comments/suggestions.

Once upon a time, I worked on a solution to this problem but got caught up in
other work. I uploaded the very incomplete code in the following CL:

https://codereview.chromium.org/1265533004/

I'm not sure if it's still relevant as it uses a different approach (treating
the entire list as one item), but it might generate some ideas.

https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/DomConverter.java (right):

https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/DomConverter.java:103: builder.embed(new
OLStart());
I think using the "embed" method here is a bit ambiguous. 

Combined with my suggestion in RelevantElements, this could become something
like:
case "OL":
case "UL":
case "LI":
  builder.list(e.getTagName());
  break;

https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
(right):

https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/filters/RelevantElements.java:38: public
PlaceHolder(WebElement start, boolean flush) {
What if there were only one class for a placeholder, something like:

public class PlaceHolder extends WebElement {
  // Maybe instead of enum just pass element.getTagName()
  public static enum PlaceHolderType{UL, OL, LI};
  private bool isStartTag;

  public PlaceHolder(PlaceHolderType type, bool isStart) {...}
  public bool isStart() {...}
  public PlaceHolderType getType() {...}
  ...
}

You could compile the eight classes you have into one and eliminate the need for
this PlaceHolder class.

https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/filters/RelevantElements.java:47: int
stackMark = -1;
I don't understand what this var is for. The only place it is used is to check
if it should modify itself.

[Marcelo Correa, 2015-07-29 17:53:23]

On 2015/07/29 17:12:16, mdjones wrote:
> Some initial comments/suggestions.
> 
> Once upon a time, I worked on a solution to this problem but got caught up in
> other work. I uploaded the very incomplete code in the following CL:
> 
> https://codereview.chromium.org/1265533004/
> 
> I'm not sure if it's still relevant as it uses a different approach (treating
> the entire list as one item), but it might generate some ideas.
> 
>
https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
> File java/org/chromium/distiller/webdocument/DomConverter.java (right):
> 
>
https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
> java/org/chromium/distiller/webdocument/DomConverter.java:103:
builder.embed(new
> OLStart());
> I think using the "embed" method here is a bit ambiguous. 
> 
> Combined with my suggestion in RelevantElements, this could become something
> like:
> case "OL":
> case "UL":
> case "LI":
>   builder.list(e.getTagName());
>   break;
> 
>
https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
> File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
> (right):
> 
>
https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
> java/org/chromium/distiller/webdocument/filters/RelevantElements.java:38:
public
> PlaceHolder(WebElement start, boolean flush) {
> What if there were only one class for a placeholder, something like:
> 
> public class PlaceHolder extends WebElement {
>   // Maybe instead of enum just pass element.getTagName()
>   public static enum PlaceHolderType{UL, OL, LI};
>   private bool isStartTag;
> 
>   public PlaceHolder(PlaceHolderType type, bool isStart) {...}
>   public bool isStart() {...}
>   public PlaceHolderType getType() {...}
>   ...
> }
> 
> You could compile the eight classes you have into one and eliminate the need
for
> this PlaceHolder class.
> 
>
https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
> java/org/chromium/distiller/webdocument/filters/RelevantElements.java:47: int
> stackMark = -1;
> I don't understand what this var is for. The only place it is used is to check
> if it should modify itself.

Hello 
Thanks for your considerations! 

I submitted a similar solution in the patch set 1.
Wychen rejected explaining that lists should not be in one item because the
filters should be processed separated for each element (item).
Also he suggested this approach using place holders to inject.

About your suggestion for using a single class:
This sounds interesting but the way we did was following Wychen's suggestion
which we kind of agree because this would avoid coupling code. 

About the stackMark:
Actually it is not test for modifying itself. It is used to mark a point in the
stack which everything lower than that point would be considered as content
since we found an element which is content. Maybe we could remove this
attribution from the conditional and put above and just test if it is content.

[mdjones, 2015-07-29 21:00:24]

I believe what wychen was referring to with leaving them separate was that an
entire list should not be grouped into a single WebElement or WebText. Having
the single class for placeholders should not affect this. As for the concern
with coupling, the elements that this CL deals with are all related to lists, so
I believe this is okay.

Either way, I prefer that lists not use the "embed" method in DomConverter.

https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
(right):

https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/filters/RelevantElements.java:60:
boolean isContent;
Set isContent to flush here instead, or just use flush below since it is not
changed until the end of the block.

[Marcelo Correa, 2015-07-29 21:21:04]

On 2015/07/29 21:00:24, mdjones wrote:
> I believe what wychen was referring to with leaving them separate was that an
> entire list should not be grouped into a single WebElement or WebText. Having
> the single class for placeholders should not affect this. As for the concern
> with coupling, the elements that this CL deals with are all related to lists,
so
> I believe this is okay.
> 
> Either way, I prefer that lists not use the "embed" method in DomConverter.
> 
>
https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
> File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
> (right):
> 
>
https://codereview.chromium.org/1230583006/diff/20001/java/org/chromium/disti...
> java/org/chromium/distiller/webdocument/filters/RelevantElements.java:60:
> boolean isContent;
> Set isContent to flush here instead, or just use flush below since it is not
> changed until the end of the block.


Hello!
Yes, that is exactly it. I was referring to your solution, which I think is
similar to my first path set. (treat the list as one item).

What did you mean about:  "so I believe this is okay." ? hehe :)

I've created this solution to be kind of generic. I mean in the future we could
deal with other elements, like this one:
https://code.google.com/p/chromium/issues/detail?id=511485&q=label%3ACr-UI-Br...
without the need to do more modifications.
That's why I've decided to use embed.  But anyway, I think your concern is
pretty reasonable. So I can change "embed" to "list" without problem.

Also I'll do as you asked about setting the content before the conditional.

Thanks for all your suggestions!

[wychen, 2015-07-29 23:53:28]

Hi,

Just talked with mdjones, he didn't mean to make you follow his old CL
(https://codereview.chromium.org/1265533004/). It's merely something to
reference if it's relevant. Sorry about the confusion.

We should still keep the list items separate, like what you did in patch set 2,
but it would be nicer to consolidate all the {OL,UL}{Start,End} classes into
one. I know the way I wrote looked like suggesting the name for the classes, but
what I thought is one class with some enum data like mdjones said. It's much
less verbose this way.

BTW, why is LI{Start,End} needed? Are you trying to preserve the possibly
incorrect nested list like option 1 in this page
(http://stackoverflow.com/questions/5899337/proper-way-to-make-html-nested-list)?

We might also need to add another scanning pass to mark these PlaceHolder
isContent when needed, and skip PlaceHolder in RelevantElements::process to
avoid false positives.

[Marcelo Correa, 2015-07-30 01:23:59]

On 2015/07/29 23:53:28, wychen wrote:
> Hi,
> 
> Just talked with mdjones, he didn't mean to make you follow his old CL
> (https://codereview.chromium.org/1265533004/). It's merely something to
> reference if it's relevant. Sorry about the confusion.
> 
> We should still keep the list items separate, like what you did in patch set
2,
> but it would be nicer to consolidate all the {OL,UL}{Start,End} classes into
> one. I know the way I wrote looked like suggesting the name for the classes,
but
> what I thought is one class with some enum data like mdjones said. It's much
> less verbose this way.
> 
> BTW, why is LI{Start,End} needed? Are you trying to preserve the possibly
> incorrect nested list like option 1 in this page
>
(http://stackoverflow.com/questions/5899337/proper-way-to-make-html-nested-list)
> 
> We might also need to add another scanning pass to mark these PlaceHolder
> isContent when needed, and skip PlaceHolder in RelevantElements::process to
> avoid false positives.

Hello wychen!
No problem! I wasn't confused at all, I was just mentioning that I tried a
similar solution in patch set 1. I understood mdjones suggestion, which was
great :D

OK, I understand what you guys mean about verbosity, however, not sure if I get
it, but using using this approach that you are suggesting wouldn't we need to
implement a switch-case + conditional; considering Type and isStart flag in
generateOuput method? If we use this PlaceHolder for other elements in the
future, we would have to add more conditionals to the method, right? I know that
i've created a lot of small classes, but in my opinion I don't see this as a
problem. But if you guys think this is the best to do, then ok, I'll do it using
this approach :).


About the LI {Start,End} - I had to inject a LI PlaceHolder specially because
our approach wouldn't work for some cases, for example:
<ol>
  <li>
    <ol>
      <li>text</li>
       ...
This would generate something like OLStart, OLStart, WebText, OLEnd, OLEnd
or <ol> 
     <ol>
       <li>text</li>
Another example   
<ol>
  <li>
    <p>text</li>
    ...
This would generate OLStart,WebText, OLEnd. However, when generating the webtext
output, would return something like: <p>text</p> which ends like this:
<ol>
  <p>text</p>
   ...

 Not sure if I understand what you said about adding another scanning pass... 
Do you think where I put the logic won't work? I put there because I'm setting
the content as true or false when iterating over the element.

Thanks for all your suggestions, and sorry for try to explain my point of view
since it's your code and you know it better than everyone! :D

[Marcelo Correa, 2015-07-30 18:01:12]

All comments were addressed.

[wychen, 2015-08-01 01:00:21]

Thanks for your explanation about LI. I see it is necessary.

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/DomConverter.java (right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/DomConverter.java:22: import
java.util.ArrayList;
nitpick: why deleting the empty line above?

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/PlaceHolder.java (right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/PlaceHolder.java:3: public class
PlaceHolder extends WebElement {
Might need a more specific name, but not too specific so that it only works for
lists, because blockquote might use this later.

WebTag?

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/WebDocument.java (right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/WebDocument.java:32: public void
addList(WebElement list) {
addList(PlaceHolder list)?

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/WebDocumentBuilderInterface.java
(right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/WebDocumentBuilderInterface.java:18:
void list(WebElement e);
list(PlaceHolder e)?

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/WebText.java (right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/WebText.java:76: return
elementClonedRoot.getInnerHTML();
I see why this is needed in this implementation, but would you mind leaving a
comment here saying if an LI element has no child element, the <li></li> tags
would be duplicated?

I think blockquote will also be a special case here.

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
(right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/filters/RelevantElements.java:36: public
static void handlePlaceHolderElements(
It makes sense to move the logic to a new file.

The test coverage of this function seems pretty low.

https://codereview.chromium.org/1230583006/diff/60001/javatests/org/chromium/...
File javatests/org/chromium/distiller/ContentExtractorTest.java (right):

https://codereview.chromium.org/1230583006/diff/60001/javatests/org/chromium/...
javatests/org/chromium/distiller/ContentExtractorTest.java:186: "<li>" +
CONTENT_TEXT + "</li>" +
nitpick: nested html would be more readable if indented according to the
structure.

https://codereview.chromium.org/1230583006/diff/60001/javatests/org/chromium/...
javatests/org/chromium/distiller/ContentExtractorTest.java:254: public void
testPreserveNestedUnorderedList() {
We can also create a test with malformed html, by directly setting innerHTML. It
should be invalid html, but looks fine in Chrome. For example, <li> without
</li>, non <li> elements in <ul> and <ol>, which is partly covered in
testPreserveNestedUnorderedListWithOtherElementsInside(), etc.

https://codereview.chromium.org/1230583006/diff/60001/javatests/org/chromium/...
File
javatests/org/chromium/distiller/webdocument/filters/RelevantElementsTest.java
(right):

https://codereview.chromium.org/1230583006/diff/60001/javatests/org/chromium/...
javatests/org/chromium/distiller/webdocument/filters/RelevantElementsTest.java:84:
PlaceHolder placeHolder = builder.addListStart();
nitpick: rename placeHolder to olStart?

https://codereview.chromium.org/1230583006/diff/60001/javatests/org/chromium/...
javatests/org/chromium/distiller/webdocument/filters/RelevantElementsTest.java:114:
builder.addText("text 2").setIsContent(true);
We should make some items non-content.

https://codereview.chromium.org/1230583006/diff/60001/javatests/org/chromium/...
javatests/org/chromium/distiller/webdocument/filters/RelevantElementsTest.java:132:
public void testRelevantNestedList() {
We should make some of the items non-content, and complicated enough to test
handlePlaceHolderElements().

[Marcelo Correa, 2015-08-01 01:28:43]

Hello wychen!
Yeah, I think WebTag would be a good name for this PlaceHolder. In the future we
can use for other tags, like you said blackquote.
I'll make the changes you suggested and add more unit tests.

Thanks!

[dalmirdasilva, 2015-08-01 16:27:04]

dalmirdasilva@gmail.com changed reviewers:
+ dalmirdasilva@gmail.com

[dalmirdasilva, 2015-08-01 16:27:04]

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/WebText.java (right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/WebText.java:76: return
elementClonedRoot.getInnerHTML();
Is here the case where we can ask someone else whether we need to use the
innerHTML? A static method inside PlaceHolder which knows all the tags that will
be duplicated. Otherwise we will have to change this code every time we include
a new tag (blockquote) .

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

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
(right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/filters/RelevantElements.java:62:
StackEntry stackEntry = holderStack.pop();
This might raise EmptyStackException if the HTML isn't balanced (more tag end
than tag start). Shouldn't we catch it?

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

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/PlaceHolder.java (right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/PlaceHolder.java:35: return
sb.toString();
I think the following is more clear:

return "<" + (isStartTag ? "" : "/") + type + ">";

The spec for HTML indicates uppercase tags, but any case is accepted.

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
(right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/filters/RelevantElements.java:46: }
What if we use Set<PlaceHolder> and Stack<PlaceHolder> instead of this class?

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

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
(right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/filters/RelevantElements.java:46: }
On 2015/08/03 16:57:55, mdjones wrote:
> What if we use Set<PlaceHolder> and Stack<PlaceHolder> instead of this class?

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]

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
(right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/filters/RelevantElements.java:46: }
> ...use Set<PlaceHolder> and Stack<PlaceHolder>...

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.

[Marcelo Correa, 2015-08-03 19:36:51]

All comments addressed again, except that one related to use Set and Stack
instead of StackEntry class.
mdjones: could you give more details of how we implement this approach using Set
instead of StackEntry class?
Thanks!

[mdjones, 2015-08-03 20:22:20]

On 2015/08/03 19:36:51, Marcelo Correa wrote:
> All comments addressed again, except that one related to use Set and Stack
> instead of StackEntry class.
> mdjones: could you give more details of how we implement this approach using
Set
> instead of StackEntry class?
> Thanks!

The stack holds the WebTags in the order that you are processing them. The set
tracks if each is currently content; i.e. if the WebTag is in the set, it is
determined to be content.

Alternatively, each WebElement, which WebTag extends, has a flag marking it as
content or not. This can be leveraged instead of using the class as well.

One way I looked at the problem involved resetting the isContent flag to false
whenever I reach a starting WebTag. If you have found content by the time you
reach an end WebTag you need to mark all parent lists (assuming it is nested) as
content; the stack makes this quite easy. Flagging an end WebTag as content
becomes as simple as checking if the top of the stack is content before popping
it.

[Marcelo Correa, 2015-08-03 21:01:30]

Fixed what mdjones requested.

[wychen, 2015-08-03 21:26:42]

On 2015/08/03 15:43:50, ds wrote:
>
https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
> File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
> (right):
> 
>
https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
> java/org/chromium/distiller/webdocument/filters/RelevantElements.java:62:
> StackEntry stackEntry = holderStack.pop();
> This might raise EmptyStackException if the HTML isn't balanced (more tag end
> than tag start). Shouldn't we catch it?

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

[mdjones, 2015-08-03 23:29:45]

https://codereview.chromium.org/1230583006/diff/100001/java/org/chromium/dist...
File java/org/chromium/distiller/ContentExtractor.java (right):

https://codereview.chromium.org/1230583006/diff/100001/java/org/chromium/dist...
java/org/chromium/distiller/ContentExtractor.java:96:
WebTagStructureKeeper.process(documentInfo.document);
This should probably be the last filter to run since it relies on content found
by all the others.

https://codereview.chromium.org/1230583006/diff/100001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/filters/WebTagStructureKeeper.java
(right):

https://codereview.chromium.org/1230583006/diff/100001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/filters/WebTagStructureKeeper.java:10:
public class WebTagStructureKeeper {
How about NestedElement{Builder|Organizer|Retainer}?

https://codereview.chromium.org/1230583006/diff/100001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/filters/WebTagStructureKeeper.java:17:
if (e instanceof WebText) {
Though I'm not sure it is a common case, this does not handle image lists. It
may be better to have this condition be:

if(!(e instanceof WebTag)) {...

https://codereview.chromium.org/1230583006/diff/100001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/filters/WebTagStructureKeeper.java:29:
boolean content = isContent || stackMark >= stack.size();
isContent |= stackMark >= stackSize();

Then just use isContent.

[wychen, 2015-08-04 02:37:01]

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
File java/org/chromium/distiller/webdocument/filters/RelevantElements.java
(right):

https://codereview.chromium.org/1230583006/diff/60001/java/org/chromium/disti...
java/org/chromium/distiller/webdocument/filters/RelevantElements.java:62:
StackEntry stackEntry = holderStack.pop();
On 2015/08/03 15:43:50, ds wrote:
> This might raise EmptyStackException if the HTML isn't balanced (more tag end
> than tag start). Shouldn't we catch it?

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

https://codereview.chromium.org/1230583006/diff/100001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/filters/WebTagStructureKeeper.java
(right):

https://codereview.chromium.org/1230583006/diff/100001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/filters/WebTagStructureKeeper.java:35:
isContent = startWebTag.getIsContent();
Does this pass the test? Moving this line 2 lines up would make the logic the
same as patch set 5.

https://codereview.chromium.org/1230583006/diff/140001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/DomConverter.java (right):

https://codereview.chromium.org/1230583006/diff/140001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/DomConverter.java:106: builder.list(new
WebTag(e.getTagName(), WebTag.TagType.START));
If we skip this when there's no child element, then we don't need to special
case LI in generateOutput().

https://codereview.chromium.org/1230583006/diff/140001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/filters/NestedElementRetainer.java
(right):

https://codereview.chromium.org/1230583006/diff/140001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/filters/NestedElementRetainer.java:34:
isContent = startWebTag.getIsContent();
This line doesn't seem necessary.

https://codereview.chromium.org/1230583006/diff/140001/javatests/org/chromium...
File javatests/org/chromium/distiller/ContentExtractorTest.java (right):

https://codereview.chromium.org/1230583006/diff/140001/javatests/org/chromium...
javatests/org/chromium/distiller/ContentExtractorTest.java:346: public void
testMalFormedListStructureWithExtraLITagEnd() {
nit: Malformed is a word, so camel case should follow.

[Marcelo Correa, 2015-08-04 03:47:03]

marcelorcorrea@gmail.com changed reviewers:
- dalmirdasilva@gmail.com

[Marcelo Correa, 2015-08-04 03:47:04]

Fixed isContent flag in NestedElementRetainer.

[Marcelo Correa, 2015-08-04 03:54:28]

Hello wychen,
I believe it is necessary because we might have situations like
<ol><li><ol><li><p>text</p></li>....
So li element can have child element but might not necessarily be webtext. It
can be another ol, or  if it is a webText, it could generate <p>text</p>.

[wychen, 2015-08-04 05:10:10]

On 2015/08/04 03:54:28, Marcelo Correa wrote:
> Hello wychen,
> I believe it is necessary because we might have situations like
> <ol><li><ol><li><p>text</p></li>....
> So li element can have child element but might not necessarily be webtext. It
> can be another ol, or  if it is a webText, it could generate <p>text</p>.

What I said is to insert WebTag if it has element children. In your example, all
the <li> and </li>s are converted to WebTag.

In another example below, the outer <li> is kept as WebTag, but the inner one is
skipped, because the inner one doesn't have element children, and would be a
WebText with li tag.

<ol><li><ol><li>text</li>...

BTW, it would be easier to read if you can reply in the code diff section, and
click "publish+mail comments".

[Marcelo Correa, 2015-08-04 16:34:48]

https://codereview.chromium.org/1230583006/diff/140001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/DomConverter.java (right):

https://codereview.chromium.org/1230583006/diff/140001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/DomConverter.java:106: builder.list(new
WebTag(e.getTagName(), WebTag.TagType.START));
I don't think we can do such thing.
There are a few cases when this solution is not going to handle. Let say
we have this HTML:
<ol>
  <li>
    Text
    <p>Text</p>
  </li>
<ol>
If we decide to not include de LI tag, it will end up with a HTML like
this:
<ol>
  <li>Text</li>
  <p>Text</p>
</ol>
If we decide to include the LI, the final HTML will look like this:
<ol>
  <li>
    <li>Text</li>
    <p>Text</p>
  </li>
</ol>
Both cases we have to avoid the extra LI in the WebText class. 
We don't think it is possible to cover all cases only with logic here.

[wychen, 2015-08-05 00:01:02]

https://codereview.chromium.org/1230583006/diff/140001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/DomConverter.java (right):

https://codereview.chromium.org/1230583006/diff/140001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/DomConverter.java:106: builder.list(new
WebTag(e.getTagName(), WebTag.TagType.START));
I see. You are right. The tag is copied to WebText when the content is a text
node.

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebDocument.java (right):

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebDocument.java:32: public void
addList(WebTag list) {
addTag()?

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebDocumentBuilderInterface.java
(right):

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebDocumentBuilderInterface.java:18:
void list(WebTag e);
tag()?

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebText.java (right):

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebText.java:72: // Since LI Tag is
being wrapped by a WebTag instance,
by a pair of |WebTag|s

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebText.java:73: // We only need to get
the innerHTML, otherwise
nit: we

[Marcelo Correa, 2015-08-05 00:26:35]

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebText.java (right):

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebText.java:72: // Since LI Tag is
being wrapped by a WebTag instance,
On 2015/08/05 00:01:02, wychen wrote:
> by a pair of |WebTag|s

You mean {@link WebTag}s or |WebTag|s?

[Marcelo Correa, 2015-08-05 00:44:04]

Comments addressed

[wychen, 2015-08-05 00:52:15]

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebText.java (right):

https://codereview.chromium.org/1230583006/diff/160001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebText.java:72: // Since LI Tag is
being wrapped by a WebTag instance,
On 2015/08/05 00:26:35, Marcelo Correa wrote:
> On 2015/08/05 00:01:02, wychen wrote:
> > by a pair of |WebTag|s
> 
> You mean {@link WebTag}s or |WebTag|s?

Oh. Should be {@link WebTag}s. Thanks for pointing it out.

[wychen, 2015-08-05 01:04:42]

The title to this CL and the commit message might need to be updated to reflect
the changed implementation.

https://codereview.chromium.org/1230583006/diff/180001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebText.java (right):

https://codereview.chromium.org/1230583006/diff/180001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebText.java:72: // Since LI Tag is
being wrapped by a pair of {@link WebTag}s instance,
I think it could be either "a pair of {@link WebTag}s," or "a pair of {@link
WebTag} instances,". I'm not sure the grammar is correct now.

[Marcelo Correa, 2015-08-05 01:09:22]

fixed grammar.

[Marcelo Correa, 2015-08-05 03:11:12]

Title changed to reflect the new implementation

[mdjones, 2015-08-05 15:44:35]

https://codereview.chromium.org/1230583006/diff/200001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebTag.java (right):

https://codereview.chromium.org/1230583006/diff/200001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebTag.java:3: public class WebTag
extends WebElement {
Briefly describe what this is and how it is used.

https://codereview.chromium.org/1230583006/diff/200001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/filters/NestedElementRetainer.java
(right):

https://codereview.chromium.org/1230583006/diff/200001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/filters/NestedElementRetainer.java:9:
public class NestedElementRetainer {
Add some documentation about what this class is and how it works.

[wychen, 2015-08-05 15:52:10]

I've rewrapped the commit message, and added back the bug id.

[Marcelo Correa, 2015-08-05 16:25:56]

Classes were documented.

[Marcelo Correa, 2015-08-05 16:26:21]

On 2015/08/05 15:52:10, wychen wrote:
> I've rewrapped the commit message, and added back the bug id.

Thanks wychen! 
Sorry about that!

[wychen, 2015-08-05 19:46:15]

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebText.java (right):

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebText.java:78: } else if
(elementClonedRoot.getTagName().equals("LI")) {
UL and OL might need to use innerHTML as well.
Even though they shouldn't according to spec, we can't rely on web developers
following the spec, especially when it looks fine.

One possible malformed HTML snippet:

<ol>
  <li>one</li>
  outside
  <li>two</li>
</ol>

If all the text blocks are content, the distilled result would be:

<ol>
  <li>one</li>
  <ol>outside</ol>
  <li>two</li>
</ol>

Then the result is visually different.

[Marcelo Correa, 2015-08-05 20:31:24]

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebText.java (right):

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebText.java:78: } else if
(elementClonedRoot.getTagName().equals("LI")) {
On 2015/08/05 19:46:15, wychen wrote:
> UL and OL might need to use innerHTML as well.
> Even though they shouldn't according to spec, we can't rely on web developers
> following the spec, especially when it looks fine.
> 
> One possible malformed HTML snippet:
> 
> <ol>
>   <li>one</li>
>   outside
>   <li>two</li>
> </ol>
> 
> If all the text blocks are content, the distilled result would be:
> 
> <ol>
>   <li>one</li>
>   <ol>outside</ol>
>   <li>two</li>
> </ol>
> 
> Then the result is visually different.

Yes, You are right. We might need to prevent that.

But I was wondering, How about we delegate this verification to another class?
Instead of WebText test if element is LI, OL, UL or another future elements, 
We ask to the WebTag itself.
For example:
WebTag.shouldUseOnlyInnerText(e.getTagName());
And WebTag has the allowed elements that should get the innerHTML.
It's just an idea to avoid writing more logic in the WebText instance.
What do you think?
What name for the method should we pick? Should we delegate this logic to WebTag
or another class?

[wychen, 2015-08-06 00:04:52]

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebText.java (right):

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebText.java:78: } else if
(elementClonedRoot.getTagName().equals("LI")) {
On 2015/08/05 20:31:24, Marcelo Correa wrote:
> 
> Yes, You are right. We might need to prevent that.
> 
> But I was wondering, How about we delegate this verification to another class?
> Instead of WebText test if element is LI, OL, UL or another future elements, 
> We ask to the WebTag itself.
> For example:
> WebTag.shouldUseOnlyInnerText(e.getTagName());
> And WebTag has the allowed elements that should get the innerHTML.
> It's just an idea to avoid writing more logic in the WebText instance.
> What do you think?
> What name for the method should we pick? Should we delegate this logic to
WebTag
> or another class? 

It would be cleaner to aggregate the tag list in one place. DomConverter can
also use it.
I think it could be a static method of WebTag.
As for the naming, maybe WebTag.canBeNested()? I'm not good at naming. :p

[Marcelo Correa, 2015-08-06 01:17:58]

Verification Logic moved to WebTag class

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebText.java (right):

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebText.java:78: } else if
(elementClonedRoot.getTagName().equals("LI")) {
On 2015/08/06 00:04:52, wychen wrote:
> On 2015/08/05 20:31:24, Marcelo Correa wrote:
> > 
> > Yes, You are right. We might need to prevent that.
> > 
> > But I was wondering, How about we delegate this verification to another
class?
> > Instead of WebText test if element is LI, OL, UL or another future elements,

> > We ask to the WebTag itself.
> > For example:
> > WebTag.shouldUseOnlyInnerText(e.getTagName());
> > And WebTag has the allowed elements that should get the innerHTML.
> > It's just an idea to avoid writing more logic in the WebText instance.
> > What do you think?
> > What name for the method should we pick? Should we delegate this logic to
> WebTag
> > or another class? 
> 
> It would be cleaner to aggregate the tag list in one place. DomConverter can
> also use it.
> I think it could be a static method of WebTag.
> As for the naming, maybe WebTag.canBeNested()? I'm not good at naming. :p

That makes two of us :P

[mdjones, 2015-08-06 15:26:20]

https://codereview.chromium.org/1230583006/diff/260001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/WebTag.java (right):

https://codereview.chromium.org/1230583006/diff/260001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/WebTag.java:18: private static
List<String> canBeNestedTags = Arrays.asList("UL", "OL", "LI");
Why not Set/HashSet? The TableClassifier and EmbedExtractors do something
similar.

As for the name, maybe "nestingTags"?

[Marcelo Correa, 2015-08-06 16:16:44]

Using Set instead of List.

[Marcelo Correa, 2015-08-10 13:43:52]

On 2015/08/06 16:16:44, Marcelo Correa wrote:
> Using Set instead of List.

Is there anything else you guys noticed that we need to change?
what was the result after running the dataset?

[wychen, 2015-08-11 00:12:15]

The score remains the same as of patch set 15.

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/DomConverter.java (right):

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/DomConverter.java:106: builder.tag(new
WebTag(e.getTagName(), WebTag.TagType.START));
We could move this out of the switch and use canBeNested() instead.

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/DomConverter.java:156: builder.tag(new
WebTag(e.getTagName(), WebTag.TagType.END));
And here.

[Marcelo Correa, 2015-08-11 00:21:01]

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
File java/org/chromium/distiller/webdocument/DomConverter.java (right):

https://codereview.chromium.org/1230583006/diff/220001/java/org/chromium/dist...
java/org/chromium/distiller/webdocument/DomConverter.java:106: builder.tag(new
WebTag(e.getTagName(), WebTag.TagType.START));
On 2015/08/11 00:12:15, wychen wrote:
> We could move this out of the switch and use canBeNested() instead.

Sure, But this wouldn't change the pattern how the elements are filtered?
I mean, excluding the embed extractor and hidden elements, everything else is
filtered by the switch.

[Marcelo Correa, 2015-08-11 16:10:28]

canBeNested move out of the switch.

[wychen, 2015-08-11 20:15:36]

lgtm

[Marcelo Correa, 2015-08-19 14:01:51]

Great!
mdjones, is there anything else ?

[mdjones, 2015-08-25 20:41:15]

lgtm

[mdjones, 2015-08-25 22:46:33]

Committed patchset #16 (id:300001) manually as
a4e94dd067684796435ca45d2f88e063b5af20af (presubmit successful).