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

[bugzilla-daemon at webkit.org]

Eric Seidel <eric 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 174571: Patch
https://bugs.webkit.org/attachment.cgi?id=174571&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=174571&action=review


Do we need a <!DOCTYPE> test?  The parser will generate DocType nodes.

Test case from our discussions:
<template>
<tr></tr>
<template></template>
<option>
</template>

I assume a rogue </template> does nothing.

We need another iteration, to fix the cycle things.  But otherwise this is r+
from me.

> Source/WebCore/dom/Document.cpp:6008
> +    if (!m_frame)

You might want to add a comment about what this case is for.

> Source/WebCore/dom/Document.cpp:6013
> +	       m_templateContentsOwnerDocument = HTMLDocument::create(0,
KURL());

"about:blank" is probably better.  emptyURL() may return what you want.

> Source/WebCore/dom/Document.h:1531
> +    RefPtr<Document> m_templateContentsOwnerDocument;

So what happens if we move a <template> element between documents.  Now all its
content subtree point back to this original document.  Which may die?  We need
to make sure that ownerDocument() stays alive?	Adam suggests that XHR
(GuardRef?) must already handle this case, so it may be I'm simply
misunderstanding.

> Source/WebCore/html/HTMLTemplateElement.cpp:60
> +DocumentFragment* HTMLTemplateElement::content()

It appears that if you ever append a <template> to it's .content
DocumentFragment, the template ref's the fragment and the fragment ref's the
template = thus a cycle/leak.

> Source/WebCore/html/HTMLTemplateElement.cpp:72
> +    if (!content || content->isShadowRoot()) {

Do we have a test case for this shadowRoot case?

> Source/WebCore/html/HTMLTemplateElement.cpp:77
> +    if (m_content && m_content.get() == content.get())

m_content check isn't necessary, since you know "content" is already non-null.

> Source/WebCore/html/HTMLTemplateElement.cpp:81
> +    if (content->ownerDocument() !=
ownerDocument()->templateContentsOwnerDocument())
> +	   ownerDocument()->templateContentsOwnerDocument()->adoptNode(content,
ASSERT_NO_EXCEPTION);

We need to do something like this when we move the <template> element between
documents, or?

This also needs a cycle check, as you could assign content to the containing
DocumentFragment of the template element and get another cycle.

You may want to explore ways to share code with the ShadowDom implementation. 
They override iteration functions and likely cover some of this cycle
detection.

> Source/WebCore/html/HTMLTemplateElement.h:46
> +    DocumentFragment* content();

This may end up const with a mutable m_content.  Depending on how others expect
to access this.  (I'm not a fan of that pattern, but it is common in WebKit.)

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:87
>	   task.parent->parserInsertBefore(task.child.get(),
task.nextChild.get());

Do we need to ASSERT in this function that the element's document matches the
parent's document?

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:409
> +    if (currentNode()->isElementNode() &&
currentElement()->hasTagName(templateTag))

You can just call currentNode()->hasTagName() directly.

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

Probably deserves a "why" comment.  // Avoid reparenting outside of the
<template> element.

Hixie may also re-write the spec to handle this case in a more generic way.

> Source/WebCore/html/parser/HTMLElementStack.cpp:550
> +    return inScopeCommon<isHTMLNode>(m_top.get(), templateTag.localName());

Should this just use isRootNode directly?  Adam mentions you may need to move
isRootNode inside the anonymous namespace for it to compile (and remove the
static keyword).

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:-1546
> -	       ASSERT(isParsingFragment());

Could this be isParsingFragmentOrTemplateContents instead?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1664
> +	       return setInsertionMode(InHeadMode);

This line appears to be an error.  I'm surprised we don't have a test case for
this (we may need to add one).

This is the case where we're calling head.innerHTML = ""?  Correct?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2178
> +	       bool ignoreFramesetForFragmentParsing  = m_tree.currentNode() ==
m_tree.openElements()->rootNode();

Do we have a better accessor for this?	m_tree.currentIsRoot(), etc?

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2269
> +	   if (token->name() == templateTag) {

Could you link to your spec bug?