filter out invisible elements

filter out invisible elements

1) un-nest DomToSaxVisitor from DomToSaxParser and remove DomToSaxParser

2) impl FilteringDomVisitor
- ContentExtractor calls FilteringDomVisitor which calls DomToSaxVisitor
- filters out invisible nodes, including computed style: display:none,
  visibility:hidden, hidden attribute
- passes other nodes on to DomToSaxVisitor for processing by ContentHandler
- added tests

BUG=367243
R=cjhopman@chromium.org

Committed: 0a31800

[cjhopman, 2014-05-09 00:27:31]

https://codereview.chromium.org/275493007/diff/40001/src/com/dom_distiller/cl...
File src/com/dom_distiller/client/FilteringDomVisitor.java (right):

https://codereview.chromium.org/275493007/diff/40001/src/com/dom_distiller/cl...
src/com/dom_distiller/client/FilteringDomVisitor.java:25: private final
DomToSaxVisitor domToSaxVisitor;
This shouldn't need to know that its a DomToSaxVisitor, just have it hold a
DomWalker.visitor and then take that in in its constructor.

[kuan, 2014-05-09 00:35:20]

https://codereview.chromium.org/275493007/diff/40001/src/com/dom_distiller/cl...
File src/com/dom_distiller/client/FilteringDomVisitor.java (right):

https://codereview.chromium.org/275493007/diff/40001/src/com/dom_distiller/cl...
src/com/dom_distiller/client/FilteringDomVisitor.java:25: private final
DomToSaxVisitor domToSaxVisitor;
On 2014/05/09 00:27:31, cjhopman wrote:
> This shouldn't need to know that its a DomToSaxVisitor, just have it hold a
> DomWalker.visitor and then take that in in its constructor.

i need to access DomToSaxVisitor.getTextNodes() which is returned from static
parse() method.  is there another way to do this if i change the type to
DomWalker.visitor?

[cjhopman, 2014-05-09 00:38:04]

https://codereview.chromium.org/275493007/diff/40001/src/com/dom_distiller/cl...
File src/com/dom_distiller/client/FilteringDomVisitor.java (right):

https://codereview.chromium.org/275493007/diff/40001/src/com/dom_distiller/cl...
src/com/dom_distiller/client/FilteringDomVisitor.java:25: private final
DomToSaxVisitor domToSaxVisitor;
On 2014/05/09 00:35:20, kuan wrote:
> On 2014/05/09 00:27:31, cjhopman wrote:
> > This shouldn't need to know that its a DomToSaxVisitor, just have it hold a
> > DomWalker.visitor and then take that in in its constructor.
> 
> i need to access DomToSaxVisitor.getTextNodes() which is returned from static
> parse() method.  is there another way to do this if i change the type to
> DomWalker.visitor?
Yeah, parse() will have to create the DomToSaxVisitor to construct the
FilteringDomVisitor. It can then just call getTextNodes() on it directly.

Also, parse() should be moved somewhere else, probably into
ContentExtractor.java would be best.

[kuan, 2014-05-09 01:05:31]

i've addressed ur comment in patch set 4.  ptal.  thx.

https://codereview.chromium.org/275493007/diff/40001/src/com/dom_distiller/cl...
File src/com/dom_distiller/client/FilteringDomVisitor.java (right):

https://codereview.chromium.org/275493007/diff/40001/src/com/dom_distiller/cl...
src/com/dom_distiller/client/FilteringDomVisitor.java:25: private final
DomToSaxVisitor domToSaxVisitor;
On 2014/05/09 00:38:04, cjhopman wrote:
> On 2014/05/09 00:35:20, kuan wrote:
> > On 2014/05/09 00:27:31, cjhopman wrote:
> > > This shouldn't need to know that its a DomToSaxVisitor, just have it hold
a
> > > DomWalker.visitor and then take that in in its constructor.
> > 
> > i need to access DomToSaxVisitor.getTextNodes() which is returned from
static
> > parse() method.  is there another way to do this if i change the type to
> > DomWalker.visitor?
> Yeah, parse() will have to create the DomToSaxVisitor to construct the
> FilteringDomVisitor. It can then just call getTextNodes() on it directly.
> 
> Also, parse() should be moved somewhere else, probably into
> ContentExtractor.java would be best.
> 

Done.

[cjhopman, 2014-05-09 01:44:10]

https://codereview.chromium.org/275493007/diff/80001/src/com/dom_distiller/cl...
File src/com/dom_distiller/client/FilteringDomVisitor.java (right):

https://codereview.chromium.org/275493007/diff/80001/src/com/dom_distiller/cl...
src/com/dom_distiller/client/FilteringDomVisitor.java:35:
e.hasAttribute("hidden") || DomUtil.hasClassName(e, "hidden")) {
I don't know how correct this is (particularly the hasClassName can be wrong). I
don't know what is in e.getStyle() (i.e. if it is just the style explicitly on
the element or if it is the computed style).

According to
http://blog.jquery.com/2009/02/20/jquery-1-3-2-released/#:visible.2F:hidden_O...
a fast and correct approach is to check `offsetWidth <= 0 && offsetHeight <= 0`
but that doesn't actually seem to be what we want (I think that descendants of
such an element can still be visible).

