Bug 106375 – Remove dependency on Document from HTMLConstructionSite::inQuirksMode()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Gentilcore]

Remove dependency on Document from HTMLConstructionSite::inQuirksMode()

[Tony Gentilcore]

Created attachment 181753 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 181753 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181753&action=review

I believe this to be pretty well tested, so I'm confident that if the EWS like it, you're golden.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:147
> +    , m_inQuirksMode(document->inQuirksMode())

This looks right.  i assume this will almost always be NoQuirksMode, since that's what documents start in.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:158
> +    , m_inQuirksMode(fragment->document()->inQuirksMode())

It's not immediately clear to me that this is what the old code did.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:334
> +    return;

Unneeded. :)

[Adam Barth]

View in context: https://bugs.webkit.org/attachment.cgi?id=181753&action=review

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:245
> +void HTMLConstructionSite::setCompatibilityModeFromDoctype(DocumentType* docType)

Previously setCompatibilityModeFromDoctype was a virtual function that did different things for HTMLDocument and Document.  Have we lost that distinction?  I guess if we're using the HTMLDocumentParser, then the m_document must be an HTMLDocument...  Should we add an ASSERT to that effect somewhere?

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:362
>          // We need to actually add the Doctype node to the DOM.
>          executeQueuedTasks();

Can we remove this executeQueuedTasks call?  Previously we needed it so that m_document->setCompatibilityModeFromDoctype() would work.  (Please fell free to remove it in a followup patch.)

> Source/WebCore/html/parser/HTMLConstructionSite.h:84
> +    void setCompatibilityMode(Document::CompatibilityMode);
> +    void setCompatibilityModeFromDoctype(DocumentType*);

Can these be private?

[Tony Gentilcore]

Created attachment 181767 [details]
Patch

[Tony Gentilcore]

(In reply to comment #2)
> (From update of attachment 181753 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181753&action=review
> 
> I believe this to be pretty well tested, so I'm confident that if the EWS like it, you're golden.
> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:147
> > +    , m_inQuirksMode(document->inQuirksMode())
> 
> This looks right.  i assume this will almost always be NoQuirksMode, since that's what documents start in.

Yep.

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:158
> > +    , m_inQuirksMode(fragment->document()->inQuirksMode())
> 
> It's not immediately clear to me that this is what the old code did.

fast/parser/fragment-parser-doctype.html fails without this.

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:334
> > +    return;
> 
> Unneeded. :)

That was a copy/paste. Removed.

[Tony Gentilcore]

(In reply to comment #3)
> View in context: https://bugs.webkit.org/attachment.cgi?id=181753&action=review
> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:245
> > +void HTMLConstructionSite::setCompatibilityModeFromDoctype(DocumentType* docType)
> 
> Previously setCompatibilityModeFromDoctype was a virtual function that did different things for HTMLDocument and Document.  Have we lost that distinction?  I guess if we're using the HTMLDocumentParser, then the m_document must be an HTMLDocument...  Should we add an ASSERT to that effect somewhere?

You are right, if we are using an HTMLDocumentParser, we have an HTMLDocument. I'm not quite sure where to ASSERT anything to that end though.

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:362
> >          // We need to actually add the Doctype node to the DOM.
> >          executeQueuedTasks();
> 
> Can we remove this executeQueuedTasks call?  Previously we needed it so that m_document->setCompatibilityModeFromDoctype() would work.  (Please fell free to remove it in a followup patch.)

I don't think we can remove it since we are still calling m_document->doctype() on the line below. Am I missing something?

> 
> > Source/WebCore/html/parser/HTMLConstructionSite.h:84
> > +    void setCompatibilityMode(Document::CompatibilityMode);
> > +    void setCompatibilityModeFromDoctype(DocumentType*);
> 
> Can these be private?

Oops. Fixed.

[Adam Barth]

(In reply to comment #6)
> (In reply to comment #3)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181753&action=review
> > 
> > > Source/WebCore/html/parser/HTMLConstructionSite.cpp:245
> > > +void HTMLConstructionSite::setCompatibilityModeFromDoctype(DocumentType* docType)
> > 
> > Previously setCompatibilityModeFromDoctype was a virtual function that did different things for HTMLDocument and Document.  Have we lost that distinction?  I guess if we're using the HTMLDocumentParser, then the m_document must be an HTMLDocument...  Should we add an ASSERT to that effect somewhere?
> 
> You are right, if we are using an HTMLDocumentParser, we have an HTMLDocument. I'm not quite sure where to ASSERT anything to that end though.

Maybe in the constructor?

> > > Source/WebCore/html/parser/HTMLConstructionSite.cpp:362
> > >          // We need to actually add the Doctype node to the DOM.
> > >          executeQueuedTasks();
> > 
> > Can we remove this executeQueuedTasks call?  Previously we needed it so that m_document->setCompatibilityModeFromDoctype() would work.  (Please fell free to remove it in a followup patch.)
> 
> I don't think we can remove it since we are still calling m_document->doctype() on the line below. Am I missing something?

We could make setCompatibilityModeFromDoctype take an AtomicHTMLToken rather than a Doctype*.  We can worry about that in a followup patch, however.  The issue is that we won't be able to have a Doctype* on the parser thread.

[Tony Gentilcore]

Created attachment 181788 [details]
Patch

[Tony Gentilcore]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #3)
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=181753&action=review
> > > 
> > > > Source/WebCore/html/parser/HTMLConstructionSite.cpp:245
> > > > +void HTMLConstructionSite::setCompatibilityModeFromDoctype(DocumentType* docType)
> > > 
> > > Previously setCompatibilityModeFromDoctype was a virtual function that did different things for HTMLDocument and Document.  Have we lost that distinction?  I guess if we're using the HTMLDocumentParser, then the m_document must be an HTMLDocument...  Should we add an ASSERT to that effect somewhere?
> > 
> > You are right, if we are using an HTMLDocumentParser, we have an HTMLDocument. I'm not quite sure where to ASSERT anything to that end though.
> 
> Maybe in the constructor?

Done.

> 
> > > > Source/WebCore/html/parser/HTMLConstructionSite.cpp:362
> > > >          // We need to actually add the Doctype node to the DOM.
> > > >          executeQueuedTasks();
> > > 
> > > Can we remove this executeQueuedTasks call?  Previously we needed it so that m_document->setCompatibilityModeFromDoctype() would work.  (Please fell free to remove it in a followup patch.)
> > 
> > I don't think we can remove it since we are still calling m_document->doctype() on the line below. Am I missing something?
> 
> We could make setCompatibilityModeFromDoctype take an AtomicHTMLToken rather than a Doctype*.  We can worry about that in a followup patch, however.  The issue is that we won't be able to have a Doctype* on the parser thread.

Done.

[Adam Barth]

Comment on attachment 181788 [details]
Patch

Looks great.  Thanks!

[WebKit Review Bot]

Comment on attachment 181788 [details]
Patch

Clearing flags on attachment: 181788

Committed r139141: <http://trac.webkit.org/changeset/139141>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

(In reply to comment #12)
> All reviewed patches have been landed.  Closing bug.

I think this change made a test assert:


http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2FexecCommand%2Finsert-list-xml.xhtml

[Tony Gentilcore]

(In reply to comment #13)
> (In reply to comment #12)
> > All reviewed patches have been landed.  Closing bug.
> 
> I think this change made a test assert:
> 
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2FexecCommand%2Finsert-list-xml.xhtml

This assertion failure is tracked by https://bugs.webkit.org/show_bug.cgi?id=106412 and there's a patch up there to fix it. So re-closing this bug out.