Bug 44788 - Implement HTML5-compliant doctype switching
Summary: Implement HTML5-compliant doctype switching
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-27 12:40 PDT by Dave Hyatt
Modified: 2010-08-28 17:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (74.51 KB, patch)
2010-08-27 13:00 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2010-08-27 12:40:34 PDT
This bug is about bringing our crufty old doctype switching code up to the HTML5 standard.  The forthcoming patch will rename all the modes to match the HTML5 specification and let the HTML5 parsing algorithm actually drive the mode setting.
Comment 1 Dave Hyatt 2010-08-27 13:00:30 PDT
Created attachment 65754 [details]
Patch
Comment 2 WebKit Review Bot 2010-08-27 13:02:59 PDT
Attachment 65754 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderListItem.cpp:167:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/dom/Element.cpp:371:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/dom/Element.cpp:392:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBlock.cpp:4910:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/InlineFlowBox.cpp:925:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 5 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Simon Fraser (smfr) 2010-08-27 13:19:26 PDT
Comment on attachment 65754 [details]
Patch

> +    // Check for Quirks Mode.
> +    if (docType->name() != "html"
> +        || docType->publicId().startsWith("+//Silmaril//dtd html Pro v0r11 19970101//", false)
> +        || docType->publicId().startsWith("-//AdvaSoft Ltd//DTD HTML 3.0 asWedit + extensions//", false)
> +        || docType->publicId().startsWith("-//AS//DTD HTML 3.0 asWedit + extensions//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 2.0 Level 1//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 2.0 Level 2//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 2.0 Strict Level 1//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 2.0 Strict Level 2//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 2.0 Strict//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 2.0//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 2.1E//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 3.0//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 3.2 Final//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 3.2//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML 3//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML Level 0//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML Level 1//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML Level 2//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML Level 3//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML Strict Level 0//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML Strict Level 1//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML Strict Level 2//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML Strict Level 3//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML Strict//", false)
> +        || docType->publicId().startsWith("-//IETF//DTD HTML//", false)
> +        || docType->publicId().startsWith("-//Metrius//DTD Metrius Presentational//", false)
> +        || docType->publicId().startsWith("-//Microsoft//DTD Internet Explorer 2.0 HTML Strict//", false)
> +        || docType->publicId().startsWith("-//Microsoft//DTD Internet Explorer 2.0 HTML//", false)
> +        || docType->publicId().startsWith("-//Microsoft//DTD Internet Explorer 2.0 Tables//", false)
> +        || docType->publicId().startsWith("-//Microsoft//DTD Internet Explorer 3.0 HTML Strict//", false)
> +        || docType->publicId().startsWith("-//Microsoft//DTD Internet Explorer 3.0 HTML//", false)
> +        || docType->publicId().startsWith("-//Microsoft//DTD Internet Explorer 3.0 Tables//", false)
> +        || docType->publicId().startsWith("-//Netscape Comm. Corp.//DTD HTML//", false)
> +        || docType->publicId().startsWith("-//Netscape Comm. Corp.//DTD Strict HTML//", false)
> +        || docType->publicId().startsWith("-//O'Reilly and Associates//DTD HTML 2.0//", false)
> +        || docType->publicId().startsWith("-//O'Reilly and Associates//DTD HTML Extended 1.0//", false)
> +        || docType->publicId().startsWith("-//O'Reilly and Associates//DTD HTML Extended Relaxed 1.0//", false)
> +        || docType->publicId().startsWith("-//SoftQuad Software//DTD HoTMetaL PRO 6.0::19990601::extensions to HTML 4.0//", false)
> +        || docType->publicId().startsWith("-//SoftQuad//DTD HoTMetaL PRO 4.0::19971010::extensions to HTML 4.0//", false)
> +        || docType->publicId().startsWith("-//Spyglass//DTD HTML 2.0 Extended//", false)
> +        || docType->publicId().startsWith("-//SQ//DTD HTML 2.0 HoTMetaL + extensions//", false)
> +        || docType->publicId().startsWith("-//Sun Microsystems Corp.//DTD HotJava HTML//", false)
> +        || docType->publicId().startsWith("-//Sun Microsystems Corp.//DTD HotJava Strict HTML//", false)
> +        || docType->publicId().startsWith("-//W3C//DTD HTML 3 1995-03-24//", false)
> +        || docType->publicId().startsWith("-//W3C//DTD HTML 3.2 Draft//", false)
> +        || docType->publicId().startsWith("-//W3C//DTD HTML 3.2 Final//", false)
> +        || docType->publicId().startsWith("-//W3C//DTD HTML 3.2//", false)
> +        || docType->publicId().startsWith("-//W3C//DTD HTML 3.2S Draft//", false)
> +        || docType->publicId().startsWith("-//W3C//DTD HTML 4.0 Frameset//", false)
> +        || docType->publicId().startsWith("-//W3C//DTD HTML 4.0 Transitional//", false)
> +        || docType->publicId().startsWith("-//W3C//DTD HTML Experimental 19960712//", false)
> +        || docType->publicId().startsWith("-//W3C//DTD HTML Experimental 970421//", false)
> +        || docType->publicId().startsWith("-//W3C//DTD W3 HTML//", false)
> +        || docType->publicId().startsWith("-//W3O//DTD W3 HTML 3.0//", false)
> +        || equalIgnoringCase(docType->publicId(), "-//W3O//DTD W3 HTML Strict 3.0//EN//")
> +        || docType->publicId().startsWith("-//WebTechs//DTD Mozilla HTML 2.0//", false)
> +        || docType->publicId().startsWith("-//WebTechs//DTD Mozilla HTML//", false)
> +        || equalIgnoringCase(docType->publicId(), "-/W3C/DTD HTML 4.0 Transitional/EN")
> +        || equalIgnoringCase(docType->publicId(), "HTML")
> +        || equalIgnoringCase(docType->systemId(), "http://www.ibm.com/data/dtd/v11/ibmxhtml1-transitional.dtd")
> +        || (docType->systemId().isEmpty() && docType->publicId().startsWith("-//W3C//DTD HTML 4.01 Frameset//", false))
> +        || (docType->systemId().isEmpty() && docType->publicId().startsWith("-//W3C//DTD HTML 4.01 Transitional//", false))) {
> +        setCompatibilityMode(QuirksMode);
> +        return;

Ick.Can we at least pull docType->publicId() into a local variable?
Comment 4 Dave Hyatt 2010-08-27 13:29:47 PDT
Fixed in r66247.
Comment 5 Darin Adler 2010-08-27 13:55:53 PDT
Comment on attachment 65754 [details]
Patch

> @@ -1394,6 +1404,7 @@ void Document::unscheduleStyleRecalc()
>          documentsThatNeedStyleRecalc->remove(this);
>  
>      m_styleRecalcTimer.stop();
> +    m_pendingStyleRecalcShouldForce = false;
>  }
>  
>  void Document::styleRecalcTimerFired(Timer<Document>*)
> @@ -1431,6 +1442,9 @@ void Document::recalcStyle(StyleChange c
>      ASSERT(!renderer() || renderArena());
>      if (!renderer() || !renderArena())
>          goto bail_out;
> +    
> +    if (m_pendingStyleRecalcShouldForce)
> +        change = Force;
>  
>      if (change == Force) {
>          // style selector may set this again during recalc
> @@ -1488,9 +1502,7 @@ void Document::updateStyleIfNeeded()
>      if (m_frame)
>          m_frame->animation()->beginAnimationUpdate();
>          
> -    recalcStyle(m_pendingStyleRecalcShouldForce ? Force : NoChange);
> -    
> -    m_pendingStyleRecalcShouldForce = false;
> +    recalcStyle(NoChange);
>  
>      // Tell the animation controller that updateStyleIfNeeded is finished and it can do any post-processing
>      if (m_frame)

These changes seem good, but separate from the doctype changes. Can you land them separately? Can you test them?
Comment 6 WebKit Review Bot 2010-08-27 14:19:39 PDT
http://trac.webkit.org/changeset/66247 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/66248
http://trac.webkit.org/changeset/66246
http://trac.webkit.org/changeset/66247
Comment 7 Eric Seidel (no email) 2010-08-27 14:22:21 PDT
So this was landed I take it?

Very strange that this all happened "in secret" w/o any r+ on bugzilla, or the original authors of this code CC'd. :p
Comment 8 Eric Seidel (no email) 2010-08-27 14:23:17 PDT
By "this code" I meant, the parser, which was an overstatement, since most of this code doesn't affect the parser. :)

Regardless, thanks for fixing.  Still seemed to happen kinda strangely.
Comment 9 Adam Barth 2010-08-27 14:26:59 PDT
It's unclear to me why HTMLDocument::setCompatibilityModeFromDoctype() part of HTMLDocument instead of part of HTMLTreeBuilder, which is where this logic is in the spec...
Comment 10 Eric Seidel (no email) 2010-08-27 14:30:49 PDT
Comment on attachment 65754 [details]
Patch

WebCore/dom/Document.cpp:592
 +          // All user stylesheets have to reparse using the different mode.
You mentioned that the renderers have to update too (over irc).  Or is that no longer the case?

WebCore/dom/Document.h:586
 +      bool inNoQuirksMode() const { return m_compatibilityMode == NoQuirksMode; }
The new names are much better. :)

WebCore/html/HTMLDocument.cpp:359
 +          || docType->publicId().startsWith("-//AdvaSoft Ltd//DTD HTML 3.0 asWedit + extensions//", false)
I agree with simon. :)  This seems to want a helper function. :)