What happens on a page like this:

<style>#foo { display: none }</style>
<div id="foo">blah</div>

I think that if it works for that case, this is probably fine for now (though I
think we should remove the hasClassName part unless we know that it has really
low false positives).

[kuan, 2014-05-13 11:21:03]

i've addressed all comments in patch set 7.  ptal.  thx.

https://codereview.chromium.org/275493007/diff/80001/src/com/dom_distiller/cl...
File src/com/dom_distiller/client/FilteringDomVisitor.java (right):

https://codereview.chromium.org/275493007/diff/80001/src/com/dom_distiller/cl...
src/com/dom_distiller/client/FilteringDomVisitor.java:35:
e.hasAttribute("hidden") || DomUtil.hasClassName(e, "hidden")) {
On 2014/05/09 01:44:11, cjhopman wrote:
> I don't know how correct this is (particularly the hasClassName can be wrong).
I
> don't know what is in e.getStyle() (i.e. if it is just the style explicitly on
> the element or if it is the computed style).
> 
> According to
>
http://blog.jquery.com/2009/02/20/jquery-1-3-2-released/#:visible.2F:hidden_O...
> a fast and correct approach is to check `offsetWidth <= 0 && offsetHeight <=
0`
> but that doesn't actually seem to be what we want (I think that descendants of
> such an element can still be visible).
> 
> What happens on a page like this:
> 
> <style>#foo { display: none }</style>
> <div id="foo">blah</div>
> 
> I think that if it works for that case, this is probably fine for now (though
I
> think we should remove the hasClassName part unless we know that it has really
> low false positives).
> 

there're 4 urls in the bug rpt that i use for verification.

1) i tried `offsetWidth <= 0 && offsetHeight <= 0` - it returns empty distilled
content for the chicago-eats url.

2) getComputedStyle() works better, even fixes 2 of the 4 urls which failed w/
my previous approach of element.getStyle().  however, i can't get it to work in
the test.  after a few days of trying everything i could think of without
success, i ended up blocking and adding a todo for the test.  i've a feeling
getComputedStyle() simply returns element.getStyle() in gwt. :(

3) i've removed hasClassName().

[cjhopman, 2014-05-13 16:20:53]

https://codereview.chromium.org/275493007/diff/110001/src/com/dom_distiller/c...
File src/com/dom_distiller/client/DomUtil.java (right):

https://codereview.chromium.org/275493007/diff/110001/src/com/dom_distiller/c...
src/com/dom_distiller/client/DomUtil.java:68: // Standard (includes IE9)
We don't care about IE.

In everything that we care about (except maybe gwt's test modes),
getComputedStyle is available on the window object, you don't need the
document.defaultView.

So, just:
return getComputedStyle(el, null);

If required for gwt tests, just fall back to el.style (i.e. get rid of the case
for older ie).

https://codereview.chromium.org/275493007/diff/110001/src/com/dom_distiller/c...
File src/com/dom_distiller/client/FilteringDomVisitor.java (right):

https://codereview.chromium.org/275493007/diff/110001/src/com/dom_distiller/c...
src/com/dom_distiller/client/FilteringDomVisitor.java:39: if
(style.getDisplay().equals("none") || style.getVisibility().equals("hidden")) {
how about:
visible = !(style.getDisplay().equals("none") ||
style.getVisibility().equals("hidden"));

[kuan, 2014-05-13 16:55:43]

i've addressed all comments in patch set 8 (which also includes rebase).  ptal. 
thx.

https://codereview.chromium.org/275493007/diff/110001/src/com/dom_distiller/c...
File src/com/dom_distiller/client/DomUtil.java (right):

https://codereview.chromium.org/275493007/diff/110001/src/com/dom_distiller/c...
src/com/dom_distiller/client/DomUtil.java:68: // Standard (includes IE9)
On 2014/05/13 16:20:53, cjhopman wrote:
> We don't care about IE.
> 
> In everything that we care about (except maybe gwt's test modes),
> getComputedStyle is available on the window object, you don't need the
> document.defaultView.
> 
> So, just:
> return getComputedStyle(el, null);
> 
> If required for gwt tests, just fall back to el.style (i.e. get rid of the
case
> for older ie).

Done.

https://codereview.chromium.org/275493007/diff/110001/src/com/dom_distiller/c...
File src/com/dom_distiller/client/FilteringDomVisitor.java (right):

https://codereview.chromium.org/275493007/diff/110001/src/com/dom_distiller/c...
src/com/dom_distiller/client/FilteringDomVisitor.java:39: if
(style.getDisplay().equals("none") || style.getVisibility().equals("hidden")) {
On 2014/05/13 16:20:53, cjhopman wrote:
> how about:
> visible = !(style.getDisplay().equals("none") ||
> style.getVisibility().equals("hidden"));

Done.

[cjhopman, 2014-05-13 17:19:19]

lgtm

[kuan, 2014-05-13 17:21:42]

Committed patchset #8 manually as r0a31800 (presubmit successful).