Bug 45649 – Move adjustLexerState to the HTMLTokenizer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Move adjustLexerState to the HTMLTokenizer

[Adam Barth]

Created attachment 67391 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 67391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67391&action=prettypatch

Seems we could remove includes of TreeBuilder.h now, no?

> WebCore/html/parser/HTMLTokenizer.h:142
> +    // Updates the tokenizer's state according to the given tag name.  This is
> +    // an approxmation of how the tree builder would update the tokenizer's
> +    // state.  This method is useful for approximating HTML tokenization.  To
> +    // get exactly the correct tokenization, you need the real tree builder.
> +    void updateState(const AtomicString& tagName, Frame*);
If this is an approximation, can we name it to say such?  approximateStateForTag or something?

[Adam Barth]

Created attachment 67446 [details]
Patch

[Darin Adler]

Comment on attachment 67446 [details]
Patch

> +    // Updates the tokenizer's state according to the given tag name.  This is
> +    // an approxmation of how the tree builder would update the tokenizer's
> +    // state.  This method is useful for approximating HTML tokenization.  To
> +    // get exactly the correct tokenization, you need the real tree builder.

Typo "approxmation"
Two spaces after periods.

I am uncomfortable with the use of the verb "approximate" here. First, it’s easy to confuse the verb with the adjective. And if this was an adjectival form then it would have a return value.

Your comment uses the verb "update" and I think that should be in the function name. Maybe updateStateForFakeElement? Or something else like that? Probably not because the element is not fake.

Another problem with "approximate" is that it doesn’t say what’s wrong. To decide if it’s OK to use the function you need to know how far off “correct” it is, not just that it’s not perfect. It’s not enough to say that it’s not 100% correct; that’s enough to get someone off the hook in a court of law or mathematical proof but is not so great for high-quality software development.

commit-queue- to give you a chance to consider my comments

[Adam Barth]

> Your comment uses the verb "update" and I think that should be in the function name. Maybe updateStateForFakeElement? Or something else like that? Probably not because the element is not fake.

:)

I originally called it "updateState" but Eric asked me to change it.

> Another problem with "approximate" is that it doesn’t say what’s wrong. To decide if it’s OK to use the function you need to know how far off “correct” it is, not just that it’s not perfect. It’s not enough to say that it’s not 100% correct; that’s enough to get someone off the hook in a court of law or mathematical proof but is not so great for high-quality software development.

We might eventually need something fancier here where we run the tree builder in a side-effect free way.  The main thing this function doesn't understand is foreign content.  I'll add more detail to the comment.

[Adam Barth]

Created attachment 67454 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 67454 [details]
Patch for landing

Clearing flags on attachment: 67454

Committed r67467: <http://trac.webkit.org/changeset/67467>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67467 might have broken GTK Linux 32-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/67465
http://trac.webkit.org/changeset/67466
http://trac.webkit.org/changeset/67467
http://trac.webkit.org/changeset/67468