[webkit-reviews] review granted: [Bug 41141] Add a new baseclass for XML, HTML and Text DocumentParsers to clean up DocumentParser call sites : [Attachment 59844] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 41141: Add a new baseclass for XML, HTML and Text DocumentParsers to clean
up DocumentParser call sites
https://bugs.webkit.org/show_bug.cgi?id=41141

Attachment 59844: Patch
https://bugs.webkit.org/attachment.cgi?id=59844&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
I'm not sure about the name "scriptable document parser". What does
"scriptable" mean here? I think it's a bug that scripts don't work in image
documents, for example, so we probably should not be enshrining that bug in our
design or class names. If that's what scriptable means.

> -    if (DocumentParser* parser = m_frame->document()->parser())
> +    ScriptableDocumentParser* parser =
m_frame->document()->scriptableDocumentParser();
> +    if (parser)
>	   return parser->lineNumber() + 1;

Why did you take the definition of the variable out of the if statement? If
you're going to do that, then I suggest changing the style to early return. Or
you could leave the style the way it was.

> +    // FIXME: This is a hack for HTMLFormControlElement::removedFromTree
>      virtual LegacyHTMLTreeBuilder* htmlTreeBuilder() const { return 0; }

I find this comment confusing. Is there some specific sense in which having a
way to downcast the tree builder is a hack? I understand that RTTI is not
elegant, but hack seems a strong word here.

> +    ScriptableDocumentParser(Document* document, bool viewSourceMode =
false);

The argument name "document" could be omitted here. This constructor should be
protected. Can any of the other public members be protected?

> +    // Only used by Document::open for deciding if its safe to act on a
> +    // javscript document.open() call right now, or it should be ignored.
> +    virtual bool isExecutingScript() const { return false; }

I suggest spelling it "JavaScript". Generally speaking I don't think functions
should have comments listing who calls them. Is there some other way you can
say this?

> +};
> +

There's an extra semicolon here after the namespace ends.

> +// FIXME: Why is this different from SVGDocumentExtentions parserLineNumber?


Spelling error: SVGDocumentExtensions.

> +static int parserLineNumber(Document* document)
>  {
> -    DocumentParser* parser = document->parser();
> +    if (document && document->scriptableDocumentParser())
> +	   return document->scriptableDocumentParser()->lineNumber() + 1;
> +    return 0;
> +}

It's not good to call a function that does RTTI like scriptableDocumentParser
twice in a row. Is there a better way to write this? Local variable and early
return perhaps?

> +// FIXME: TextDocumentParser could just be an HTMLDocumentParser
> +// which started the Tokenizer in the PlainText state.

Really? Once you're in the PlainText state you can't get out?

Seems OK. r=me despite the concerns above