Bug 41716

Summary: Factor HTMLConstructionSite out of HTMLTreeBuilder
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 41123    
Attachments:
Description Flags
Patch eric: review+

Description Adam Barth 2010-07-06 15:02:11 PDT
Factor HTMLConstructionSite out of HTMLTreeBuilder
Comment 1 Adam Barth 2010-07-06 15:03:59 PDT
Created attachment 60655 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-07-06 15:34:50 PDT
Comment on attachment 60655 [details]
Patch

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.
Comment 3 Adam Barth 2010-07-06 15:51:29 PDT
Committed r62599: <http://trac.webkit.org/changeset/62599>