Bug 41187

Summary: REGRESSION(HTML 5): HTMLDocumentParser does not report html parse errors to the console
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: DOMAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Major CC: abarth, ap, commit-queue, darin, dglazkov, hyatt, pfeldman, tonyg, webkit.review.bot, yurys
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 41060    
Bug Blocks: 41115    
Attachments:
Description Flags
Patch
none
Patch for landing none

Eric Seidel (no email)
Reported 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.
Attachments
Patch (3.24 KB, patch)
2011-02-07 23:45 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch for landing (3.26 KB, patch)
2011-02-09 12:32 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2010-08-30 13:54:02 PDT
I think I'll work on this today.
Eric Seidel (no email)
Comment 2 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.
Adam Barth
Comment 3 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.
Yury Semikhatsky
Comment 4 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.
Adam Barth
Comment 5 2011-02-06 23:24:18 PST
Remove the reporting didn't require rebaselining any tests, so somewhere there much be a branch...
Yury Semikhatsky
Comment 6 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.
Adam Barth
Comment 7 2011-02-07 23:45:34 PST
Created attachment 81603 [details] Patch
Eric Seidel (no email)
Comment 8 2011-02-07 23:47:36 PST
Comment on attachment 81603 [details] Patch Wonderful. :) Now how do we test this?
WebKit Review Bot
Comment 9 2011-02-07 23:48:54 PST
Attachment 81603 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7863002
Adam Barth
Comment 10 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
Eric Seidel (no email)
Comment 11 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.
Adam Barth
Comment 12 2011-02-08 00:03:03 PST
I'm sure it's possible to test by hacking up DumpRenderTree.
WebKit Review Bot
Comment 13 2011-02-08 02:52:23 PST
Attachment 81603 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7829038
Eric Seidel (no email)
Comment 14 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?
Eric Seidel (no email)
Comment 15 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.
Darin Adler
Comment 16 2011-02-08 15:53:38 PST
(In reply to comment #14) > whoever added the original error messages Hyatt
Eric Seidel (no email)
Comment 17 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.
Adam Barth
Comment 18 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.
Adam Barth
Comment 19 2011-02-09 12:32:05 PST
Created attachment 81852 [details] Patch for landing
WebKit Commit Bot
Comment 20 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>
WebKit Commit Bot
Comment 21 2011-02-09 13:54:21 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.