Start using computed style instead of default tag actions.

Start using computed style instead of default tag actions.

Currently, actions are taken based on the tag only. This CL changes it so that
the computed style is also taken into account to ensure there is no inlining
of styles that have display:block.

BUG=375553
R=yfriedman@chromium.org

Committed: f6e5bbb

[nyquist, 2014-05-28 23:51:33]

yfriedman: PTAL

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
File boilerpipe-core/src/main/de/l3s/boilerpipe/sax/DefaultTagActionMap.java
(left):

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/DefaultTagActionMap.java:69:
setTagAction("ABBR", CommonTagActions.TA_INLINE_WHITESPACE);
These two are now treated as TA_INLINE_NO_WHITESPACE.

[Yaron, 2014-05-29 01:09:10]

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
File
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/BoilerpipeHTMLContentHandler.java
(right):

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/BoilerpipeHTMLContentHandler.java:89:
* Contains the computed style of each element. The element key is a weak
reference, to ensure
Is this true? I don't see that

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/BoilerpipeHTMLContentHandler.java:144:
displayStyleToTagAction.put("inline", CommonTagActions.TA_INLINE_NO_WHITESPACE);
Can you reference a source for these?

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/BoilerpipeHTMLContentHandler.java:147:
displayStyleToTagAction.put("block", CommonTagActions.TA_BLOCK_LEVEL);
nit: dup

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/BoilerpipeHTMLContentHandler.java:160:
displayStyleToTagAction.put("list-item", CommonTagActions.TA_BLOCK_LEVEL);
So display:none is handled elsewhere... Logically this could be merged with that
but I guess it depends on removal of BoilerpipeHTMLContentHandler so not worth
doing now.

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
File boilerpipe-core/src/main/de/l3s/boilerpipe/sax/DefaultTagActionMap.java
(left):

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/DefaultTagActionMap.java:69:
setTagAction("ABBR", CommonTagActions.TA_INLINE_WHITESPACE);
On 2014/05/28 23:51:33, nyquist wrote:
> These two are now treated as TA_INLINE_NO_WHITESPACE.

Seems correct

https://codereview.chromium.org/296113004/diff/30001/src/com/dom_distiller/cl...
File src/com/dom_distiller/client/DomToSaxVisitor.java (right):

https://codereview.chromium.org/296113004/diff/30001/src/com/dom_distiller/cl...
src/com/dom_distiller/client/DomToSaxVisitor.java:61:
handler.endElement(sHtmlNamespace, e.getTagName(), e.getTagName(), e);
Why bother continuing to pass getTagName? Can you just pass e and clean up this
interface since we're diverging?

https://codereview.chromium.org/296113004/diff/30001/src/com/dom_distiller/cl...
File src/com/dom_distiller/client/sax/ContentHandler.java (right):

