Bug 86031 – [HTMLTemplateElement] Implement DOM element and parser changes

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Rafael Weinstein]

http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html

[Rafael Weinstein]

Created attachment 141044 [details]
Patch

[Rafael Weinstein]

Ok, here's my implementation of HTMLTemplateElement which is based on the implied context element parsing from DocumentFragment.innerHTML.

Note that this bug is depends on 84646, and includes its changes (if there's a better way to do this, let me know).

This is conceptually similar to work that Dimitri did in https://bugs.webkit.org/show_bug.cgi?id=78734 -- and it includes the parser tests that Dimitri authored for that patch.

The deltas from Dimitri's initial work are:

1) The HTMLTemplateElement is actually implemented, including its read-only content attribute whichs contains inert DOM
2) the template element serializes it's inert content (e.g. as with innerHTML)
3) The parsing changes are relative to the implied context parsing approach, rather than defining a new insertion mode.

I'm looking for feedback about the general direction of this implementation. All guidance welcome & appreciated.

[Adam Klein]

Comment on attachment 141044 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141044&action=review

> Source/WebCore/xml/parser/MarkupTokenBase.h:477
> +    const size_t characterDataSize() const

Remove the first "const" here, clang warns about it and it has no effect on a return-by-value.

[Build Bot]

Comment on attachment 141044 [details]
Patch

Attachment 141044 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12646835

[Early Warning System Bot]

Comment on attachment 141044 [details]
Patch

Attachment 141044 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12648733

[Early Warning System Bot]

Comment on attachment 141044 [details]
Patch

Attachment 141044 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12648734

[Gyuyoung Kim]

Comment on attachment 141044 [details]
Patch

Attachment 141044 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12665141

[Hajime Morrita]

Comment on attachment 141044 [details]
Patch

It looks pulling up innerHTML to DocumentFragment can be done in a separate patch.
It's more trivial and will be easier to land, making the size of essential change smaller.

[Ryosuke Niwa]

