Replace htmlparser 1.0.7 with htmlparser 1.2.1

This modifies htmlparser1.2.1 to expose
ElementName and AttributeName.

This means that we're parsing an up-to-date version of HTML5, but the
new parser delays creating text nodes, which leads to bad file
positions.  This should be fixable in later CLs, but is a defect of
the current implementation.


Submitted @3914

[Jasvir, 2009-12-11 02:54:20]

LGTM

http://codereview.appspot.com/165082/diff/1014/21
File
third_party/java/htmlparser/src/nu/validator/htmlparser/impl/AttributeName.java
(right):

http://codereview.appspot.com/165082/diff/1014/21#newcode269
third_party/java/htmlparser/src/nu/validator/htmlparser/impl/AttributeName.java:269:
private static int stringToHash(String s) {
Is speed the reason to clone bufToHash rather than call it with s.toCharArray()?

[johnfargo, 2009-12-11 03:27:35]

Not the most thorough review, since I'm no expert on the underlying impl, but I
did have a few questions on APIs used.

Speaking of the underlying lib, what would you say is the main value we get out
of it right now? Do you use the HTML5-validation pieces, or mostly just the
SAX-ish callback pattern constructing the DOM? Just wondering what the ongoing
ratio is between <lines of code patching up lib oddity> / <lines of code used by
lib> ;)

http://codereview.appspot.com/165082/diff/1014/17
File src/com/google/caja/parser/html/CajaTreeBuilder.java (right):

