[webkit-reviews] review denied: [Bug 86031] Implement HTMLTemplateElement : [Attachment 174042] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Rafael Weinstein
<rafaelw at chromium.org>'s request for review:
Bug 86031: Implement HTMLTemplateElement
https://bugs.webkit.org/show_bug.cgi?id=86031

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=174042&action=review


Looks like you've got some compile trouble.

> Source/WebCore/css/html.css:1265
> +  display: none

four-space indent

> Source/WebCore/dom/Document.cpp:6006
> +	   m_templateContentsOwnerDocument =
implementation()->createHTMLDocument("");

"" ?

Do we really need to go through the implementation?  Why can't we just create
an HTML document via HTMLDocument::create ?

> Source/WebCore/html/HTMLElement.h:58
> -    void setInnerHTML(const String&, ExceptionCode&);
> +    virtual void setInnerHTML(const String&, ExceptionCode&);

Can you run
http://trac.webkit.org/browser/trunk/PerformanceTests/Parser/tiny-innerHTML.htm
l to make sure this isn't slowing us down?

> Source/WebCore/html/HTMLTemplateElement.cpp:46
> +inline HTMLTemplateElement::HTMLTemplateElement(const QualifiedName&
tagName, Document* document)

The inline keyword is meaningless here.

> Source/WebCore/html/HTMLTemplateElement.cpp:72
> +    if (m_content)
> +	   return m_content.get();
> +
> +    m_content =
DocumentFragment::create(ownerDocument()->templateContentsOwnerDocument());
> +    return m_content.get();

Apparently Darin Adler prefers the

if (!m_content)
    m_content = ....
return m_content.get()

pattern for some reason.

> Source/WebCore/html/HTMLTemplateElement.cpp:89
> +	   ownerDocument()->templateContentsOwnerDocument()->adoptNode(content,
nofail);

There's a macro that does the "nofail" thing magically for you.

> Source/WebCore/html/HTMLTemplateElement.cpp:101
> +#ifndef NDEBUG
> +const HTMLTemplateElement* toHTMLTemplateElement(const Node* node)
> +{
> +    ASSERT(!node || (node->isHTMLElement() &&
node->hasTagName(templateTag)));
> +    return static_cast<const HTMLTemplateElement*>(node);
> +}
> +#endif

?

> Source/WebCore/html/HTMLTemplateElement.h:50
> +    virtual void setInnerHTML(const String&, ExceptionCode&);

Please add the OVERRIDE keyword.

> Source/WebCore/html/HTMLTemplateElement.h:71
> +#ifdef NDEBUG
> +inline const HTMLTemplateElement* toHTMLTemplateElement(const Node* node)
> +{
> +    // FIXME: Add debug asserts.
> +    return static_cast<const HTMLTemplateElement*>(node);
> +}
> +#endif // NDEBUG

It seems like a bad idea to have a Debug-only overload that differs only in
const-ness from the Release version....

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:408
> +inline Document* HTMLConstructionSite::ownerDocumentForCurrentNode()

The inline keyword has no effect here.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:423
> +

This blank line seems spurious.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:489
> +    HTMLElementStack::ElementRecord* lastTemplateElement =
m_openElements.topmost(templateTag.localName());

lastTemplateElement -> topmostTemplateElement ?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:686
>	   if (!m_tree.openElements()->secondElementIsHTMLBodyElement() ||
m_tree.openElements()->hasOnlyOneElement()) {

There's no call to hasTemplateInHTMLScope here?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1320
> +	       processStartTagForInHead(token);

This pattern seems inefficient.  We already know that the token is templateTag.
 Shouldn't we call a function that knows that too?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1634
> +#if !ENABLE(TEMPLATE_TAG)
>	       ASSERT(isParsingFragment());
> +#endif

I would just remove these ASSERTs.  They're not worth conditionalizing.

> Source/WebKit/chromium/features.gypi:113
> +	 'ENABLE_TEMPLATE_TAG=1',

I guess there's no way to have a runtime flag for the template tag.  What's our
plan for controlling when we ship this feature?