[webkit-reviews] review granted: [Bug 41716] Factor HTMLConstructionSite out of HTMLTreeBuilder : [Attachment 60655] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 41716: Factor HTMLConstructionSite out of HTMLTreeBuilder
https://bugs.webkit.org/show_bug.cgi?id=41716

Attachment 60655: Patch
https://bugs.webkit.org/attachment.cgi?id=60655&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebCore/html/HTMLTreeBuilder.h: 
 +	// FIXME: FragmentScriptingPermission is a HACK for
platform/Pasteboard.
I recommend leaving this code and FIXME in  the TreeBuilder.  It exists to
support the legacy treebuilder usage and can be removed from the base class
when we either factor our the legacy shim, or remove the legacy shim entirely.

WebCore/html/HTMLTreeBuilder.h:113
 +	// FIXME: FragmentScriptingPermission is a HACK for
platform/Pasteboard.
This FIXME doesn't apply to this usage, only to the old usage for the legacy
shim, since the legacy shim was emulating hackish behavior of the old
tokenizer.

WebCore/html/HTMLTreeBuilder.h:110
 +	mutable HTMLElementStack m_openElements;
Confused why these are mutable?

WebCore/html/HTMLTreeBuilder.h:91
 +	PassRefPtr<ChildType> attach(Node* parent, PassRefPtr<ChildType>
prpChild)
This doesn't need to be in the header since its only used from this class.

WebCore/html/HTMLTreeBuilder.h:86
 +	// FIXME: Remove this method.  It exists only to service the legacy
tree builder.
I think we should leave this hack on the TreeBuilder instead of moving it here.
 HTMLContructionSite has valid reasons to know the
m_fragmentScriptingPermission.	The FIXME from before was about the shim
needing to do hacky things.

WebCore/html/HTMLTreeBuilder.h:84
 +	void setForm(Element* form) { m_form = form; }
PassRefPtr<Element>?

WebCore/html/HTMLTreeBuilder.h:82
 +	PassRefPtr<Element> releaseForm() { return m_form.release(); }
We're inconsistent about take vs. release.

WebCore/html/HTMLTreeBuilder.cpp:2313
 +  void HTMLTreeBuilder::generateImpliedEndTags()
Seems this should move onto the site, no?

WebCore/html/HTMLTreeBuilder.cpp:2307
 +  void HTMLTreeBuilder::generateImpliedEndTagsWithExclusion(const
AtomicString& tagName)
Seems this should move onto site, no?

WebCore/html/HTMLTreeBuilder.cpp:@
 +  void HTMLTreeBuilder::reconstructTheActiveFormattingElements()
Seems this belongs on the site as well.

WebCore/html/HTMLTreeBuilder.cpp:2257
 +  bool HTMLTreeBuilder::indexOfFirstUnopenFormattingElement(unsigned&
firstUnopenElementIndex) const
And this...

WebCore/html/HTMLTreeBuilder.cpp:1784
 +		ASSERT(m_site.currentElement()->tagQName() == noscriptTag);
These shoudl be hasTagName(...)

Please consider the above comments.