Last Comment Bug 818976 - Implement web components template.
: Implement web components template.
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla22
Assigned To: William Chen [:wchen]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: CVE-2013-1720 CVE-2013-5603 1017896 CVE-2016-1960
Blocks: webcomponents
  Show dependency treegraph
 
Reported: 2012-12-06 10:33 PST by William Chen [:wchen]
Modified: 2016-02-14 13:30 PST (History)
12 users (show)
wchen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Modify HTML5 parser to parse 'template' element. (73.11 KB, patch)
2013-01-29 22:39 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Modify HTML5 parser to parse 'template' element. (80.62 KB, patch)
2013-01-30 14:24 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
interdiff (22.23 KB, patch)
2013-01-30 14:25 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Modify HTML5 parser to parse 'template' element. (80.78 KB, patch)
2013-01-30 15:11 PST, William Chen [:wchen]
hsivonen: review+
hsivonen: feedback+
Details | Diff | Splinter Review
Modify HTML5 parser to parse 'template' element. (104.53 KB, patch)
2013-02-05 06:53 PST, William Chen [:wchen]
hsivonen: review+
Details | Diff | Splinter Review
interdiff (38.57 KB, patch)
2013-02-05 06:54 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Modify HTML5 parser to parse 'template' element. (108.34 KB, patch)
2013-02-21 09:07 PST, William Chen [:wchen]
hsivonen: review+
Details | Diff | Splinter Review
parser interdiff (41.15 KB, patch)
2013-02-21 09:07 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Part 1: Add supplement code and new tree op to support generated code in HTML5 parser. (5.47 KB, patch)
2013-02-21 09:08 PST, William Chen [:wchen]
hsivonen: review+
Details | Diff | Splinter Review
Part 2: Implement template element interface. (108.34 KB, patch)
2013-02-21 09:09 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Part 3: Generated code for HTML5 parser. (208.04 KB, patch)
2013-02-21 09:09 PST, William Chen [:wchen]
hsivonen: review+
Details | Diff | Splinter Review
Part 2: Implement template element interface. (38.76 KB, patch)
2013-02-21 09:11 PST, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Part 2: Implement template element interface. v2 (41.52 KB, patch)
2013-03-18 13:00 PDT, William Chen [:wchen]
mrbkap: review+
bzbarsky: review-
Details | Diff | Splinter Review
template element v1 diff v2 (15.76 KB, patch)
2013-03-18 13:00 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Part 2: Implement template element interface. v3 (46.47 KB, patch)
2013-03-21 15:32 PDT, William Chen [:wchen]
bzbarsky: review+
Details | Diff | Splinter Review
template element v2 diff v3 (17.29 KB, patch)
2013-03-21 15:33 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review

Comment 1 William Chen [:wchen] 2013-01-29 22:39:13 PST
Created attachment 708009 [details] [diff] [review]
Modify HTML5 parser to parse 'template' element.
Comment 2 Henri Sivonen (:hsivonen) 2013-01-30 08:06:54 PST
Upon first reading, this looks very good!

However, it seems to me that  "link", "script", "style", "meta", template" in template contents mode are not handled per spec. The spec requires falling through to "in head" but keeping the mode. The code switches the mode to "in body" and reprocesses. So the token processing ends up being right but the mode ends up changing when it shouldn't.

I'll have to re-read this with the spec still.
Comment 3 William Chen [:wchen] 2013-01-30 14:24:58 PST
Created attachment 708327 [details] [diff] [review]
Modify HTML5 parser to parse 'template' element.

I noticed that in my last patch I was changing the mode in other places where I shouldn't have. I've gone through and fixed those cases. I'm convinced that there isn't a nice way to use switch statement fall through to implement "Process the token using the rules for the ... insertion mode." for templates in all cases. It looks like other rules just duplicate the code (sigh) when this problem comes up, let me know if there are better ways. I've used fall through where it seems appropriate and function calls everywhere else.
Comment 4 William Chen [:wchen] 2013-01-30 14:25:44 PST
Created attachment 708328 [details] [diff] [review]
interdiff
Comment 5 William Chen [:wchen] 2013-01-30 15:11:56 PST
Created attachment 708359 [details] [diff] [review]
Modify HTML5 parser to parse 'template' element.
Comment 6 Henri Sivonen (:hsivonen) 2013-02-04 09:11:49 PST
Comment on attachment 708359 [details] [diff] [review]
Modify HTML5 parser to parse 'template' element.

