Bug 106375

Summary: Remove dependency on Document from HTMLConstructionSite::inQuirksMode()
Product: WebKit Reporter: Tony Gentilcore <tonyg>
Component: New BugsAssignee: Tony Gentilcore <tonyg>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 106412    
Bug Blocks: 106127    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Tony Gentilcore 2013-01-08 13:58:41 PST
Remove dependency on Document from HTMLConstructionSite::inQuirksMode()
Comment 1 Tony Gentilcore 2013-01-08 14:00:04 PST
Created attachment 181753 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-01-08 14:08:04 PST
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. :)
Comment 3 Adam Barth 2013-01-08 14:10:43 PST
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?
Comment 4 Tony Gentilcore 2013-01-08 14:26:08 PST
Created attachment 181767 [details]
Patch
Comment 5 Tony Gentilcore 2013-01-08 14:28:36 PST
(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.
Comment 6 Tony Gentilcore 2013-01-08 14:30:01 PST
(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.
Comment 7 Adam Barth 2013-01-08 14:33:46 PST
(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.
Comment 8 Tony Gentilcore 2013-01-08 15:32:34 PST
Created attachment 181788 [details]
Patch
Comment 9 Tony Gentilcore 2013-01-08 15:33:30 PST
(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.
Comment 10 Adam Barth 2013-01-08 15:37:05 PST
Comment on attachment 181788 [details]
Patch

Looks great.  Thanks!
Comment 11 WebKit Review Bot 2013-01-08 17:45:16 PST
Comment on attachment 181788 [details]
Patch

Clearing flags on attachment: 181788

Committed r139141: <http://trac.webkit.org/changeset/139141>
Comment 12 WebKit Review Bot 2013-01-08 17:45:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Dimitri Glazkov (Google) 2013-01-09 08:43:43 PST
(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
Comment 14 Tony Gentilcore 2013-01-09 09:57:17 PST
(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.