[webkit-reviews] review denied: [Bug 40961] REGRESSION (HTML5 Parser): Pages broken due to <tag<tag> parsing changes : [Attachment 67902] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andy Estes <aestes at apple.com>'s
request for review:
Bug 40961: REGRESSION (HTML5 Parser): Pages broken due to <tag<tag> parsing
changes
https://bugs.webkit.org/show_bug.cgi?id=40961

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67902&action=prettypatch

> WebCore/html/parser/HTMLTokenizer.h:122
> -    static PassOwnPtr<HTMLTokenizer> create() { return adoptPtr(new
HTMLTokenizer); }
> +    static PassOwnPtr<HTMLTokenizer> create(Settings* settings) { return
adoptPtr(new HTMLTokenizer(settings)); }

It’s quite inelegant to tie the tokenizer and parser to document settings in
this fashion.

It would be quite a bit better to have the parser and tokenizer publish some
sort of object or flags word that expresses the quirks to be used, then we
could have the code that creates the parser convert the settings into that
form. Having the tokenizer read the settings directly is inelegant and I think
too big a change to the class.

> WebCore/html/parser/HTMLTokenizer.h:268
> +    inline bool usePreHTML5ParserQuirks();

Not sure why the existing code includes these “inline” keywords. They don’t do
any good that I can see.

I don’t think that all pre-HTML5 parser quirks should be grouped in a single
flag. I think we should have an explicit flag for each quirk.