>+    public static final ElementName TEMPLATE = new ElementName("template", "template", TreeBuilder.TEMPLATE);


The spec puts <template> in the special category (Section 7), so you need 
TreeBuilder.Template | SPECIAL
as the last argument there.


>+    private static final int TEMPLATE_CONTENTS = 22;

(This is fine for now, but I'll need to renumber these as part of the Rust work so that the order of the numbers matches the order they are used is the switches. It's sad to have to manually number these, but using real Java enums kills perf on HotSpot.)

>+                // https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html#template-contents-insertion-mode
>+                case TEMPLATE_CONTENTS:
>+                    if (currentPtr == 0) {
>+                        assert fragment;
>+                        break eofloop;
>+                    } else {

No need for "else" after break/continue/return. (There's a bunch of existing "else after break/return" occurrences, but let's not add more.)


>+                        popOnEof();

Please add some reminder above the popOnEof() line that parser error reporting needs to be added. (We can defer the implementation of error reporting for later, since it involves localizable strings in the Gecko case.)

>+                        resetTheInsertionMode();
>+                        // Reprocess token.
>+                        continue;
>+                    }

>             switch (mode) {
>+                // https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html#template-contents-insertion-mode


>+                case TEMPLATE_CONTENTS:

Let's add
templatecontentsloop: for (;;) {
here.

(Java really allows labeled blocks, so we don't really need the "for (;;) part here, but I was unaware of that when I wrote the parser and now my Rust translator can't deal with the clearer Java case, yet...)

>+                    switch (group) {

>+                        case META:
>+                            checkMetaCharset(attributes);
>+                            appendVoidElementToCurrentMayFoster(
>+                                    elementName,
>+                                    attributes);
>+                            selfClosing = false;
>+                            attributes = null; // CPP
>+                            break starttagloop;
>+                        case LINK_OR_BASEFONT_OR_BGSOUND:
>+                            appendVoidElementToCurrentMayFoster(
>+                                    elementName,
>+                                    attributes);
>+                            selfClosing = false;
>+                            attributes = null; // CPP
>+                            break starttagloop;
>+                        case SCRIPT:
>+                            startTagScriptInHead(elementName, attributes);
>+                            attributes = null; // CPP
>+                            break starttagloop;
>+                        case STYLE:
>+                            startTagGenericRawText(elementName, attributes);
>+                            attributes = null; // CPP
>+                            break starttagloop;
>+                        case TEMPLATE:
>+                            startTagTemplateInHead(elementName, attributes);
>+                            break starttagloop;

You should be able to do just:
case META:
case LINK_OR_BASEFONT_OR_BGSOUND:
case SCRIPT:
case STYLE:
case TEMPLATE:
  break templatecontentsloop;

Thanks to the loop added per my previous comment.

(Yay goto in disguise.)

>+    private void endTagTemplateInHead(String name) throws SAXException {
>+        int eltPos;
>+        eltPos = findLastInScope(name);
>+        if (eltPos == TreeBuilder.NOT_FOUND_ON_STACK) {
>+            errStrayEndTag(name);
>+        } else {

I think it would read better like the spec if you used early return instead of an "else" block.

>             } else if ("head" == name) {
>-                mode = framesetOk ? FRAMESET_OK : IN_BODY; // really
>+                if (node == contextNode) {
>+                    mode = framesetOk ? FRAMESET_OK : IN_BODY; // really
>+                } else {
>+                    mode = IN_HEAD;
>+                }

So this wasn't good. Spec bug needed for this.
Comment 7 Henri Sivonen (:hsivonen) 2013-02-04 09:14:20 PST
Comment on attachment 708359 [details] [diff] [review]
Modify HTML5 parser to parse 'template' element.

It seems there are still other cases that are parse errors per spec and could use some TODO marker at least.

And with the above comments addressed, r+. Thanks for getting this done.
Comment 8 Henri Sivonen (:hsivonen) 2013-02-04 09:21:42 PST
(In reply to Henri Sivonen (:hsivonen) from comment #6)
> You should be able to do just:
> case META:
> case LINK_OR_BASEFONT_OR_BGSOUND:
> case SCRIPT:
> case STYLE:
> case TEMPLATE:
>   break templatecontentsloop;
> 
> Thanks to the loop added per my previous comment.

Oops. Foster parenting gets in the way. Nevermind.
Comment 9 Henri Sivonen (:hsivonen) 2013-02-04 09:24:54 PST
Though makes me wonder why the spec foster parents those particular elements. Seems useless to foster parent them in the first place. Maybe for consistency.
Comment 10 William Chen [:wchen] 2013-02-05 06:53:17 PST
Created attachment 710185 [details] [diff] [review]
Modify HTML5 parser to parse 'template' element.

(In reply to Henri Sivonen (:hsivonen) from comment #6)
> Comment on attachment 708359 [details] [diff] [review]
> Modify HTML5 parser to parse 'template' element.
> 
> 
> >+                        resetTheInsertionMode();
> >+                        // Reprocess token.
> >+                        continue;
> >+                    }
> 
> >             switch (mode) {
> >+                // https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html#template-contents-insertion-mode
> 
> 
> >+                case TEMPLATE_CONTENTS:
> 
> Let's add
> templatecontentsloop: for (;;) {
> here.
> 
I didn't add it because it doesn't look like I need it anymore after comment 8.
> 
> >             } else if ("head" == name) {
> >-                mode = framesetOk ? FRAMESET_OK : IN_BODY; // really
> >+                if (node == contextNode) {
> >+                    mode = framesetOk ? FRAMESET_OK : IN_BODY; // really
> >+                } else {
> >+                    mode = IN_HEAD;
> >+                }
> 
> So this wasn't good. Spec bug needed for this.

So there is a logic error in my code (node == contextNode). I'm not sure why that rule exists but it does pass the tree builder test suite.

All other comments addressed.

(In reply to Henri Sivonen (:hsivonen) from comment #7)
> Comment on attachment 708359 [details] [diff] [review]
> Modify HTML5 parser to parse 'template' element.
> 
> It seems there are still other cases that are parse errors per spec and
> could use some TODO marker at least.
> 
> And with the above comments addressed, r+. Thanks for getting this done.

I'm not too sure which parse errors you are referring to, but I added some TODOs for the fragment case parse errors in "reset the insertion mode".
Comment 11 William Chen [:wchen] 2013-02-05 06:54:09 PST
Created attachment 710187 [details] [diff] [review]
interdiff
Comment 12 Henri Sivonen (:hsivonen) 2013-02-05 09:36:39 PST
Comment on attachment 710185 [details] [diff] [review]
Modify HTML5 parser to parse 'template' element.

>-    public static final ElementName DIR = new ElementName("dir", "dir", TreeBuilder.ADDRESS_OR_ARTICLE_OR_ASIDE_OR_DETAILS_OR_DIR_OR_FIGCAPTION_OR_FIGURE_OR_FOOTER_OR_HEADER_OR_HGROUP_OR_MAIN_OR_NAV_OR_SECTION_OR_SUMMARY | SPECIAL);
>+    public static final ElementName DIR = new ElementName("dir", "dir", TreeBuilder.ADDRESS_OR_ARTICLE_OR_ASIDE_OR_DETAILS_OR_DIR_OR_FIGCAPTION_OR_FIGURE_OR_FOOTER_OR_HEADER_OR_HGROUP_OR_NAV_OR_SECTION_OR_SUMMARY | SPECIAL);

It looks like this rolls back the name of the constant not to mention "main" as the result of rebasing. This patch should change the name of this constant.
Comment 13 Henri Sivonen (:hsivonen) 2013-02-05 09:42:59 PST
Comment on attachment 710185 [details] [diff] [review]
Modify HTML5 parser to parse 'template' element.

>-    final static int ADDRESS_OR_ARTICLE_OR_ASIDE_OR_DETAILS_OR_DIR_OR_FIGCAPTION_OR_FIGURE_OR_FOOTER_OR_HEADER_OR_HGROUP_OR_MAIN_OR_NAV_OR_SECTION_OR_SUMMARY = 51;
>+    final static int ADDRESS_OR_ARTICLE_OR_ASIDE_OR_DETAILS_OR_DIR_OR_FIGCAPTION_OR_FIGURE_OR_FOOTER_OR_HEADER_OR_HGROUP_OR_NAV_OR_SECTION_OR_SUMMARY = 51;

So this change should not be here.

>+
>+        if (ElementName.TEMPLATE == elementName) {
>+            elt = createDocumentFragmentForTemplate(elt);
>+        }
>+
...
>+    // [NOCPP[
>+    T createDocumentFragmentForTemplate(T template) {
>+      return template;
>+    }
>+    // ]NOCPP]

So per our f2f discussion, this bit isn't needed after all.

r+ with those two things addressed.
Comment 14 William Chen [:wchen] 2013-02-21 09:07:11 PST
Created attachment 716598 [details] [diff] [review]
Modify HTML5 parser to parse 'template' element.

I've addressed the earlier comments. Also, it turns out that I do need to push a different node on the stack because the parser appends into the content while script appends into the template element. I've also updated the implementation to include some recent changes to the spec, updated the tree builder tester to handle the extensions for template content, and fixed some edge cases that I missed.
Comment 15 William Chen [:wchen] 2013-02-21 09:07:38 PST
Created attachment 716600 [details] [diff] [review]
parser interdiff
Comment 16 William Chen [:wchen] 2013-02-21 09:08:32 PST
Created attachment 716602 [details] [diff] [review]
Part 1: Add supplement code and new tree op to support generated code in HTML5 parser.
Comment 17 William Chen [:wchen] 2013-02-21 09:09:18 PST
Created attachment 716604 [details] [diff] [review]
Part 2: Implement template element interface.
Comment 18 William Chen [:wchen] 2013-02-21 09:09:56 PST
Created attachment 716607 [details] [diff] [review]
Part 3: Generated code for HTML5 parser.
Comment 19 William Chen [:wchen] 2013-02-21 09:11:26 PST
Created attachment 716609 [details] [diff] [review]
Part 2: Implement template element interface.
Comment 20 Blake Kaplan (:mrbkap) 2013-03-15 11:32:53 PDT
Comment on attachment 716609 [details] [diff] [review]
Part 2: Implement template element interface.

Review of attachment 716609 [details] [diff] [review]:
-----------------------------------------------------------------

I have a few questions, some nits and one non-nit to fix.

::: content/base/src/Element.cpp
@@ +3144,5 @@
> +GetFirstChildOfTemplateOrNode(nsINode* aNode)
> +{
> +  if (aNode->NodeType() == nsIDOMNode::ELEMENT_NODE &&
> +      static_cast<nsIContent*>(aNode)->IsHTML() &&
> +      aNode->Tag() == nsGkAtoms::_template) {

The pattern: aNode->NodeType() == nsIDOMNode::ELEMENT_NODE && static_cast<nsIContent*>(aNode)->IsHTML() && aNode->Tag() == nsGkAtoms::_template appears a few times throughout this patch. It might be worth making a helper somewhere for it.

@@ +3148,5 @@
> +      aNode->Tag() == nsGkAtoms::_template) {
> +    nsRefPtr<DocumentFragment> frag =
> +      static_cast<HTMLTemplateElement*>(aNode)->Content();
> +    return frag->GetFirstChild();
> +  } else {

Nit: no need for else after return.

@@ +3238,5 @@
>          break;
>        }
>  
>        current = current->GetParentNode();
> +      // Template case, if we are in a template's content, then the parent

Nit: please add a blank line above the comment.

@@ +3358,5 @@
> +  FragmentOrElement* target = this;
> +  // Handle template case.
> +  if (target->NodeType() == nsIDOMNode::ELEMENT_NODE &&
> +      target->IsHTML() && target->Tag() == nsGkAtoms::_template) {
> +    nsRefPtr<DocumentFragment> frag =

Here and in a couple of other cases, you use an nsRefPtr for the result of Content. I'm wondering whether it's necessary or if you can simply use a weak pointer and avoid the extra refcounting.

::: content/base/src/nsContentUtils.cpp
@@ +1844,5 @@
> +  do {
> +    if (aPossibleDescendant == aPossibleAncestor)
> +      return true;
> +    if (aPossibleDescendant->NodeType() ==
> +      nsIDOMNode::DOCUMENT_FRAGMENT_NODE) {

Nit: This wrapping is weird, can it fit on one line? If not, the "n" in nsIDOMNode should be either directly under the "a" or indented to the right of it.

::: content/html/content/src/HTMLTemplateElement.cpp
@@ +42,5 @@
> +HTMLTemplateElement::Init()
> +{
> +  nsIDocument* doc = OwnerDoc();
> +  nsresult rv;
> +  // Used to test if the document "has a browsing context".

I wonder if it might be worth adding a helper on nsIDocument "HasBrowsingContext". I don't know how often this sort of test comes up in HTML5 specs, so if it'll only ever be used in this one place, it probably isn't.

@@ +48,5 @@
> +  if (container) {
> +    rv = NS_NewHTMLDocument(getter_AddRefs(mOwnerDoc));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    mOwnerDoc->SetDocumentURI(doc->GetDocumentURI());

By my reading of the spec, this document should live on OwnerDoc() and be shared by all template elements (https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.html#dfn-template-contents-owner). This appears to make a document per template element.
Comment 21 William Chen [:wchen] 2013-03-18 13:00:20 PDT
Created attachment 726295 [details] [diff] [review]
Part 2: Implement template element interface. v2

(In reply to Blake Kaplan (:mrbkap) from comment #20)
> Comment on attachment 716609 [details] [diff] [review]
> Part 2: Implement template element interface.
> 
> Review of attachment 716609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +3358,5 @@
> > +  FragmentOrElement* target = this;
> > +  // Handle template case.
> > +  if (target->NodeType() == nsIDOMNode::ELEMENT_NODE &&
> > +      target->IsHTML() && target->Tag() == nsGkAtoms::_template) {
> > +    nsRefPtr<DocumentFragment> frag =
> 
> Here and in a couple of other cases, you use an nsRefPtr for the result of
> Content. I'm wondering whether it's necessary or if you can simply use a
> weak pointer and avoid the extra refcounting.
> 
I'm using an nsRefPtr because DocumentFragment returns an already_AddRefed pointer. The alternative is to have another internal method that returns a weak pointer.
> 
> ::: content/html/content/src/HTMLTemplateElement.cpp
> @@ +42,5 @@
> > +HTMLTemplateElement::Init()
> > +{
> > +  nsIDocument* doc = OwnerDoc();
> > +  nsresult rv;
> > +  // Used to test if the document "has a browsing context".
> 
> I wonder if it might be worth adding a helper on nsIDocument
> "HasBrowsingContext". I don't know how often this sort of test comes up in
> HTML5 specs, so if it'll only ever be used in this one place, it probably
> isn't.
> 
I don't think this kind of test is used used elsewhere, apparently "has a browsing context" isn't very well defined but for templates, testing GetContainer() is close enough.
> @@ +48,5 @@
> > +  if (container) {
> > +    rv = NS_NewHTMLDocument(getter_AddRefs(mOwnerDoc));
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    mOwnerDoc->SetDocumentURI(doc->GetDocumentURI());
> 
> By my reading of the spec, this document should live on OwnerDoc() and be
> shared by all template elements
> (https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/templates/index.
> html#dfn-template-contents-owner). This appears to make a document per
> template element.
You are right, I've fixed this.
Comment 22 William Chen [:wchen] 2013-03-18 13:00:47 PDT
Created attachment 726296 [details] [diff] [review]
template element v1 diff v2
Comment 23 Blake Kaplan (:mrbkap) 2013-03-18 14:12:10 PDT
Comment on attachment 726295 [details] [diff] [review]
Part 2: Implement template element interface. v2

Review of attachment 726295 [details] [diff] [review]:
-----------------------------------------------------------------

Asking bz for an additional review to make sure I'm not missing any dumb DOM things.

::: content/base/src/nsDocument.cpp
@@ +8537,5 @@
> +    NS_ENSURE_TRUE(scriptObject || !hasHadScriptObject, nullptr);
> +    mTemplateContentsOwner->SetScriptHandlingObject(scriptObject);
> +  }
> +
> +  return mTemplateContentsOwner.get();

No need for .get().
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2013-03-20 20:30:56 PDT
Comment on attachment 726295 [details] [diff] [review]
Part 2: Implement template element interface. v2

I don't understand either the function name or the comment in nsContentUtils.h.

It needs either a much longer comment to explain what the "host" bit means or a reference to what it means by "host" or something.

As for the function name, is the "Host" in it bound to "Including" or to "Content"?  As in does it mean "Content is a host including descendants of ..." or does it mean "Content is a host-including descendant of ..."?

If the latter, as I suspect based on the implementation, can we talk about flattened tree ancestors instead?  Or is this distinct from that concept?

How did you decide the set of ContentIsDescendantOf callers to convert to the new method?

>+  mozilla::dom::HTMLTemplateElement* GetHost() const

Drop the namespacing.  Just "HTMLTemplateElement".  Same for SetHost.

>+nsDocument::GetTemplateContentsOwner()

The script handling object bits don't make sense to me.

Say someone calls GetTemplateContentsOwner on us.  We have a null scriptObject but hasHadScriptObject is true, so we bail without calling SetScriptHandlingObject.  But the _next_ call we'll happily hand out the mTemplateContentsOwner we just created...  Should we be nulling it out on that failure?

>+    nsresult rv = NS_NewHTMLDocument(getter_AddRefs(mTemplateContentsOwner));

Shouldn't you pass "true" as the second argument?  Or are templates supposed to be able to load external resources?

If these are supposed to be data documents, I strongly urge you to use NS_NewDOMDocument instead of reimplementing the HTML parts of it...

>@@ -557,16 +558,34 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNod

When we're cloning the kids, shouldn't we be cloning them into the nodeinfo manager of the clone's document fragment's owner document, not into "nodeInfoManager"?  And as far as that goes, it's not clear to me what the story there should be with aNewScope....

Also, why do we pass aClone but pass "true" for aDeep?  Just be consistent about the two, please.

>+  return aNode->NodeType() == nsIDOMNode::ELEMENT_NODE &&
>+    static_cast<const nsIContent*>(aNode)->IsHTML() &&
>+    aNode->Tag() == nsGkAtoms::_template;

 return aNode->IsElement() && aNode->AsElement()->IsHTML(nsGkAtoms::_template);

>+  nsCOMPtr<nsIDOMDocumentFragment> frag;
>+  nsresult rv = NS_NewDocumentFragment(getter_AddRefs(frag),
>+                                       contentsOwner->NodeInfoManager());
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  mContent = static_cast<DocumentFragment*>(frag.get());

  ErrorResult rv;
  mContent = contentOwner->CreateDocumentFragment(rv);
  if (rv.Failed()) {
    return rv.ErrorCode();
  }

and then you don't need the extra casting and such.

You probably want to add the HTML_TAG line to the several tests that use those.

I think this needs fixes to the non-innerHTML serializers, right?  In particular, a testcase that uses XMLSerializer on a tree with a <template> would be good to add.  Also innerHTML tests that get innerHTML on the _parent_ of a <template>.

Speaking of which, the test comments say outerHTML is not specified, but it sure seems to be at first glance...
Comment 25 William Chen [:wchen] 2013-03-21 15:32:54 PDT
Created attachment 727925 [details] [diff] [review]
Part 2: Implement template element interface. v3

(In reply to Boris Zbarsky (:bz) from comment #24)
> Comment on attachment 726295 [details] [diff] [review]
> Part 2: Implement template element interface. v2
> 
> I don't understand either the function name or the comment in
> nsContentUtils.h.
> 
> It needs either a much longer comment to explain what the "host" bit means
> or a reference to what it means by "host" or something.
> 
> As for the function name, is the "Host" in it bound to "Including" or to
> "Content"?  As in does it mean "Content is a host including descendants of
> ..." or does it mean "Content is a host-including descendant of ..."?
> 
> If the latter, as I suspect based on the implementation, can we talk about
> flattened tree ancestors instead?  Or is this distinct from that concept?
> 
The concept of the DocumentFragment host and host-including is in a new addition to the DOM spec used to specify an algorithm to prevent cycles with template content. I don't think I want to cross the template content boundary for flattened tree ancestor because it will probably expose the content in places I don't want it to be. But you are right, the function name is a little confusing and I've added more comments to explain what it means.

http://dom.spec.whatwg.org/#concept-tree-host-including-inclusive-ancestor
> How did you decide the set of ContentIsDescendantOf callers to convert to
> the new method?
> 
The DOM spec specifies its use in preinsert and replace algorithms.
> >+nsDocument::GetTemplateContentsOwner()
> 
> The script handling object bits don't make sense to me.
> 
> Say someone calls GetTemplateContentsOwner on us.  We have a null
> scriptObject but hasHadScriptObject is true, so we bail without calling
> SetScriptHandlingObject.  But the _next_ call we'll happily hand out the
> mTemplateContentsOwner we just created...  Should we be nulling it out on
> that failure?
> 
Ooops. I've changed it to nulled it out in the failure case. Alternatively the method could return a document without setting a script handing object but it doesn't seem very useful because it causes assertion failures and exceptions to be thrown whenever you try to access the template contents.
> >+    nsresult rv = NS_NewHTMLDocument(getter_AddRefs(mTemplateContentsOwner));
> 
> Shouldn't you pass "true" as the second argument?  Or are templates supposed
> to be able to load external resources?
> 
> If these are supposed to be data documents, I strongly urge you to use
> NS_NewDOMDocument instead of reimplementing the HTML parts of it...
> 
Done.
> >@@ -557,16 +558,34 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNod
> 
> When we're cloning the kids, shouldn't we be cloning them into the nodeinfo
> manager of the clone's document fragment's owner document, not into
> "nodeInfoManager"?  And as far as that goes, it's not clear to me what the
> story there should be with aNewScope....
> 
That makes sense, I've changed it to clone into the document fragments document nodeinfo manager. What are the consequences with leaving aNewScope the way it is now? I haven't seen anything bad happen yet.
> I think this needs fixes to the non-innerHTML serializers, right?  In
> particular, a testcase that uses XMLSerializer on a tree with a <template>
> would be good to add.  Also innerHTML tests that get innerHTML on the
> _parent_ of a <template>.
> 
Done. Are there other serializers I should be aware of?
> Speaking of which, the test comments say outerHTML is not specified, but it
> sure seems to be at first glance...
The template spec explicitly mentions what to do for innerHTML, but there is no mention of outerHTML.

All other comments addressed.
Comment 26 William Chen [:wchen] 2013-03-21 15:33:18 PDT
Created attachment 727926 [details] [diff] [review]
template element v2 diff v3
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2013-03-21 18:38:02 PDT
Comment on attachment 727925 [details] [diff] [review]
Part 2: Implement template element interface. v3

>   static bool ContentIsHostIncludingDescendantOf(

Thanks for the better comments.  It's much clearer now.

One thought is maybe rephrasing them like this:

  Similar to ContentIsDescendantOf, except will treat an HTMLTemplateElement
  or ShadowRoot as an ancestor of things in the corresponding DocumentFragment.
  See the concept of "host-including inclusive ancestor" in the DOM
  specification.

It's too bad we can't have 'Host-Including' in the function name.  ;)

>+      mTemplateContentsOwner->SetScriptHandlingObject(scriptObject);

Why not do the script object bits before the NS_NewDOMDocument call and then just pass it as the aEventObject?

> The DOM spec specifies its use in preinsert and replace algorithms.

OK, that works.

> Alternatively the method could return a document without setting a script
> handing object

No, returning null makes way more sense to me.

> What are the consequences with leaving aNewScope the way it is now?

I think it's actually fine -- that's used as the scope for wrapper reparenting on adopt, and the template should have the same scope as its contents, I would think.

>+nsNodeUtils::GetFirstChildOfTemplateOrNode(nsINode* aNode)

Does 'frag' here need to be a strong ref?  I don't think it does.

> Done. Are there other serializers I should be aware of?

I assume that nsXHTMLContentSerializer::AppendEndOfElementStart doesn't produce <template/> because the parser service claims <template> to be a container?

Apart from that, I think we should be ok on serializers here....

> The template spec explicitly mentions what to do for innerHTML

It explicitly mentions what to do for _setting_ innerHTML.  The spec also explicitly defines what should happen for getting both innerHTML and outerHTML and any other serialization in the "Serializing HTML Templates" section.

It doesn't seem to do anything special for setting outerHTML, but presumably its parsing changes cover that.

r=me with the nits.
Comment 28 William Chen [:wchen] 2013-03-26 00:19:34 PDT
(In reply to Boris Zbarsky (:bz) from comment #27)
> Comment on attachment 727925 [details] [diff] [review]
> Part 2: Implement template element interface. v3
> >+      mTemplateContentsOwner->SetScriptHandlingObject(scriptObject);
> 
> Why not do the script object bits before the NS_NewDOMDocument call and then
> just pass it as the aEventObject?
Done.
> 
> >+nsNodeUtils::GetFirstChildOfTemplateOrNode(nsINode* aNode)
> 
> Does 'frag' here need to be a strong ref?  I don't think it does.
> 
You're right, it doesn't. I've changed it to return a raw pointer.
> > Done. Are there other serializers I should be aware of?
> 
> I assume that nsXHTMLContentSerializer::AppendEndOfElementStart doesn't
> produce <template/> because the parser service claims <template> to be a
> container?
> 
That's right.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9862de183ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/f704985a8246
https://hg.mozilla.org/integration/mozilla-inbound/rev/833a9f1cce27

Note You need to log in before you can comment on or make changes to this bug.