Bug 86031 - [HTMLTemplateElement] Implement DOM element and parser changes
Summary: [HTMLTemplateElement] Implement DOM element and parser changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rafael Weinstein
URL:
Keywords:
: 78734 (view as bug list)
Depends on: 103966
Blocks: 103547
  Show dependency treegraph
 
Reported: 2012-05-09 16:15 PDT by Rafael Weinstein
Modified: 2013-02-11 07:24 PST (History)
26 users (show)

See Also:


Attachments
Patch (61.08 KB, patch)
2012-05-09 16:21 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (61.04 KB, patch)
2012-05-10 09:28 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (31.25 KB, patch)
2012-05-10 10:07 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (32.41 KB, patch)
2012-05-17 11:52 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (75.79 KB, patch)
2012-10-17 11:07 PDT, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (76.71 KB, patch)
2012-11-13 12:02 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (73.95 KB, patch)
2012-11-13 18:02 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (76.68 KB, patch)
2012-11-15 17:37 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (79.70 KB, patch)
2012-11-28 10:47 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (80.30 KB, patch)
2012-11-28 11:29 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (92.19 KB, patch)
2012-11-28 17:29 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (95.89 KB, patch)
2012-11-29 12:28 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (95.94 KB, patch)
2012-11-29 12:44 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (95.97 KB, patch)
2012-11-29 13:51 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (96.06 KB, patch)
2012-11-29 17:55 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (96.20 KB, patch)
2012-11-30 10:32 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (93.79 KB, patch)
2012-12-01 08:57 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (94.60 KB, patch)
2012-12-03 14:27 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch for landing (94.62 KB, patch)
2012-12-03 17:33 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Rafael Weinstein 2012-05-09 16:21:56 PDT
Created attachment 141044 [details]
Patch
Comment 2 Rafael Weinstein 2012-05-09 16:26:46 PDT
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.
Comment 3 Adam Klein 2012-05-09 18:10:56 PDT
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.
Comment 4 Build Bot 2012-05-09 18:36:35 PDT
Comment on attachment 141044 [details]
Patch

Attachment 141044 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12646835
Comment 5 Early Warning System Bot 2012-05-09 18:59:05 PDT
Comment on attachment 141044 [details]
Patch

Attachment 141044 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12648733
Comment 6 Early Warning System Bot 2012-05-09 18:59:39 PDT
Comment on attachment 141044 [details]
Patch

Attachment 141044 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12648734
Comment 7 Gyuyoung Kim 2012-05-09 20:59:02 PDT
Comment on attachment 141044 [details]
Patch

