Bug 41187 – REGRESSION(HTML 5): HTMLDocumentParser does not report html parse errors to the console

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric Seidel (no email)]

HTML 5 Parser does not report html parse errors to the console

This is due to LegacyHTMLTreeBuilder::reportErrorToConsole only working with the LegacyHTMLDocumentParser.

[Eric Seidel (no email)]

I think I'll work on this today.

[Eric Seidel (no email)]

I'm not sure how to test this.  It appears there are no existing tests for this functionality and I'm not sure how to retrieve the HTML parser errors from the console object in a layout test.

[Adam Barth]

We could test this in a layout test by piping these messages to STDOUT, like we do for console.log.

[Yury Semikhatsky]

(In reply to comment #3)
> We could test this in a layout test by piping these messages to STDOUT, like we do for console.log.

Currently JavaScript errors are printed to STDOUT. If we decide to print all parser errors to STDOUT it would require rebaselining tons of html tests to include the error messages I'm not sure that we want to clutter expectations with the parser errors.

[Adam Barth]

Remove the reporting didn't require rebaselining any tests, so somewhere there much be a branch...

[Yury Semikhatsky]

(In reply to comment #5)
> Remove the reporting didn't require rebaselining any tests
Which probably means there were no tests for the parser errors.

[Adam Barth]

Created attachment 81603 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 81603 [details]
Patch

Wonderful. :)  Now how do we test this?

[WebKit Review Bot]

Attachment 81603 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7863002

[Adam Barth]

> Attachment 81603 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/7863002

This bug crops up a lot.  Some header is missing DOMWindow.h

[Eric Seidel (no email)]

The ChromeClient is called back with every error.  So I suspect some code in DumpRenderTree sees all these.

Would be nice to enable certain ones via layoutTestController.  Or better yet, to turn on recording and be able to grab them all later with layoutTestController.consoleMessages() or something.

If it's not possible to test this, you shoudl just say so in the ChangeLog and file a bug about how to fix that.

[Adam Barth]

I'm sure it's possible to test by hacking up DumpRenderTree.

[WebKit Review Bot]

Attachment 81603 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7829038

[Eric Seidel (no email)]

Comment on attachment 81603 [details]
Patch

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

This is obviously very primitive.

I'm OK with approving this patch, but we absolutely need testing.  The whole reason why this support got removed during the re-write is that whoever added the original error messages added *no* testing.  Thus although we knew we were removing it, it was never a critical issue and only ever got really noticed 9 months after its removal!

Please file a bug about testing this and link to it from the ChangeLog.

Looks like this might break Chromium too. Not sure why.

> Source/WebCore/ChangeLog:11
> +        * html/parser/HTMLDocumentParser.h:

Would be nice to mention here what you're doing (moving private to public) since you can't easily see that from the diff.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2810
> +void HTMLTreeBuilder::parseError(AtomicHTMLToken&)

Why not use the line/column information from the Token?

[Eric Seidel (no email)]

Comment on attachment 81603 [details]
Patch

It seems we want to be passing Tokens to the error reporting instead of AtomicHTMLTokens.

[Darin Adler]

(In reply to comment #14)
> whoever added the original error messages

Hyatt

[Eric Seidel (no email)]

Might mention in your message that this is a treebuilder error.  Since we also have HTMLTokenizer::parseError() which could also report errors in this similarly simple manner.

[Adam Barth]

> It seems we want to be passing Tokens to the error reporting instead of AtomicHTMLTokens.

The problem is the synthetic tokens are AtomicHTMLTokens.  Maybe we should cache the current HTMLToken on the tree builder somewhere?  The benefit of using an HTMLToken is that we can get column information and/or source for the token.

I'm going to land this as-is.  We can iterate to improve.

[Adam Barth]

Created attachment 81852 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 81852 [details]
Patch for landing

Clearing flags on attachment: 81852

Committed r78127: <http://trac.webkit.org/changeset/78127>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.