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
: WebKit
HTML DOM
: 528+ (Nightly build)
: All All
: P1 Major
Assigned To:
:
:
: 41060
: 41115
  Show dependency treegraph
 
Reported: 2010-06-24 17:39 PST by
Modified: 2011-02-09 13:54 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-06-24 17:39:07 PST
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 From 2010-08-30 13:54:02 PST -------
I think I'll work on this today.
------- Comment #2 From 2010-08-30 14:56:14 PST -------
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 From 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 From 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 From 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 From 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 From 2011-02-07 23:45:34 PST -------
Created an attachment (id=81603) [details]
Patch
------- Comment #8 From 2011-02-07 23:47:36 PST -------
(From update of attachment 81603 [details])
Wonderful. :)  Now how do we test this?
------- Comment #9 From 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 From 2011-02-07 23:51:54 PST -------
> Attachment 81603 [details] [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 From 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 From 2011-02-08 00:03:03 PST -------
I'm sure it's possible to test by hacking up DumpRenderTree.
------- Comment #13 From 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 From 2011-02-08 15:51:11 PST -------
(From update of attachment 81603 [details])
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 From 2011-02-08 15:53:25 PST -------
(From update of attachment 81603 [details])
It seems we want to be passing Tokens to the error reporting instead of AtomicHTMLTokens.
------- Comment #16 From 2011-02-08 15:53:38 PST -------
(In reply to comment #14)
> whoever added the original error messages

Hyatt
------- Comment #17 From 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 From 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 From 2011-02-09 12:32:05 PST -------
Created an attachment (id=81852) [details]
Patch for landing
------- Comment #20 From 2011-02-09 13:54:16 PST -------
(From update of attachment 81852 [details])
Clearing flags on attachment: 81852

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