Teach HTML5TreeBuilder how to merge attributes from extra html/body elements
Created attachment 59994 [details] Patch
Comment on attachment 59994 [details] Patch WebCore/html/HTMLTreeBuilder.h:142 + m_htmlElement = static_cast<HTMLHtmlElement*>(element.get()); Let's take out these static casts. WebCore/html/HTMLTreeBuilder.h:212 + // keeps them alive. They are never popped from the stack. Can you ASSERT this during pop?
(In reply to comment #2) > (From update of attachment 59994 [details]) > WebCore/html/HTMLTreeBuilder.h:142 > + m_htmlElement = static_cast<HTMLHtmlElement*>(element.get()); > Let's take out these static casts. No. :) As stated in person, the type safey is useful. I added back the ASSERTs. > WebCore/html/HTMLTreeBuilder.h:212 > + // keeps them alive. They are never popped from the stack. > Can you ASSERT this during pop? Yes. Done.
> No. :) As stated in person, the type safey is useful. I added back the ASSERTs. Only Eric would invoke "type safety" as a reason to use static_cast! :)
Created attachment 59995 [details] this doesn't work
Created attachment 59997 [details] Patch
Comment on attachment 59997 [details] Patch Nice test progression. Here are a bunch of comments, mostly about naming and asserts. I'd like to see this one more time. You might have written it while tired. :) WebCore/html/HTMLTreeBuilder.cpp:286 + m_openElements.pushHtml(attach(m_document, element.release())); How do we decide if HTML should be Html? WebCore/html/HTMLTreeBuilder.cpp:331 + insertHead(token); insertHeadElement WebCore/html/HTMLTreeBuilder.cpp:351 + insertBody(token); insertBodyElement WebCore/html/HTMLTreeBuilder.cpp:958 + void HTMLTreeBuilder::insertHtml(AtomicHTMLToken& token) ugg. Naming problems... WebCore/html/HTMLTreeBuilder.cpp: + ASSERT(token.type() == HTMLToken::StartTag); Why did you remove these asserts? WebCore/html/HTMLTreeBuilder.cpp: + ASSERT(token.type() == HTMLToken::StartTag); ditto WebCore/html/HTMLTreeBuilder.cpp:963 + void HTMLTreeBuilder::insertHead(AtomicHTMLToken& token) Please ASSERT that the token is a StartTag with a name of HEAD WebCore/html/HTMLTreeBuilder.cpp:969 + void HTMLTreeBuilder::insertBody(AtomicHTMLToken& token) please add that ASSERT to all these cases. WebCore/html/HTMLTreeBuilder.h:133 + ASSERT(m_headElement == m_headElement); This assert seems vacuous. WebCore/html/HTMLTreeBuilder.h:169 + ASSERT(m_htmlElement); Please assert that element doesn't have a tag name of HTML, Body, or Head. WebCore/html/HTMLTreeBuilder.h:251 + // We remember <html>, <head> and <body> as their pushed. Their their -> they are
(In reply to comment #7) > WebCore/html/HTMLTreeBuilder.cpp:286 > + m_openElements.pushHtml(attach(m_document, element.release())); > How do we decide if HTML should be Html? Donno. I originally named it pushHTMLElement, but then it seemed confusing because lots of things are HTMLElements. <html> is called HTMLHtmlElement, so I matched the casing. > WebCore/html/HTMLTreeBuilder.cpp:331 > + insertHead(token); > insertHeadElement > > WebCore/html/HTMLTreeBuilder.cpp:351 > + insertBody(token); > insertBodyElement > > WebCore/html/HTMLTreeBuilder.cpp:958 > + void HTMLTreeBuilder::insertHtml(AtomicHTMLToken& token) > ugg. Naming problems... These were part of avoiding pushHTMLElement which seemed confusing. > WebCore/html/HTMLTreeBuilder.cpp: > + ASSERT(token.type() == HTMLToken::StartTag); > Why did you remove these asserts? > > WebCore/html/HTMLTreeBuilder.cpp: > + ASSERT(token.type() == HTMLToken::StartTag); > ditto Because the very next executed line ASSERTS the same? :) > WebCore/html/HTMLTreeBuilder.cpp:963 > + void HTMLTreeBuilder::insertHead(AtomicHTMLToken& token) > Please ASSERT that the token is a StartTag with a name of HEAD This is already done. :) createAndAttachElement asserts startTag and pushHead asserts name HEAD. > WebCore/html/HTMLTreeBuilder.cpp:969 > + void HTMLTreeBuilder::insertBody(AtomicHTMLToken& token) > please add that ASSERT to all these cases. Again, the asserts are already there. :) > WebCore/html/HTMLTreeBuilder.h:133 > + ASSERT(m_headElement == m_headElement); > This assert seems vacuous. Yes, was a typo. Fixed locally (and in the other patch I uploaded last night). Fixed. > WebCore/html/HTMLTreeBuilder.h:169 > + ASSERT(m_htmlElement); > Please assert that element doesn't have a tag name of HTML, Body, or Head. Fixed. > WebCore/html/HTMLTreeBuilder.h:251 > + // We remember <html>, <head> and <body> as their pushed. Their > their -> they are Fixed. Please advise. The first bunch of comments remained non-actionable.
(In reply to comment #8) > (In reply to comment #7) > > WebCore/html/HTMLTreeBuilder.cpp:286 > > + m_openElements.pushHtml(attach(m_document, element.release())); > > How do we decide if HTML should be Html? > > Donno. I originally named it pushHTMLElement, but then it seemed confusing because lots of things are HTMLElements. <html> is called HTMLHtmlElement, so I matched the casing. Hum... Maybe we should call it HTMLHtmlElement then. We can probably skip the "HTML" from HTMLHeadElement. It's slightly inconsistent, but hopefully easier to understand. > > WebCore/html/HTMLTreeBuilder.cpp:331 > > + insertHead(token); > > insertHeadElement > > > > WebCore/html/HTMLTreeBuilder.cpp:351 > > + insertBody(token); > > insertBodyElement > > > > WebCore/html/HTMLTreeBuilder.cpp:958 > > + void HTMLTreeBuilder::insertHtml(AtomicHTMLToken& token) > > ugg. Naming problems... > > These were part of avoiding pushHTMLElement which seemed confusing. > > > WebCore/html/HTMLTreeBuilder.cpp: > > + ASSERT(token.type() == HTMLToken::StartTag); > > Why did you remove these asserts? > > > > WebCore/html/HTMLTreeBuilder.cpp: > > + ASSERT(token.type() == HTMLToken::StartTag); > > ditto > > Because the very next executed line ASSERTS the same? :) I'd rather have extra ASSERTs than wonder if they're redundant with other ASSERTs. In this case, they're documenting what you're allowed to call this function with. > > WebCore/html/HTMLTreeBuilder.cpp:963 > > + void HTMLTreeBuilder::insertHead(AtomicHTMLToken& token) > > Please ASSERT that the token is a StartTag with a name of HEAD > > This is already done. :) createAndAttachElement asserts startTag and pushHead asserts name HEAD. I should make this more explicit in the code. There's a controller layer, a model layer, and the constituents of the model. The insert* functions are the entry points to the modeling layer (the process* functions are in the controller layer). Because they form an "internal API" of sorts, I think it's better if they document what sorts of arguments they accept. Longer term, we should split off the PartiallyConstructedHTMLTree into its own class. > > WebCore/html/HTMLTreeBuilder.cpp:969 > > + void HTMLTreeBuilder::insertBody(AtomicHTMLToken& token) > > please add that ASSERT to all these cases. > > Again, the asserts are already there. :) See above. > Please advise. The first bunch of comments remained non-actionable. I think the naming and layering issues are important. Sorry for not documenting these more clearly. This class is a bit of a "work in progress." :)
(In reply to comment #9) > I should make this more explicit in the code. There's a controller layer, a model layer, and the constituents of the model. The insert* functions are the entry points to the modeling layer (the process* functions are in the controller layer). Because they form an "internal API" of sorts, I think it's better if they document what sorts of arguments they accept. > > Longer term, we should split off the PartiallyConstructedHTMLTree into its own class. Yup should probably split different layers off into separate objects. :) We can include all the .cpp files in an all-in-one file to have inlining if thats needed. > I think the naming and layering issues are important. Sorry for not documenting these more clearly. This class is a bit of a "work in progress." :) Ok. I'm not sure I yet understand the layering bit, but I'll post a patch which fixes the ASSERTS.
Looking back, I don't agree with you that the ASSERTS I removed make sense where they are: - ASSERT(m_openElements.top()->tagQName() == headTag); - m_openElements.pop(); + m_openElements.popHead(); popHead better assert it is popping a head element, otherwise it's a poorly named call. And twice: - ASSERT(token.type() == HTMLToken::StartTag); insertElement(token); It should never be OK to call insertElement with a non-start tag. so that ASSERT also seems superfluous. I agree, more asserts are almost always a good thing. In this case, it seems like the caller is reaching through the abstraction layer to make an ASSERT. Especially in the head case.
(In reply to comment #8) > > WebCore/html/HTMLTreeBuilder.h:251 > > + // We remember <html>, <head> and <body> as their pushed. Their > > their -> they are > > Fixed. I lied. You must have read wrong. "Their ElementRecords". The element records which "belong to" the head, html, and body elements.
Created attachment 60071 [details] With Adam's comments
Comment on attachment 60071 [details] With Adam's comments WebCore/html/HTMLTreeBuilder.h:281 + // We remember <html>, <head> and <body> as their pushed. Their Ok. It's fine to reject my ideas about asserts but, but "We remember <html>, <head> and <body> as their pushed." is really grammatically incorrect. :)
In my zeal to prove You wrong I seem to have missed that we were talking about different "their"s. :)
Created attachment 60095 [details] Patch for landing
Committed r62166: <http://trac.webkit.org/changeset/62166>
http://trac.webkit.org/changeset/62166 might have broken Qt Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/62168 http://trac.webkit.org/changeset/62166 http://trac.webkit.org/changeset/62167