Bug 74658

Summary: <!DOCTYPE html><pre>&#x0a;&#x0a;A</pre> doesn't parse correctly
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, kbr, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Adam Barth
Reported 2011-12-15 15:50:46 PST
<!DOCTYPE html><pre>&#x0a;&#x0a;A</pre> doesn't parse correctly
Attachments
Patch (8.51 KB, patch)
2011-12-15 15:54 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (9.77 KB, patch)
2011-12-15 16:05 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2011-12-15 15:54:10 PST
Created attachment 119511 [details] Patch
Darin Adler
Comment 2 2011-12-15 16:00:43 PST
Comment on attachment 119511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119511&action=review > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:271 > + void skipAtMostOneLeadingNewLine() I thought the term of art was “newline” rather than “new line”. Accordingly, it should be “Newline” rather than “NewLine”. > Source/WebCore/html/parser/HTMLTreeBuilder.h:242 > + bool m_skipLeadingNewLineForListing; I think it would read a little better if it had the word “should” in it, completing the sentence “tree builder <xxx>”. Is the term “for listing” something from the specification? I ask, because it seems strangely specific to one of the three tokens that trigger this.
Darin Adler
Comment 3 2011-12-15 16:01:13 PST
Comment on attachment 119511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119511&action=review > Source/WebCore/ChangeLog:16 > + Test: html5lib/runner.html Is there an expected result for the html5lib parsing test that needs to be updated?
Adam Barth
Comment 4 2011-12-15 16:02:12 PST
(In reply to comment #3) > (From update of attachment 119511 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119511&action=review > > > Source/WebCore/ChangeLog:16 > > + Test: html5lib/runner.html > > Is there an expected result for the html5lib parsing test that needs to be updated? Yes. Sorry, I forgot to --reset-results. I'll upload a new version of the patch that shows the diff.
Adam Barth
Comment 5 2011-12-15 16:05:46 PST
Created attachment 119513 [details] Patch
Adam Barth
Comment 6 2011-12-15 16:10:28 PST
(In reply to comment #2) > (From update of attachment 119511 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119511&action=review > > > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:271 > > + void skipAtMostOneLeadingNewLine() > > I thought the term of art was “newline” rather than “new line”. Accordingly, it should be “Newline” rather than “NewLine”. Fixed. > > Source/WebCore/html/parser/HTMLTreeBuilder.h:242 > > + bool m_skipLeadingNewLineForListing; > > I think it would read a little better if it had the word “should” in it, completing the sentence “tree builder <xxx>”. Fixed. > Is the term “for listing” something from the specification? I ask, because it seems strangely specific to one of the three tokens that trigger this. The specification doesn't have a name for this piece of state. I've removed the "for listing" part of the name. Thanks for the review.
Adam Barth
Comment 7 2011-12-15 16:51:14 PST
Committed r103000: <http://trac.webkit.org/changeset/103000>
Kenneth Russell
Comment 8 2011-12-15 18:44:54 PST
This patch caused ~25 layout test failures, for example: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/18033 Discussed this with abarth on IRC and he stated he knows what the problem is with this patch. Rolling this out. I have a feeling that a subsequent dependent one may also need to be rolled out; we'll see.
Ryosuke Niwa
Comment 9 2011-12-15 19:00:33 PST
It also broke a whole bunch of editing/forms tests on Mac: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r103001%20(35547)/results.html
Kenneth Russell
Comment 10 2011-12-15 19:06:21 PST
Reverted r103000 for reason: Does not handle text/plain documents correctly. Committed r103013: <http://trac.webkit.org/changeset/103013>
Adam Barth
Comment 11 2011-12-16 13:44:57 PST
Committed r103102: <http://trac.webkit.org/changeset/103102>
Note You need to log in before you can comment on or make changes to this bug.