Bug 106375

Created attachment 181753 [details] Patch

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

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?

Created attachment 181767 [details] Patch

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

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

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

Created attachment 181788 [details] Patch

(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 on attachment 181788 [details] Patch Looks great. Thanks!

Comment on attachment 181788 [details] Patch Clearing flags on attachment: 181788 Committed r139141: <http://trac.webkit.org/changeset/139141>

All reviewed patches have been landed. Closing bug.

(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

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