http://codereview.appspot.com/165082/diff/1014/17#newcode101
src/com/google/caja/parser/html/CajaTreeBuilder.java:101: &&
htmlLocalName.equals(child.getLocalName())) {
important to test case-insensitively?

http://codereview.appspot.com/165082/diff/1014/17#newcode254
src/com/google/caja/parser/html/CajaTreeBuilder.java:254: pos = null;
unnecessary else-block

http://codereview.appspot.com/165082/diff/1014/17#newcode261
src/com/google/caja/parser/html/CajaTreeBuilder.java:261: if
(el.hasAttributeNS(ns, localName)) { continue; }
when would this happen?

http://codereview.appspot.com/165082/diff/1014/17#newcode265
src/com/google/caja/parser/html/CajaTreeBuilder.java:265: if (pos != null) {
&& needsDebugData, presumably

http://codereview.appspot.com/165082/diff/1014/17#newcode430
src/com/google/caja/parser/html/CajaTreeBuilder.java:430: if
(startPos.source().equals(InputSource.UNKNOWN)) {
null check needed?

http://codereview.appspot.com/165082/diff/1014/17#newcode475
src/com/google/caja/parser/html/CajaTreeBuilder.java:475: if
("body".equals(unpopped.getTagName())
ignore case?

http://codereview.appspot.com/165082/diff/1014/17#newcode487
src/com/google/caja/parser/html/CajaTreeBuilder.java:487: &&
HTML_NAMESPACE.equals(unpopped.getNamespaceURI())) {
ignore case?

http://codereview.appspot.com/165082/diff/1014/18
File src/com/google/caja/parser/html/Html5ElementStack.java (right):

http://codereview.appspot.com/165082/diff/1014/18#newcode92
src/com/google/caja/parser/html/Html5ElementStack.java:92:
builder.setFragmentContext(null);
FMI, what's this do?

http://codereview.appspot.com/165082/diff/1014/18#newcode411
src/com/google/caja/parser/html/Html5ElementStack.java:411:
builder.headClosed();
also fmi, what do these calls do? stuff like this always makes me nervous since
I suspect random elements are being synthesized and reordered :)

http://codereview.appspot.com/165082/diff/1014/16
File src/com/google/caja/parser/html/OpenElementStack.java (right):

http://codereview.appspot.com/165082/diff/1014/16#newcode76
src/com/google/caja/parser/html/OpenElementStack.java:76: 
sorry about these...

[MikeSamuel, 2009-12-11 04:35:19]

http://codereview.appspot.com/165082/diff/1014/17
File src/com/google/caja/parser/html/CajaTreeBuilder.java (right):

http://codereview.appspot.com/165082/diff/1014/17#newcode101
src/com/google/caja/parser/html/CajaTreeBuilder.java:101: &&
htmlLocalName.equals(child.getLocalName())) {
On 2009/12/11 03:27:35, johnfargo wrote:
> important to test case-insensitively?

The names should already have been case normalized.  I added a test that would
fail if it were not.

http://codereview.appspot.com/165082/diff/1014/17#newcode254
src/com/google/caja/parser/html/CajaTreeBuilder.java:254: pos = null;
pos would be undefined without this.

http://codereview.appspot.com/165082/diff/1014/17#newcode261
src/com/google/caja/parser/html/CajaTreeBuilder.java:261: if
(el.hasAttributeNS(ns, localName)) { continue; }
Possibly in the case of
  <isindex name="foo">

http://codereview.appspot.com/165082/diff/1014/17#newcode265
src/com/google/caja/parser/html/CajaTreeBuilder.java:265: if (pos != null) {
On 2009/12/11 03:27:35, johnfargo wrote:
> && needsDebugData, presumably

If needsDebugData were false, then pos would be null.

http://codereview.appspot.com/165082/diff/1014/17#newcode430
src/com/google/caja/parser/html/CajaTreeBuilder.java:430: if
(startPos.source().equals(InputSource.UNKNOWN)) {
Nodes.getFilePositionFor never returns null.

http://codereview.appspot.com/165082/diff/1014/17#newcode475
src/com/google/caja/parser/html/CajaTreeBuilder.java:475: if
("body".equals(unpopped.getTagName())
Already normalized.

http://codereview.appspot.com/165082/diff/1014/17#newcode487
src/com/google/caja/parser/html/CajaTreeBuilder.java:487: &&
HTML_NAMESPACE.equals(unpopped.getNamespaceURI())) {
Already normalized.

http://codereview.appspot.com/165082/diff/1014/18
File src/com/google/caja/parser/html/Html5ElementStack.java (right):

http://codereview.appspot.com/165082/diff/1014/18#newcode92
src/com/google/caja/parser/html/Html5ElementStack.java:92:
builder.setFragmentContext(null);
Tells the parser that an element other than html at the top is not an error.

http://codereview.appspot.com/165082/diff/1014/18#newcode411
src/com/google/caja/parser/html/Html5ElementStack.java:411:
builder.headClosed();
Makes sure that head and body are synthesized with the right file position. 
Otherwise they end up with the fileposition of the end of the </html> tag.

Added comment.

http://codereview.appspot.com/165082/diff/1014/21
File
third_party/java/htmlparser/src/nu/validator/htmlparser/impl/AttributeName.java
(right):

http://codereview.appspot.com/165082/diff/1014/21#newcode269
third_party/java/htmlparser/src/nu/validator/htmlparser/impl/AttributeName.java:269:
private static int stringToHash(String s) {
On 2009/12/11 02:54:20, jasvir wrote:
> Is speed the reason to clone bufToHash rather than call it with
s.toCharArray()?

Yes.  Shindig is speed sensitive when it comes to HTML parsing.

[johnfargo, 2009-12-11 04:51:22]

LGTM

Thanks for the quick response.

On Thu, Dec 10, 2009 at 8:35 PM, <mikesamuel@gmail.com> wrote:

>
> http://codereview.appspot.com/165082/diff/1014/17
> File src/com/google/caja/parser/html/CajaTreeBuilder.java (right):
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode101
> src/com/google/caja/parser/html/CajaTreeBuilder.java:101: &&
> htmlLocalName.equals(child.getLocalName())) {
> On 2009/12/11 03:27:35, johnfargo wrote:
>
>> important to test case-insensitively?
>>
>
> The names should already have been case normalized.  I added a test that
> would fail if it were not.
>
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode254
> src/com/google/caja/parser/html/CajaTreeBuilder.java:254: pos = null;
> pos would be undefined without this.
>
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode261
> src/com/google/caja/parser/html/CajaTreeBuilder.java:261: if
> (el.hasAttributeNS(ns, localName)) { continue; }
> Possibly in the case of
>  <isindex name="foo">
>
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode265
> src/com/google/caja/parser/html/CajaTreeBuilder.java:265: if (pos !=
> null) {
> On 2009/12/11 03:27:35, johnfargo wrote:
>
>> && needsDebugData, presumably
>>
>
> If needsDebugData were false, then pos would be null.
>
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode430
> src/com/google/caja/parser/html/CajaTreeBuilder.java:430: if
> (startPos.source().equals(InputSource.UNKNOWN)) {
> Nodes.getFilePositionFor never returns null.
>
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode475
> src/com/google/caja/parser/html/CajaTreeBuilder.java:475: if
> ("body".equals(unpopped.getTagName())
> Already normalized.
>
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode487
> src/com/google/caja/parser/html/CajaTreeBuilder.java:487: &&
> HTML_NAMESPACE.equals(unpopped.getNamespaceURI())) {
> Already normalized.
>
>
> http://codereview.appspot.com/165082/diff/1014/18
> File src/com/google/caja/parser/html/Html5ElementStack.java (right):
>
> http://codereview.appspot.com/165082/diff/1014/18#newcode92
> src/com/google/caja/parser/html/Html5ElementStack.java:92:
> builder.setFragmentContext(null);
> Tells the parser that an element other than html at the top is not an
> error.
>
>
> http://codereview.appspot.com/165082/diff/1014/18#newcode411
> src/com/google/caja/parser/html/Html5ElementStack.java:411:
> builder.headClosed();
> Makes sure that head and body are synthesized with the right file
> position.  Otherwise they end up with the fileposition of the end of the
> </html> tag.
>
> Added comment.
>
>
> http://codereview.appspot.com/165082/diff/1014/21
> File
>
>
third_party/java/htmlparser/src/nu/validator/htmlparser/impl/AttributeName.java
> (right):
>
> http://codereview.appspot.com/165082/diff/1014/21#newcode269
>
>
third_party/java/htmlparser/src/nu/validator/htmlparser/impl/AttributeName.java:269:
> private static int stringToHash(String s) {
> On 2009/12/11 02:54:20, jasvir wrote:
>
>> Is speed the reason to clone bufToHash rather than call it with
>>
> s.toCharArray()?
>
> Yes.  Shindig is speed sensitive when it comes to HTML parsing.
>
>
> http://codereview.appspot.com/165082
>

[MikeSamuel, 2009-12-11 04:59:01]

2009/12/10  <johnfargo@gmail.com>:
> Not the most thorough review, since I'm no expert on the underlying
> impl, but I did have a few questions on APIs used.
>
> Speaking of the underlying lib, what would you say is the main value we
> get out of it right now? Do you use the HTML5-validation pieces, or
> mostly just the SAX-ish callback pattern constructing the DOM? Just
> wondering what the ongoing ratio is between <lines of code patching up
> lib oddity> / <lines of code used by lib> ;)

The underlying lib gets right all the adoption agency algorithm stuff
and the format element stack.  This is not a small portion of the
HTML5 parsing spec.  And by using this library, we can guarantee that
we are compatible with mozilla, which I think is moving to using this
for HTML/SVG/MathML parsing.

I'm going to start a discussion about whether we can get a treebuilder
that publishes events like
    nodeProduced(Node n, int nodeStart, int nodeEnd)
so that we can get everything we want without wrapping -- once it
supports full embedded XML, like <os:*> tags.


> http://codereview.appspot.com/165082/diff/1014/17
> File src/com/google/caja/parser/html/CajaTreeBuilder.java (right):
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode101
> src/com/google/caja/parser/html/CajaTreeBuilder.java:101: &&
> htmlLocalName.equals(child.getLocalName())) {
> important to test case-insensitively?
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode254
> src/com/google/caja/parser/html/CajaTreeBuilder.java:254: pos = null;
> unnecessary else-block
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode261
> src/com/google/caja/parser/html/CajaTreeBuilder.java:261: if
> (el.hasAttributeNS(ns, localName)) { continue; }
> when would this happen?
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode265
> src/com/google/caja/parser/html/CajaTreeBuilder.java:265: if (pos !=
> null) {
> && needsDebugData, presumably
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode430
> src/com/google/caja/parser/html/CajaTreeBuilder.java:430: if
> (startPos.source().equals(InputSource.UNKNOWN)) {
> null check needed?
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode475
> src/com/google/caja/parser/html/CajaTreeBuilder.java:475: if
> ("body".equals(unpopped.getTagName())
> ignore case?
>
> http://codereview.appspot.com/165082/diff/1014/17#newcode487
> src/com/google/caja/parser/html/CajaTreeBuilder.java:487: &&
> HTML_NAMESPACE.equals(unpopped.getNamespaceURI())) {
> ignore case?
>
> http://codereview.appspot.com/165082/diff/1014/18
> File src/com/google/caja/parser/html/Html5ElementStack.java (right):
>
> http://codereview.appspot.com/165082/diff/1014/18#newcode92
> src/com/google/caja/parser/html/Html5ElementStack.java:92:
> builder.setFragmentContext(null);
> FMI, what's this do?
>
> http://codereview.appspot.com/165082/diff/1014/18#newcode411
> src/com/google/caja/parser/html/Html5ElementStack.java:411:
> builder.headClosed();
> also fmi, what do these calls do? stuff like this always makes me
> nervous since I suspect random elements are being synthesized and
> reordered :)
>
> http://codereview.appspot.com/165082/diff/1014/16
> File src/com/google/caja/parser/html/OpenElementStack.java (right):
>
> http://codereview.appspot.com/165082/diff/1014/16#newcode76
> src/com/google/caja/parser/html/OpenElementStack.java:76:
> sorry about these...
>
> http://codereview.appspot.com/165082
>

[johnfargo, 2009-12-11 05:27:57]

Great thx - I wasn't necessarily going one place or another with that line
of discussion, just checking for my own benefit (though tinted by the fact
that I've only really been deep into the patch-up code). The direction
you're suggesting sounds great; feel free to loop me into any discussions if
you find value in doing so.

On Thu, Dec 10, 2009 at 8:59 PM, Mike Samuel <mikesamuel@gmail.com> wrote:

> 2009/12/10  <johnfargo@gmail.com>:
> > Not the most thorough review, since I'm no expert on the underlying
> > impl, but I did have a few questions on APIs used.
> >
> > Speaking of the underlying lib, what would you say is the main value we
> > get out of it right now? Do you use the HTML5-validation pieces, or
> > mostly just the SAX-ish callback pattern constructing the DOM? Just
> > wondering what the ongoing ratio is between <lines of code patching up
> > lib oddity> / <lines of code used by lib> ;)
>
> The underlying lib gets right all the adoption agency algorithm stuff
> and the format element stack.  This is not a small portion of the
> HTML5 parsing spec.  And by using this library, we can guarantee that
> we are compatible with mozilla, which I think is moving to using this
> for HTML/SVG/MathML parsing.
>
> I'm going to start a discussion about whether we can get a treebuilder
> that publishes events like
>    nodeProduced(Node n, int nodeStart, int nodeEnd)
> so that we can get everything we want without wrapping -- once it
> supports full embedded XML, like <os:*> tags.
>
>
> > http://codereview.appspot.com/165082/diff/1014/17
> > File src/com/google/caja/parser/html/CajaTreeBuilder.java (right):
> >
> > http://codereview.appspot.com/165082/diff/1014/17#newcode101
> > src/com/google/caja/parser/html/CajaTreeBuilder.java:101: &&
> > htmlLocalName.equals(child.getLocalName())) {
> > important to test case-insensitively?
> >
> > http://codereview.appspot.com/165082/diff/1014/17#newcode254
> > src/com/google/caja/parser/html/CajaTreeBuilder.java:254: pos = null;
> > unnecessary else-block
> >
> > http://codereview.appspot.com/165082/diff/1014/17#newcode261
> > src/com/google/caja/parser/html/CajaTreeBuilder.java:261: if
> > (el.hasAttributeNS(ns, localName)) { continue; }
> > when would this happen?
> >
> > http://codereview.appspot.com/165082/diff/1014/17#newcode265
> > src/com/google/caja/parser/html/CajaTreeBuilder.java:265: if (pos !=
> > null) {
> > && needsDebugData, presumably
> >
> > http://codereview.appspot.com/165082/diff/1014/17#newcode430
> > src/com/google/caja/parser/html/CajaTreeBuilder.java:430: if
> > (startPos.source().equals(InputSource.UNKNOWN)) {
> > null check needed?
> >
> > http://codereview.appspot.com/165082/diff/1014/17#newcode475
> > src/com/google/caja/parser/html/CajaTreeBuilder.java:475: if
> > ("body".equals(unpopped.getTagName())
> > ignore case?
> >
> > http://codereview.appspot.com/165082/diff/1014/17#newcode487
> > src/com/google/caja/parser/html/CajaTreeBuilder.java:487: &&
> > HTML_NAMESPACE.equals(unpopped.getNamespaceURI())) {
> > ignore case?
> >
> > http://codereview.appspot.com/165082/diff/1014/18
> > File src/com/google/caja/parser/html/Html5ElementStack.java (right):
> >
> > http://codereview.appspot.com/165082/diff/1014/18#newcode92
> > src/com/google/caja/parser/html/Html5ElementStack.java:92:
> > builder.setFragmentContext(null);
> > FMI, what's this do?
> >
> > http://codereview.appspot.com/165082/diff/1014/18#newcode411
> > src/com/google/caja/parser/html/Html5ElementStack.java:411:
> > builder.headClosed();
> > also fmi, what do these calls do? stuff like this always makes me
> > nervous since I suspect random elements are being synthesized and
> > reordered :)
> >
> > http://codereview.appspot.com/165082/diff/1014/16
> > File src/com/google/caja/parser/html/OpenElementStack.java (right):
> >
> > http://codereview.appspot.com/165082/diff/1014/16#newcode76
> > src/com/google/caja/parser/html/OpenElementStack.java:76:
> > sorry about these...
> >
> > http://codereview.appspot.com/165082
> >
>