(In reply to comment #8)
> (From update of attachment 141044 [details])
> It looks pulling up innerHTML to DocumentFragment can be done in a separate patch.
> It's more trivial and will be easier to land, making the size of essential change smaller.

Yes, it will be done in https://bugs.webkit.org/show_bug.cgi?id=84646, which is a blocker of this bug.

[Rafael Weinstein]

Created attachment 141182 [details]
Patch

[Rafael Weinstein]

Created attachment 141189 [details]
Patch

[Rafael Weinstein]

Patch now contains diffs relative to https://bugs.webkit.org/show_bug.cgi?id=84646 (thanks, Arv)

[Dimitri Glazkov (Google)]

Comment on attachment 141189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141189&action=review

Great stuff! A few things:
* looks like there's a patch-in-the-middle missing -- the one that adds HTML_TEMPLATES define to WebKit build system
* template token is not being protected by defines, which means that even though you have the flag, the parser will start behaving in a new way immediately.
* similarly, the HTMLTemplateElement is always being compiled. The WebKit practice is to guard the file with defines.

> Source/WebCore/html/HTMLTagNames.in:125
> +template interfaceName=HTMLTemplateElement, conditional=TEMPLATE_TAG

TEMPLATE_TAG -> HTML_TEMPLATES?

[Rafael Weinstein]

Created attachment 142518 [details]
Patch

[Rafael Weinstein]

Comments addressed. Also, this patch now depends on the new Document.parse() patch (still https://bugs.webkit.org/show_bug.cgi?id=84646).

[Dimitri Glazkov (Google)]

(In reply to comment #15)
> Comments addressed. Also, this patch now depends on the new Document.parse() patch (still https://bugs.webkit.org/show_bug.cgi?id=84646).

Huzzah!

[Ojan Vafai]

Comment on attachment 142518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142518&action=review

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:493
> +    // Template Fragment case.

Nit: useless comment. Just describes what the next two lines of code clearly do.

[Adam Klein]

Comment on attachment 142518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142518&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:122
> +            effectiveNode = static_cast<Node*>(static_cast<HTMLTemplateElement*>(effectiveNode)->content().get());

Nit: no need for the static_cast<Node*>().

> Source/WebCore/html/HTMLTemplateElement.cpp:3
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

Nit: probably can drop this copyright line

> Source/WebCore/html/HTMLTemplateElement.h:3
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

Ditto, no need for this copyright methinks

> Source/WebCore/html/HTMLTemplateElement.h:37
> +#include "HTMLCollection.h"

Unnecessary include, I think.

> Source/WebCore/html/HTMLTemplateElement.h:45
> +    PassRefPtr<DocumentFragment> content();

This should return a DocumentFragment* instead of a PassRefPtr. Most uses don't take a reference (they call .get()). That'll simplify the callsites and avoid unnecessary churn (the former is more important to me than the latter).

[Ryosuke Niwa]

Comment on attachment 142518 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142518&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:119
> +        Node* effectiveNode = targetNode;

I'm not sure effectiveName is a descriptive name. I'd prefer declaring current node here instead as in:
Node* current = targetNode->hasTagName(templateTag) ? toHTMLTemplateElement(targetNode)->content().get() : targetNode->firstChild();

> Source/WebCore/html/HTMLTemplateElement.cpp:54
> +inline HTMLTemplateElement::HTMLTemplateElement(const QualifiedName& tagName, Document* document)
> +    : HTMLElement(tagName, document)
> +{
> +}
> +
> +PassRefPtr<HTMLTemplateElement> HTMLTemplateElement::create(const QualifiedName& tagName, Document* document)
> +{
> +    return adoptRef(new HTMLTemplateElement(tagName, document));
> +}

These two functions can be inline.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1073
> +    m_fragmentContext.push(static_cast<HTMLTemplateElement*>(m_tree.currentElement())->content().get());

Can we add a safe (with an assertion) toHTMLTemplateElement?

> LayoutTests/resources/dump-as-markup.js:226
> +      return node.namespaceURI = 'http://www.w3.org/1999/xhtml' && node.tagName == 'TEMPLATE';

Nit: indentation should be 4 spaces.

[Rafael Weinstein]

Created attachment 169219 [details]
Patch

[Rafael Weinstein]

Note: This latest patch is a complete implementation of the current template element spec: 

http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html

The purpose of the current patch is informational for now. I imagine landing the HTMLTemplateElement in a series of smaller patches:

1) Feature flag
2) Template Element (simple implementation -- just implement the content attribute)
3) Parser changes (directs parsed markup to the content attribute) + html5lib parser tests
4) Full Template Element API implementation (innerHTML changes, templateContentsOwnerDocument) + tests
5) "Real" implementation of preload scanner properly ignoring template content resources + tests

Also note that this patch no longer depends on Document.parse() (https://bugs.webkit.org/show_bug.cgi?id=84646). The "implied context" parsing which was agreed on in WebApps is effectively implemented here, but exposing actual direct API for Document.parse() did not gain consensus.

[Dimitri Glazkov (Google)]

Comment on attachment 169219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169219&action=review

This looks seriously cool.

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

;

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

Interesting. So we share this document among all templates? Is this a provision in spec?

[Rafael Weinstein]

Comment on attachment 169219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169219&action=review

>> Source/WebCore/dom/Document.h:1524
>> +    RefPtr<Document> m_templateContentsOwnerDocument;
> 
> Interesting. So we share this document among all templates? Is this a provision in spec?

Yes. http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html#dfn-template-contents-owner

[Rafael Weinstein]

Comment on attachment 169219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169219&action=review

> Source/WebCore/html/HTMLTemplateElement.cpp:53
> +    if (RefPtr<DocumentFragment> fragment = createFragmentForInnerOuterHTML(html, this, AllowScriptingContent, ec))

Note to self: Move this to be a check in HTMLElement for hasLocalName(templateTag). The one pointer check will be faster than making setInnerHTML be virtual

[Rafael Weinstein]

Created attachment 173943 [details]
Patch

[Rafael Weinstein]

Created attachment 174042 [details]
Patch

[Early Warning System Bot]

Comment on attachment 174042 [details]
Patch

Attachment 174042 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14832143

[Build Bot]

Comment on attachment 174042 [details]
Patch

Attachment 174042 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14832150

[EFL EWS Bot]

Comment on attachment 174042 [details]
Patch

Attachment 174042 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14688537

[Adam Barth]

Comment on attachment 174042 [details]
Patch

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.html 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?

[Adam Barth]

The patch generally looks good.  I'd like Eric Seidel to review the patch as well (perhaps for the next iteration).

[Adam Barth]

Comment on attachment 174042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174042&action=review

> Source/WebCore/ChangeLog:8
> +        Initial implementation. This patch includes the parser changes, new IDL and element implementation for <template>.

Can you add a link to the spec here?  My understanding is that the spec is being updated after the TPAC discussion, but perhaps we can link to the in-progress spec?

[Adam Klein]

Comment on attachment 174042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174042&action=review

>> 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 ?

Same thought as Adam: when I did my quick-and-dirty XHTML version, I replaced this with the simple call to create (to match what I did for the newly-created XHTML doc).

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

I don't believe that's quite true, since it gives this method "inline" linkage. And that's fine, because it's only called from this compilation unit. But I can't say how much effect it's having.

>> Source/WebCore/html/HTMLTemplateElement.cpp:89
>> +        ownerDocument()->templateContentsOwnerDocument()->adoptNode(content, nofail);
> 
> There's a macro that does the "nofail" thing magically for you.

It's called ASSERT_NO_EXCEPTION (and it just gets passed wherever you'd pass an ExceptionCode)