WebCore/html/HTMLTreeBuilder.cpp:434
 +      m_dummyDocumentForFragmentParsing->setCompatibilityMode(fragment->document()->compatibilityMode());
Do we have tests for this?

I retract my earlier comments.  I lept to judgement.

Fantastic patch.  Thanks again.
Comment 11 Tony Chang 2010-08-27 14:35:08 PDT
I think these tests started to fail after http://trac.webkit.org/changeset/66247:
  tables/mozilla_expected_failures/bugs/bug56024.html
  fast/multicol/span/span-as-nested-columns-child.html
Comment 12 Darin Adler 2010-08-28 15:56:17 PDT
(In reply to comment #11)
> I think these tests started to fail after http://trac.webkit.org/changeset/66247:
>   tables/mozilla_expected_failures/bugs/bug56024.html
>   fast/multicol/span/span-as-nested-columns-child.html

Hyatt, will you be fixing this? Do we need a bug report?
Comment 13 Ryosuke Niwa 2010-08-28 17:17:43 PDT
(In reply to comment #11)
> I think these tests started to fail after http://trac.webkit.org/changeset/66247:
>   tables/mozilla_expected_failures/bugs/bug56024.html
>   fast/multicol/span/span-as-nested-columns-child.html

I believe these tests no longer fails after rebaselined in http://trac.webkit.org/changeset/66274 and http://trac.webkit.org/changeset/66278.  The rational for rebaseline being their getting the old beahvior back after rebaselines in http://trac.webkit.org/changeset/66154 and http://trac.webkit.org/changeset/66147.