Summary: | Teach HTML5TreeBuilder how to merge attributes from extra html/body elements | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||
Component: | New Bugs | Assignee: | Eric Seidel (no email) <eric> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, eric, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Other | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 41123 | ||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2010-06-28 23:55:45 PDT
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 |