https://codereview.chromium.org/296113004/diff/30001/src/com/dom_distiller/cl...
src/com/dom_distiller/client/sax/ContentHandler.java:13: public void
startElement(
Same as above. Let's clean up the interface here as we start moving towards
traversing a DOM

https://codereview.chromium.org/296113004/diff/30001/test/com/dom_distiller/c...
File test/com/dom_distiller/client/BoilerpipeHTMLContentHandlerTest.java
(right):

https://codereview.chromium.org/296113004/diff/30001/test/com/dom_distiller/c...
test/com/dom_distiller/client/BoilerpipeHTMLContentHandlerTest.java:27: 
Do you want to add tests for the default actions for the tags removed from
DefaultTagActionMap or does that seem like overkill?

[nyquist, 2014-05-29 23:42:25]

yfriedman: PTAL

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
File
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/BoilerpipeHTMLContentHandler.java
(right):

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/BoilerpipeHTMLContentHandler.java:89:
* Contains the computed style of each element. The element key is a weak
reference, to ensure
On 2014/05/29 01:09:10, Yaron wrote:
> Is this true? I don't see that

I'm a liar who does not update comments after realizing GWT does not have
WeakHashMap.
Done.

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/BoilerpipeHTMLContentHandler.java:144:
displayStyleToTagAction.put("inline", CommonTagActions.TA_INLINE_NO_WHITESPACE);
On 2014/05/29 01:09:10, Yaron wrote:
> Can you reference a source for these?

Done. Also reordered and clarified with multiple sources for easy access.

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/BoilerpipeHTMLContentHandler.java:147:
displayStyleToTagAction.put("block", CommonTagActions.TA_BLOCK_LEVEL);
On 2014/05/29 01:09:10, Yaron wrote:
> nit: dup

Done.

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/BoilerpipeHTMLContentHandler.java:160:
displayStyleToTagAction.put("list-item", CommonTagActions.TA_BLOCK_LEVEL);
On 2014/05/29 01:09:10, Yaron wrote:
> So display:none is handled elsewhere... Logically this could be merged with
that
> but I guess it depends on removal of BoilerpipeHTMLContentHandler so not worth
> doing now.

Added TODO.

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
File boilerpipe-core/src/main/de/l3s/boilerpipe/sax/DefaultTagActionMap.java
(left):

https://codereview.chromium.org/296113004/diff/30001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/DefaultTagActionMap.java:69:
setTagAction("ABBR", CommonTagActions.TA_INLINE_WHITESPACE);
On 2014/05/29 01:09:10, Yaron wrote:
> On 2014/05/28 23:51:33, nyquist wrote:
> > These two are now treated as TA_INLINE_NO_WHITESPACE.
> 
> Seems correct

Also removed the now unused TagAction for this.

https://codereview.chromium.org/296113004/diff/30001/src/com/dom_distiller/cl...
File src/com/dom_distiller/client/DomToSaxVisitor.java (right):

https://codereview.chromium.org/296113004/diff/30001/src/com/dom_distiller/cl...
src/com/dom_distiller/client/DomToSaxVisitor.java:61:
handler.endElement(sHtmlNamespace, e.getTagName(), e.getTagName(), e);
On 2014/05/29 01:09:10, Yaron wrote:
> Why bother continuing to pass getTagName? Can you just pass e and clean up
this
> interface since we're diverging?

Done. Also cleaned up Attributes.

https://codereview.chromium.org/296113004/diff/30001/src/com/dom_distiller/cl...
File src/com/dom_distiller/client/sax/ContentHandler.java (right):

https://codereview.chromium.org/296113004/diff/30001/src/com/dom_distiller/cl...
src/com/dom_distiller/client/sax/ContentHandler.java:13: public void
startElement(
On 2014/05/29 01:09:10, Yaron wrote:
> Same as above. Let's clean up the interface here as we start moving towards
> traversing a DOM

Done.

https://codereview.chromium.org/296113004/diff/30001/test/com/dom_distiller/c...
File test/com/dom_distiller/client/BoilerpipeHTMLContentHandlerTest.java
(right):

https://codereview.chromium.org/296113004/diff/30001/test/com/dom_distiller/c...
test/com/dom_distiller/client/BoilerpipeHTMLContentHandlerTest.java:27: 
On 2014/05/29 01:09:10, Yaron wrote:
> Do you want to add tests for the default actions for the tags removed from
> DefaultTagActionMap or does that seem like overkill?

It seems like GWT doesn't handle the defaults correctly, even for the typed ones
such as textarea:
http://www.gwtproject.org/javadoc/latest/com/google/gwt/user/client/DOM.html
Except <span> they all seem to be treated as block level elements.

It does not seem to parse inline / text markup correctly. See
https://developer.mozilla.org/en-US/docs/Web/HTML/Inline_elemente and
http://www.w3.org/TR/html4/sgml/dtd.html#inline (and the section on Text Markup
in general).

[Yaron, 2014-05-30 01:23:28]

Let's try scoring this one and see what the result is.

https://codereview.chromium.org/296113004/diff/50001/boilerpipe-core/src/main...
File boilerpipe-core/src/main/de/l3s/boilerpipe/sax/CommonTagActions.java
(right):

https://codereview.chromium.org/296113004/diff/50001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/CommonTagActions.java:50: return
t1.start(instance, atts)
Nit; these look like they can be oneline (same in |end|)

[nyquist, 2014-05-30 17:08:00]

yfriedman: PTAL

https://codereview.chromium.org/296113004/diff/50001/boilerpipe-core/src/main...
File boilerpipe-core/src/main/de/l3s/boilerpipe/sax/CommonTagActions.java
(right):

https://codereview.chromium.org/296113004/diff/50001/boilerpipe-core/src/main...
boilerpipe-core/src/main/de/l3s/boilerpipe/sax/CommonTagActions.java:50: return
t1.start(instance, atts)
On 2014/05/30 01:23:29, Yaron wrote:
> Nit; these look like they can be oneline (same in |end|)

Done.

[Yaron, 2014-05-30 17:31:19]

On 2014/05/30 17:08:00, nyquist wrote:
> yfriedman: PTAL
> 
>
https://codereview.chromium.org/296113004/diff/50001/boilerpipe-core/src/main...
> File boilerpipe-core/src/main/de/l3s/boilerpipe/sax/CommonTagActions.java
> (right):
> 
>
https://codereview.chromium.org/296113004/diff/50001/boilerpipe-core/src/main...
> boilerpipe-core/src/main/de/l3s/boilerpipe/sax/CommonTagActions.java:50:
return
> t1.start(instance, atts)
> On 2014/05/30 01:23:29, Yaron wrote:
> > Nit; these look like they can be oneline (same in |end|)
> 
> Done.

oops, meant to lgtm

[Yaron, 2014-05-30 17:31:20]

On 2014/05/30 17:08:00, nyquist wrote:
> yfriedman: PTAL
> 
>
https://codereview.chromium.org/296113004/diff/50001/boilerpipe-core/src/main...
> File boilerpipe-core/src/main/de/l3s/boilerpipe/sax/CommonTagActions.java
> (right):
> 
>
https://codereview.chromium.org/296113004/diff/50001/boilerpipe-core/src/main...
> boilerpipe-core/src/main/de/l3s/boilerpipe/sax/CommonTagActions.java:50:
return
> t1.start(instance, atts)
> On 2014/05/30 01:23:29, Yaron wrote:
> > Nit; these look like they can be oneline (same in |end|)
> 
> Done.

oops, meant to lgtm

[nyquist, 2014-05-30 21:34:51]

Committed patchset #5 manually as rf6e5bbb (presubmit successful).