>> Source/WebCore/html/HTMLTemplateElement.cpp:101
>> +#endif
> 
> ?

This is in-keeping with other HTML element helpers. The reason it's not in the .h file is to avoid #including HTMLNames (needed for HTMLNames::templateTag).  See, e.g., HTMLOptionElement and HTMLTableCellElement for other examples.

> Source/WebCore/html/HTMLTemplateElement.h:68
> +    // FIXME: Add debug asserts.

This FIXME is wrong, should be replaced with a comment saying that the debug version is in the implementation file.

>> Source/WebCore/html/HTMLTemplateElement.h:71
>> +#endif // NDEBUG
> 
> It seems like a bad idea to have a Debug-only overload that differs only in const-ness from the Release version....

Probably deserves a comment. As noted above, this is common practice under WebCore/html. This is the non-debug implementation of the same thing it's implemented for debug in the .cpp file (see above). Note the function declaration a few lines above. We only do this for the const version and do a const_cast in the non-const version to avoid duplicating the checks.  If there's some nicer way to do this I'm all ears (or maybe we don't care about including HTMLNames.h anymore?).

>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:408
>> +inline Document* HTMLConstructionSite::ownerDocumentForCurrentNode()
> 
> The inline keyword has no effect here.

Again, not the case (unless I'm misunderstanding C++), and I think it's quite possibly valuable here (though maybe it should only be added if it speeds up the benchmark).

[Adam Barth]

> I don't believe that's quite true, since it gives this method "inline" linkage. And that's fine, because it's only called from this compilation unit. But I can't say how much effect it's having.

I didn't know you could give out-of-line methods internal linkage.  There's so much to learn about C++.

[Adam Barth]

I mentioned to Raf that this patch as a subtle GC bug.  We might collect the wrappers for DOM nodes inside the template even through they are reachable via the template element.  The solution we discussed was to add a hidden reference from the HTMLTemplateElement wrapper to the wrapper for its document fragment.

[Rafael Weinstein]

Created attachment 174571 [details]
Patch

[Rafael Weinstein]

All comments addressed except for the GC issue (template contents wrappers won't be kept alive).

abarth, do you want me to address that in this patch?

[Rafael Weinstein]

Comment on attachment 174042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174042&action=review

>> Source/WebCore/ChangeLog:8
>> +        Initial implementation. This patch includes the parser changes, new IDL and element implementation for <template>.
> 
> Can you add a link to the spec here?  My understanding is that the spec is being updated after the TPAC discussion, but perhaps we can link to the in-progress spec?

done.

>> Source/WebCore/css/html.css:1265
>> +  display: none
> 
> four-space indent

done

>>> 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 ?
> 
> Same thought as Adam: when I did my quick-and-dirty XHTML version, I replaced this with the simple call to create (to match what I did for the newly-created XHTML doc).

done. Additionally, we determined that the templateContentsOwnerDocument needs to match the ownerDocument's isHTMLDocument(). I've added that to the spec, to this patch and added a test (ownerDocumentXHTML).

>> Source/WebCore/html/HTMLElement.h:58
>> +    virtual void setInnerHTML(const String&, ExceptionCode&);
> 
> Can you run http://trac.webkit.org/browser/trunk/PerformanceTests/Parser/tiny-innerHTML.html to make sure this isn't slowing us down?

I've moved this to HTMLElement to avoid the virtual vtable lookup and verified that it doesn't regress perf noticably in the test you mention.

>>> Source/WebCore/html/HTMLTemplateElement.cpp:46
>>> +inline HTMLTemplateElement::HTMLTemplateElement(const QualifiedName& tagName, Document* document)
>> 
>> The inline keyword is meaningless here.
> 
> I don't believe that's quite true, since it gives this method "inline" linkage. And that's fine, because it's only called from this compilation unit. But I can't say how much effect it's having.

leaving.

>> Source/WebCore/html/HTMLTemplateElement.cpp:72
>> +    return m_content.get();
> 
> Apparently Darin Adler prefers the
> 
> if (!m_content)
>     m_content = ....
> return m_content.get()
> 
> pattern for some reason.

done.

>>> Source/WebCore/html/HTMLTemplateElement.cpp:89
>>> +        ownerDocument()->templateContentsOwnerDocument()->adoptNode(content, nofail);
>> 
>> There's a macro that does the "nofail" thing magically for you.
> 
> It's called ASSERT_NO_EXCEPTION (and it just gets passed wherever you'd pass an ExceptionCode)

done

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

removed

>> Source/WebCore/html/HTMLTemplateElement.h:68
>> +    // FIXME: Add debug asserts.
> 
> This FIXME is wrong, should be replaced with a comment saying that the debug version is in the implementation file.

done

>>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:408
>>> +inline Document* HTMLConstructionSite::ownerDocumentForCurrentNode()
>> 
>> The inline keyword has no effect here.
> 
> Again, not the case (unless I'm misunderstanding C++), and I think it's quite possibly valuable here (though maybe it should only be added if it speeds up the benchmark).

leaving.

>> Source/WebCore/html/parser/HTMLConstructionSite.cpp:423
>> +
> 
> This blank line seems spurious.

removed

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

the fast that there's confusion here between our impl and the spec, WRT the stack growing up or down, i feel like "last" is actually clearer. I'll change if you feel strongly.

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:686
>>          if (!m_tree.openElements()->secondElementIsHTMLBodyElement() || m_tree.openElements()->hasOnlyOneElement()) {
> 
> There's no call to hasTemplateInHTMLScope here?

No. the change here (like all cases that are now isParsingFragmentOrTemplateContents) is that while this case could previously only occurred while parsing innerHTML (fragment case), it can now occur while parsing template contents. However, this code here doesn't know *which* of these has occurred -- only that one of them has.

>> 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?

Done. I've factored this out into processTemplateStartTag to avoid the extra dispatching, and it's used whenever a template start tag is encountered.

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:1634
>> +#endif
> 
> I would just remove these ASSERTs.  They're not worth conditionalizing.

done.

>> 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?

As discussed, there's not really a good way to run-time enable this, so we'll have to give release engineering a heads up that this is on the train, but may get yanked off before ship if the spec isn't settled.

[Adam Barth]

I think it's fine to work on the GC issue in a separate patch, but I would like eseidel to take a look at this patch.

[Early Warning System Bot]

Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14846659

[Early Warning System Bot]

Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14862002

[kov's GTK+ EWS bot]

Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14860081

[EFL EWS Bot]

Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14857467

[Build Bot]

Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14843879

[Build Bot]

Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14861295

[Eric Seidel (no email)]

Comment on attachment 174571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174571&action=review

Will look in much greater detail in a bit.

> Source/WebCore/dom/Document.cpp:6006
> +Document* Document::templateContentsOwnerDocument()

Document doesn't seem to know if it's owned by a template elmenet or not, does it?  This function, if called, will allocate a new Document regardless of whether there is a template related to this document?  Should this logic be on <template> instead?

[Eric Seidel (no email)]

Comment on attachment 174571 [details]
Patch

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?

[Rafael Weinstein]

Created attachment 176518 [details]
Patch

[Early Warning System Bot]

Comment on attachment 176518 [details]
Patch

Attachment 176518 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15003937

[Early Warning System Bot]

Comment on attachment 176518 [details]
Patch

Attachment 176518 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15018519

[EFL EWS Bot]

Comment on attachment 176518 [details]
Patch

Attachment 176518 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15026506

[Rafael Weinstein]

Created attachment 176531 [details]
Patch

[Rafael Weinstein]

Comment on attachment 174571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174571&action=review

>> Source/WebCore/dom/Document.cpp:6006
>> +Document* Document::templateContentsOwnerDocument()
> 
> Document doesn't seem to know if it's owned by a template elmenet or not, does it?  This function, if called, will allocate a new Document regardless of whether there is a template related to this document?  Should this logic be on <template> instead?

As discussed offline: this is reflected in the spec. All templates contained within a document, share the same templateContentsOwnerDocument. It needs to be here.

>> Source/WebCore/dom/Document.cpp:6008
>> +    if (!m_frame)
> 
> You might want to add a comment about what this case is for.

add exact spec language.

>> Source/WebCore/dom/Document.cpp:6013
>> +            m_templateContentsOwnerDocument = HTMLDocument::create(0, KURL());
> 
> "about:blank" is probably better.  emptyURL() may return what you want.

done (used blankURL()). note that this is implied by the spec: (DOM) 'Unless explicitly given when a document is created its encoding is the utf-8 encoding, its content type is "application/xml", and its URL is "about:blank".'

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

Yes. I believe the solution is to enforce that all templates reachable from a given document should have templateContentsOwnerDocuments which matches that of the containing document.

I'd like to do this is a follow-on patch.

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

Yes. ShadowRoot has a similar problem. I'd like to wait for ShadowRoot to land a patch and then piggyback on that solution. Again, I'd like to approach this in a separate patch.

>> Source/WebCore/html/HTMLTemplateElement.cpp:72
>> +    if (!content || content->isShadowRoot()) {
> 
> Do we have a test case for this shadowRoot case?

Yes. It's in ownerDocument.html

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

done

>> Source/WebCore/html/HTMLTemplateElement.cpp:81
>> +        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.

a) Yes. This is the same issue as above (moving templates between documents).
b) Yes. This is the same issue as above (disallowing cycles)
c) Yup. That's my hope.

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

done.

>> 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?

added ASSERT()s inside ContainerNode::parserInsertionBefore/AppendChild

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

done.

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

again, copied spec language

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

done

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:-1546
>> -            ASSERT(isParsingFragment());
> 
> Could this be isParsingFragmentOrTemplateContents instead?

No. This is the case of <select><template></template>...

When the template pops, resetInsertionMode will be called, and there is no longer a <template> on the stack to detect the parsing template contents case.

>> 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?

There are tests on both side. I just hadn't run the non-template enabled tests. conditional reversed.

>> 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?

added HTMLConstructionSite::currentIsRootNode() (and several usages).

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2269
>> +        if (token->name() == templateTag) {
> 
> Could you link to your spec bug?

done

[Rafael Weinstein]

Ok, this is ready for review now. Note that a few issues which arose in this review will be addressed in follow-on patches:

The only one which isn't a spec bug and linked to within this patch with a FIXME is:

-Keep alive wrappers for content nodes.

[Rafael Weinstein]

(p.s. will obvious fix all builds before landing).

[Early Warning System Bot]

Comment on attachment 176531 [details]
Patch

Attachment 176531 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15003951

[Early Warning System Bot]

Comment on attachment 176531 [details]
Patch

Attachment 176531 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15031341

[EFL EWS Bot]

Comment on attachment 176531 [details]
Patch

Attachment 176531 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15031345

[Rafael Weinstein]

*** Bug 78734 has been marked as a duplicate of this bug. ***

[Build Bot]

Comment on attachment 176531 [details]
Patch

Attachment 176531 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15018563

[Build Bot]

Comment on attachment 176531 [details]
Patch

Attachment 176531 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15027525

[Rafael Weinstein]

Created attachment 176611 [details]
Patch

[Rafael Weinstein]

Hopefully this patch fixes all builds.

[Build Bot]

Comment on attachment 176611 [details]
Patch

Attachment 176611 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15031461

New failing tests:
fast/dom/HTMLTemplateElement/ownerDocument.html
fast/dom/HTMLTemplateElement/innerHTML.html
fast/dom/HTMLTemplateElement/cloneNode.html
html5lib/run-template.html
fast/dom/HTMLTemplateElement/inertContents.html
fast/dom/HTMLTemplateElement/ownerDocumentXHTML.xhtml

[Build Bot]

Comment on attachment 176611 [details]
Patch

Attachment 176611 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15027650

New failing tests:
fast/dom/HTMLTemplateElement/ownerDocument.html
fast/dom/HTMLTemplateElement/cloneNode.html
fast/dom/HTMLTemplateElement/innerHTML.html
html5lib/run-template.html
fast/dom/HTMLTemplateElement/inertContents.html
fast/dom/HTMLTemplateElement/ownerDocumentXHTML.xhtml

[Rafael Weinstein]

Created attachment 176788 [details]
Patch

[WebKit Review Bot]

Attachment 176788 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/gtk/TestExpectations:81:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/gtk/TestExpectations:82:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 2 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rafael Weinstein]

Created attachment 176791 [details]
Patch

[Rafael Weinstein]

Created attachment 176811 [details]
Patch

[Rafael Weinstein]

Created attachment 176870 [details]
Patch

[Rafael Weinstein]

Created attachment 176990 [details]
Patch

[EFL EWS Bot]

Comment on attachment 176990 [details]
Patch

Attachment 176990 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15057441

[Adam Barth]

Comment on attachment 176990 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176990&action=review

Looks like you've got a compile problem on EFL.

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:9680
> +            <File

Looks like you have spaces here rather than tabs.  This is one of the rare files in WebKit that uses tabs.

> Source/WebCore/css/html.css:1267
> +template {
> +    display: none
> +}

Does this need to be conditional on ENABLE(TEMPLATE_ELEMENT) ?

> Source/WebCore/html/HTMLTagNames.in:127
> +template interfaceName=HTMLTemplateElement, conditional=TEMPLATE_ELEMENT

You shouldn't need the interfaceName attribute.  The default here is going to be HTMLTemplateElement.  textarea needs it because the first A is capitalized.

> Source/WebCore/html/HTMLTemplateElement.cpp:68
> +

You've got an extra blank line here.

[Rafael Weinstein]

Created attachment 177109 [details]
Patch

[Rafael Weinstein]

Comment on attachment 176990 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176990&action=review

>> Source/WebCore/WebCore.vcproj/WebCore.vcproj:9680
>> +            <File
> 
> Looks like you have spaces here rather than tabs.  This is one of the rare files in WebKit that uses tabs.

woops. fixed.

>> Source/WebCore/css/html.css:1267
>> +}
> 
> Does this need to be conditional on ENABLE(TEMPLATE_ELEMENT) ?

Yes. Good catch. Done.

>> Source/WebCore/html/HTMLTagNames.in:127
>> +template interfaceName=HTMLTemplateElement, conditional=TEMPLATE_ELEMENT
> 
> You shouldn't need the interfaceName attribute.  The default here is going to be HTMLTemplateElement.  textarea needs it because the first A is capitalized.

done.

>> Source/WebCore/html/HTMLTemplateElement.cpp:68
>> +
> 
> You've got an extra blank line here.

removed.

[EFL EWS Bot]

Comment on attachment 177109 [details]
Patch

Attachment 177109 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15061863

[Adam Barth]

Comment on attachment 177109 [details]
Patch

Looks like you still have an EFL build issue to resolve, but this seems ready to land.

[Rafael Weinstein]

Created attachment 177330 [details]
Patch

[Rafael Weinstein]

Created attachment 177381 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177330 [details]
Patch

Rejecting attachment 177330 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ripts/update-webkit line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
   c932ddb..08cc24c  HEAD       -> origin/HEAD
error: Ref refs/remotes/origin/master is at 08cc24ce3a9ec5a16849aab51e2af879acbc10d7 but expected c932ddba2bb69e3e4446533e5ce469603719b3e9
 ! c932ddb..08cc24c  master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15100803

[WebKit Review Bot]

Comment on attachment 177330 [details]
Patch

Clearing flags on attachment: 177330

Committed r136467: <http://trac.webkit.org/changeset/136467>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 103966