| Summary: | REGRESSION(HTML 5): HTMLDocumentParser does not report html parse errors to the console | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||
| Component: | DOM | Assignee: | 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
Eric Seidel (no email)
2010-06-24 17:39:07 PDT
I think I'll work on this today. 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. We could test this in a layout test by piping these messages to STDOUT, like we do for console.log. (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. Remove the reporting didn't require rebaselining any tests, so somewhere there much be a branch... (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. Created attachment 81603 [details]
Patch
Comment on attachment 81603 [details]
Patch
Wonderful. :) Now how do we test this?
Attachment 81603 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7863002 > 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
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. I'm sure it's possible to test by hacking up DumpRenderTree. Attachment 81603 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7829038 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 on attachment 81603 [details]
Patch
It seems we want to be passing Tokens to the error reporting instead of AtomicHTMLTokens.
(In reply to comment #14) > whoever added the original error messages Hyatt 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. > 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.
Created attachment 81852 [details]
Patch for landing
Comment on attachment 81852 [details] Patch for landing Clearing flags on attachment: 81852 Committed r78127: <http://trac.webkit.org/changeset/78127> All reviewed patches have been landed. Closing bug. |