Bug 42804

Summary: Enable HTML5 tree builder
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, eric, peter, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 41123    
Attachments:
Description Flags
Patch
none
Patch
none
Patch abarth: review+

Description Adam Barth 2010-07-21 21:50:34 PDT
Enabled HTML5 tree builder
Comment 1 Adam Barth 2010-07-21 21:52:18 PDT
Created attachment 62263 [details]
Patch
Comment 2 Adam Barth 2010-07-21 21:55:57 PDT
The diffs to the test expectations are hard to read.  :(
Comment 3 Eric Seidel (no email) 2010-07-22 05:12:14 PDT
Comment on attachment 62263 [details]
Patch

LayoutTests/ChangeLog:8
 +          Update test expectations.
Would be nice to explain the various changes in the ChangeLog.  We certainly hav that data.

LayoutTests/ChangeLog:10
 +          * dom/html/level2/html/HTMLIsIndexElement01-expected.txt:
e.g. these fail because there is no longer ever a parser-created <isindex> element, per HTML5.

LayoutTests/fast/dom/no-elements-expected.txt:1
 +  This tests that the elements noframes, nolayer, noscript and noembed are created as elements and put in the DOM tree. The elements other than <nolayer> should not contain any children. If the test is successful, the four messages below will list 0, 0, 0, and 1 child.
This text needs update, no?

LayoutTests/fast/events/tabindex-focus-blur-all-expected.txt:1
 +  333 focus / 333 blur events dispatched, and should be 337 / 337 PASSED
I don't understand why this is changing?

LayoutTests/fast/forms/datalist-nonoption-child-expected.txt:8
 +  FAIL datalist.firstChild.nodeName should be #text. Was DIV.
And this?

LayoutTests/platform/mac/editing/selection/designmode-no-caret-expected.txt:20
 +  caret: position 0 of child 0 {#text} of body
This is due to text node coalescing?

LayoutTests/platform/mac/fast/events/focusingUnloadedFrame-expected.txt:22
 +          layer at (0,0) size 8x8
This looks like a quirks/non-quirks mode change.  is this intentional?

I think this is great!  But if we want this reviewed, we need to post more explanation in the ChnageLog as to why each test is changing.  Certainly it's difficult for me to review as-is, and probably impossible for anyone else to review with understanding of what's changing.
Comment 4 Adam Barth 2010-07-22 09:48:46 PDT
> Would be nice to explain the various changes in the ChangeLog.  We certainly hav that data.

Ok.

> e.g. these fail because there is no longer ever a parser-created <isindex> element, per HTML5.

Yep.

> LayoutTests/fast/dom/no-elements-expected.txt:1
>  +  This tests that the elements noframes, nolayer, noscript and noembed are created as elements and put in the DOM tree. The elements other than <nolayer> should not contain any children. If the test is successful, the four messages below will list 0, 0, 0, and 1 child.
> This text needs update, no?

Ok.

> LayoutTests/fast/events/tabindex-focus-blur-all-expected.txt:1
>  +  333 focus / 333 blur events dispatched, and should be 337 / 337 PASSED
> I don't understand why this is changing?

I don't quite understand why it's changing either.  I'll spend some time investigating, but this test is really scattershot.  Hopefully there's a reduction.

> LayoutTests/fast/forms/datalist-nonoption-child-expected.txt:8
>  +  FAIL datalist.firstChild.nodeName should be #text. Was DIV.
> And this?

datalist used to have magical custom parsing.

> LayoutTests/platform/mac/editing/selection/designmode-no-caret-expected.txt:20
>  +  caret: position 0 of child 0 {#text} of body
> This is due to text node coalescing?

I believe so.

> LayoutTests/platform/mac/fast/events/focusingUnloadedFrame-expected.txt:22
>  +          layer at (0,0) size 8x8
> This looks like a quirks/non-quirks mode change.  is this intentional?

Oh, I hadn't considered that.  I'll check document.compatMode.

> I think this is great!  But if we want this reviewed, we need to post more explanation in the ChnageLog as to why each test is changing.  Certainly it's difficult for me to review as-is, and probably impossible for anyone else to review with understanding of what's changing.

It's very difficult to understand what's changing.  Maybe we need some more incremental patches that move the old parser to be closer to the new parser?
Comment 5 Adam Barth 2010-08-04 18:17:34 PDT
Created attachment 63524 [details]
Patch
Comment 6 Eric Seidel (no email) 2010-08-04 18:20:31 PDT
Comment on attachment 63524 [details]
Patch

Although I think it's kinda cool to do this w/o results.  I think it would be better to include at least the non-platform specific results.
Comment 7 Eric Seidel (no email) 2010-08-04 19:27:40 PDT
Created attachment 63535 [details]
Patch
Comment 8 Adam Barth 2010-08-04 19:29:56 PDT
Comment on attachment 63535 [details]
Patch

Okiedokes.  Fingers crossed.
Comment 9 WebKit Review Bot 2010-08-04 19:30:27 PDT
Attachment 63535 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Adam Barth 2010-08-04 22:58:04 PDT
Committed r64712: <http://trac.webkit.org/changeset/64712>
Comment 11 Adam Barth 2010-08-04 23:16:19 PDT
Committed r64714: <http://trac.webkit.org/changeset/64714>
Comment 12 WebKit Review Bot 2010-08-04 23:16:57 PDT
http://trac.webkit.org/changeset/64712 might have broken Qt Linux Release