41337 – Teach HTML5TreeBuilder how to merge attributes from extra html/body elements

RESOLVED FIXED 41337

Teach HTML5TreeBuilder how to merge attributes from extra html/body elements

https://bugs.webkit.org/show_bug.cgi?id=41337

Summary Teach HTML5TreeBuilder how to merge attributes from extra html/body elements

Eric Seidel (no email)

Reported 2010-06-28 23:55:45 PDT

Teach HTML5TreeBuilder how to merge attributes from extra html/body elements

Attachments
Patch (6.22 KB, patch) 2010-06-28 23:57 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
this doesn't work (6.34 KB, patch) 2010-06-29 00:16 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Patch (12.26 KB, patch) 2010-06-29 01:46 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
With Adam's comments (15.95 KB, patch) 2010-06-29 16:45 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Patch for landing (15.95 KB, patch) 2010-06-30 01:06 PDT, Eric Seidel (no email)	eric: commit-queue+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2010-06-28 23:57:01 PDT

Created attachment 59994 [details] Patch

Adam Barth

Comment 2 2010-06-29 00:00:27 PDT

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?

Eric Seidel (no email)

Comment 3 2010-06-29 00:05:12 PDT

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

Adam Barth

Comment 4 2010-06-29 00:09:20 PDT

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

Eric Seidel (no email)

Comment 5 2010-06-29 00:16:36 PDT

Created attachment 59995 [details] this doesn't work

Eric Seidel (no email)

Comment 6 2010-06-29 01:46:55 PDT

Created attachment 59997 [details] Patch

Adam Barth

Comment 7 2010-06-29 09:55:19 PDT

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

Eric Seidel (no email)

Comment 8 2010-06-29 11:58:23 PDT

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

Adam Barth

Comment 9 2010-06-29 13:24:52 PDT

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

Eric Seidel (no email)

Comment 10 2010-06-29 15:41:43 PDT

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

Eric Seidel (no email)

Comment 11 2010-06-29 16:04:43 PDT

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.

Eric Seidel (no email)

Comment 12 2010-06-29 16:41:33 PDT

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

Eric Seidel (no email)

Comment 13 2010-06-29 16:45:44 PDT

Created attachment 60071 [details] With Adam's comments

Adam Barth

Comment 14 2010-06-29 17:17:49 PDT

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

Eric Seidel (no email)

Comment 15 2010-06-29 21:27:05 PDT

In my zeal to prove You wrong I seem to have missed that we were talking about different "their"s. :)

Eric Seidel (no email)

Comment 16 2010-06-30 01:06:41 PDT

Created attachment 60095 [details] Patch for landing

Eric Seidel (no email)

Comment 17 2010-06-30 01:19:13 PDT

Committed r62166: <http://trac.webkit.org/changeset/62166>

WebKit Review Bot

Comment 18 2010-06-30 01:54:55 PDT

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Other

OS OS X 10.5

Product WebKit

Component New Bugs

Assignee

Eric Seidel (no email)

Reported

2010-06-28 23:55 PDT

Modified

2010-06-30 01:54 PDT History

CC List

3 users Show

URL

Keywords

Depends on

Blocks

41123

Dependencies

tree graph