Bug 44788 – Implement HTML5-compliant doctype switching

• Component: DOM
• Status: RESOLVED FIXED

[Dave Hyatt]

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.

[Dave Hyatt]

Created attachment 65754 [details]
Patch

[WebKit Review Bot]

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.

[Simon Fraser (smfr)]

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?

[Dave Hyatt]

Fixed in r66247.

[Darin Adler]

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?

[WebKit Review Bot]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Adam Barth]

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...

[Eric Seidel (no email)]

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.

[Tony Chang]

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

[Darin Adler]

(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?

[Ryosuke Niwa]

(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.