Bug 41187 - REGRESSION(HTML 5): HTMLDocumentParser does not report html parse errors to the console
: REGRESSION(HTML 5): HTMLDocumentParser does not report html parse errors to t...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: All All
: P1 Major
Assigned To: Adam Barth
:
Depends on: 41060
Blocks: 41115
  Show dependency treegraph
 
Reported: 2010-06-24 17:39 PDT by Eric Seidel
Modified: 2011-02-09 13:54 PST (History)
10 users (show)

See Also:


Attachments
Patch (3.24 KB, patch)
2011-02-07 23:45 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (3.26 KB, patch)
2011-02-09 12:32 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2010-06-24 17:39:07 PDT
HTML 5 Parser does not report html parse errors to the console

This is due to LegacyHTMLTreeBuilder::reportErrorToConsole only working with the LegacyHTMLDocumentParser.
Comment 1 Eric Seidel 2010-08-30 13:54:02 PDT
I think I'll work on this today.
Comment 2 Eric Seidel 2010-08-30 14:56:14 PDT
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.
Comment 3 Adam Barth 2011-02-04 18:24:00 PST
We could test this in a layout test by piping these messages to STDOUT, like we do for console.log.
Comment 4 Yury Semikhatsky 2011-02-06 23:16:33 PST
(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.
Comment 5 Adam Barth 2011-02-06 23:24:18 PST
Remove the reporting didn't require rebaselining any tests, so somewhere there much be a branch...
Comment 6 Yury Semikhatsky 2011-02-06 23:30:53 PST
(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.
Comment 7 Adam Barth 2011-02-07 23:45:34 PST
Created attachment 81603 [details]
Patch
Comment 8 Eric Seidel 2011-02-07 23:47:36 PST
Comment on attachment 81603 [details]
Patch

Wonderful. :)  Now how do we test this?
Comment 9 WebKit Review Bot 2011-02-07 23:48:54 PST
Attachment 81603 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7863002
Comment 10 Adam Barth 2011-02-07 23:51:54 PST
> 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
Comment 11 Eric Seidel 2011-02-07 23:56:39 PST
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.
Comment 12 Adam Barth 2011-02-08 00:03:03 PST
I'm sure it's possible to test by hacking up DumpRenderTree.
Comment 13 WebKit Review Bot 2011-02-08 02:52:23 PST
Attachment 81603 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7829038
Comment 14 Eric Seidel 2011-02-08 15:51:11 PST
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?
Comment 15 Eric Seidel 2011-02-08 15:53:25 PST
Comment on attachment 81603 [details]
Patch

It seems we want to be passing Tokens to the error reporting instead of AtomicHTMLTokens.
Comment 16 Darin Adler 2011-02-08 15:53:38 PST
(In reply to comment #14)
> whoever added the original error messages

Hyatt
Comment 17 Eric Seidel 2011-02-08 16:03:04 PST
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.
Comment 18 Adam Barth 2011-02-09 12:30:33 PST
> 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.
Comment 19 Adam Barth 2011-02-09 12:32:05 PST
Created attachment 81852 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2011-02-09 13:54:16 PST
Comment on attachment 81852 [details]
Patch for landing

Clearing flags on attachment: 81852

Committed r78127: <http://trac.webkit.org/changeset/78127>
Comment 21 WebKit Commit Bot 2011-02-09 13:54:21 PST
All reviewed patches have been landed.  Closing bug.