Attachment 141044 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12665141
Comment 8 Hajime Morrita 2012-05-10 00:11:20 PDT
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.
Comment 9 Ryosuke Niwa 2012-05-10 01:07:45 PDT
(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.
Comment 10 Rafael Weinstein 2012-05-10 09:28:36 PDT
Created attachment 141182 [details]
Patch
Comment 11 Rafael Weinstein 2012-05-10 10:07:22 PDT
Created attachment 141189 [details]
Patch
Comment 12 Rafael Weinstein 2012-05-10 10:08:30 PDT
Patch now contains diffs relative to https://bugs.webkit.org/show_bug.cgi?id=84646 (thanks, Arv)
Comment 13 Dimitri Glazkov (Google) 2012-05-10 11:43:43 PDT
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?
Comment 14 Rafael Weinstein 2012-05-17 11:52:38 PDT
Created attachment 142518 [details]
Patch
Comment 15 Rafael Weinstein 2012-05-17 11:56:36 PDT
Comments addressed. Also, this patch now depends on the new Document.parse() patch (still https://bugs.webkit.org/show_bug.cgi?id=84646).
Comment 16 Dimitri Glazkov (Google) 2012-05-17 12:18:38 PDT
(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!
Comment 17 Ojan Vafai 2012-05-17 23:42:53 PDT
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.
Comment 18 Adam Klein 2012-05-23 15:37:15 PDT
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).
Comment 19 Ryosuke Niwa 2012-05-23 17:28:48 PDT
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.
Comment 20 Rafael Weinstein 2012-10-17 11:07:53 PDT
Created attachment 169219 [details]
Patch
Comment 21 Rafael Weinstein 2012-10-17 11:14:53 PDT
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.
Comment 22 Dimitri Glazkov (Google) 2012-10-17 11:21:09 PDT
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?
Comment 23 Rafael Weinstein 2012-10-17 11:30:39 PDT
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
Comment 24 Rafael Weinstein 2012-10-18 13:45:40 PDT
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
Comment 25 Rafael Weinstein 2012-11-13 12:02:03 PST
Created attachment 173943 [details]
Patch
Comment 26 Rafael Weinstein 2012-11-13 18:02:56 PST
Created attachment 174042 [details]
Patch
Comment 27 Early Warning System Bot 2012-11-13 18:15:12 PST
Comment on attachment 174042 [details]
Patch

Attachment 174042 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14832143
Comment 28 Build Bot 2012-11-13 18:41:34 PST
Comment on attachment 174042 [details]
Patch

Attachment 174042 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14832150
Comment 29 EFL EWS Bot 2012-11-13 18:56:46 PST
Comment on attachment 174042 [details]
Patch

Attachment 174042 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14688537
Comment 30 Adam Barth 2012-11-13 23:13:45 PST
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?
Comment 31 Adam Barth 2012-11-13 23:15:24 PST
The patch generally looks good.  I'd like Eric Seidel to review the patch as well (perhaps for the next iteration).
Comment 32 Adam Barth 2012-11-13 23:16:27 PST
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?
Comment 33 Adam Klein 2012-11-14 07:55:13 PST
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).
Comment 34 Adam Barth 2012-11-14 08:48:12 PST
> 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++.
Comment 35 Adam Barth 2012-11-14 12:13:25 PST
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.
Comment 36 Rafael Weinstein 2012-11-15 17:37:27 PST
Created attachment 174571 [details]
Patch
Comment 37 Rafael Weinstein 2012-11-15 17:38:47 PST
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?
Comment 38 Rafael Weinstein 2012-11-15 17:39:03 PST
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.
Comment 39 Adam Barth 2012-11-15 17:52:53 PST
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.
Comment 40 Early Warning System Bot 2012-11-15 18:31:48 PST
Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14846659
Comment 41 Early Warning System Bot 2012-11-15 18:32:26 PST
Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14862002
Comment 42 kov's GTK+ EWS bot 2012-11-15 18:43:45 PST
Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14860081
Comment 43 EFL EWS Bot 2012-11-15 19:42:05 PST
Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14857467
Comment 44 Build Bot 2012-11-15 21:35:03 PST
Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14843879
Comment 45 Build Bot 2012-11-16 12:13:14 PST
Comment on attachment 174571 [details]
Patch

Attachment 174571 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14861295
Comment 46 Eric Seidel (no email) 2012-11-19 10:17:23 PST
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?
Comment 47 Eric Seidel (no email) 2012-11-19 14:51:30 PST
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?
Comment 48 Rafael Weinstein 2012-11-28 10:47:10 PST
Created attachment 176518 [details]
Patch
Comment 49 Early Warning System Bot 2012-11-28 11:01:22 PST
Comment on attachment 176518 [details]
Patch

Attachment 176518 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15003937
Comment 50 Early Warning System Bot 2012-11-28 11:04:46 PST
Comment on attachment 176518 [details]
Patch

Attachment 176518 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15018519
Comment 51 EFL EWS Bot 2012-11-28 11:07:50 PST
Comment on attachment 176518 [details]
Patch

Attachment 176518 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15026506
Comment 52 Rafael Weinstein 2012-11-28 11:29:22 PST
Created attachment 176531 [details]
Patch
Comment 53 Rafael Weinstein 2012-11-28 11:30:16 PST
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
Comment 54 Rafael Weinstein 2012-11-28 11:33:42 PST
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.
Comment 55 Rafael Weinstein 2012-11-28 11:34:17 PST
(p.s. will obvious fix all builds before landing).
Comment 56 Early Warning System Bot 2012-11-28 11:38:55 PST
Comment on attachment 176531 [details]
Patch

Attachment 176531 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15003951
Comment 57 Early Warning System Bot 2012-11-28 11:41:55 PST
Comment on attachment 176531 [details]
Patch

Attachment 176531 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15031341
Comment 58 EFL EWS Bot 2012-11-28 12:05:00 PST
Comment on attachment 176531 [details]
Patch

Attachment 176531 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15031345
Comment 59 Rafael Weinstein 2012-11-28 12:30:25 PST
*** Bug 78734 has been marked as a duplicate of this bug. ***
Comment 60 Build Bot 2012-11-28 12:53:37 PST
Comment on attachment 176531 [details]
Patch

Attachment 176531 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15018563
Comment 61 Build Bot 2012-11-28 12:58:29 PST
Comment on attachment 176531 [details]
Patch

Attachment 176531 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15027525
Comment 62 Rafael Weinstein 2012-11-28 17:29:57 PST
Created attachment 176611 [details]
Patch
Comment 63 Rafael Weinstein 2012-11-28 17:30:31 PST
Hopefully this patch fixes all builds.
Comment 64 Build Bot 2012-11-28 18:47:43 PST
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
Comment 65 Build Bot 2012-11-28 19:52:48 PST
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
Comment 66 Rafael Weinstein 2012-11-29 12:28:58 PST
Created attachment 176788 [details]
Patch
Comment 67 WebKit Review Bot 2012-11-29 12:33:14 PST
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.
Comment 68 Rafael Weinstein 2012-11-29 12:44:30 PST
Created attachment 176791 [details]
Patch
Comment 69 Rafael Weinstein 2012-11-29 13:51:52 PST
Created attachment 176811 [details]
Patch
Comment 70 Rafael Weinstein 2012-11-29 17:55:45 PST
Created attachment 176870 [details]
Patch
Comment 71 Rafael Weinstein 2012-11-30 10:32:53 PST
Created attachment 176990 [details]
Patch
Comment 72 EFL EWS Bot 2012-11-30 11:21:00 PST
Comment on attachment 176990 [details]
Patch

Attachment 176990 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15057441
Comment 73 Adam Barth 2012-11-30 13:47:52 PST
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.
Comment 74 Rafael Weinstein 2012-12-01 08:57:00 PST
Created attachment 177109 [details]
Patch
Comment 75 Rafael Weinstein 2012-12-01 08:58:14 PST
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.
Comment 76 EFL EWS Bot 2012-12-01 09:38:29 PST
Comment on attachment 177109 [details]
Patch

Attachment 177109 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15061863
Comment 77 Adam Barth 2012-12-03 14:04:51 PST
Comment on attachment 177109 [details]
Patch

Looks like you still have an EFL build issue to resolve, but this seems ready to land.
Comment 78 Rafael Weinstein 2012-12-03 14:27:18 PST
Created attachment 177330 [details]
Patch
Comment 79 Rafael Weinstein 2012-12-03 17:33:14 PST
Created attachment 177381 [details]
Patch for landing
Comment 80 WebKit Review Bot 2012-12-03 18:30:31 PST
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
Comment 81 WebKit Review Bot 2012-12-03 19:10:45 PST
Comment on attachment 177330 [details]
Patch

Clearing flags on attachment: 177330

Committed r136467: <http://trac.webkit.org/changeset/136467>
Comment 82 WebKit Review Bot 2012-12-03 19:10:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 83 WebKit Review Bot 2012-12-03 20:37:43 PST
Re-opened since this is blocked by bug 103966