Bug 104181 – [HTMLTemplateElement] make content readonly and cloneNode(deep) clone content

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

[Rafael Weinstein]

e.g. Implement https://www.w3.org/Bugs/Public/show_bug.cgi?id=16277, http://dvcs.w3.org/hg/webcomponents/rev/96d3865c980d

[Rafael Weinstein]

Created attachment 177855 [details]
Patch

[Adam Barth]

Comment on attachment 177855 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8HTMLTemplateElementCustom.cpp:48
> +    v8::Handle<v8::Value> wrapper = v8::Handle<v8::Value>(DOMDataStore::current(info.GetIsolate())->get(content));
> +    if (wrapper.IsEmpty())
> +        wrapper = toV8(content, info.Holder(), info.GetIsolate());

Why not just call toV8 in the first place?  toV8 checks DOMDataStore for you.

> Source/WebCore/bindings/v8/custom/V8HTMLTemplateElementCustom.cpp:51
> +        V8DOMWrapper::setNamedHiddenReference(info.Holder(), "content", wrapper);

We should add an IDL attribute that does this work.  The code generator already knows how to do all of this.

> Source/WebCore/html/HTMLTemplateElement.cpp:77
> +    return clone;

clone.release()

> Source/WebCore/html/HTMLTemplateElement.idl:34
> -    attribute DocumentFragment content setter raises (DOMException);
> +    [Custom] readonly attribute DocumentFragment content;

[CacheAttributeForGC] ?  See http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm#L1026 for where you'd teach the CodeGenerator to read this attribute.

[Adam Barth]

This looks good, but I think we should teach the code generator how to do it since it's a one-line change to the code generator and it avoids the custom bindings.

[Adam Klein]

Created attachment 178058 [details]
Patch

[Adam Klein]

Comment on attachment 177855 [details]
Patch

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

Taking this patch over for raf

>> Source/WebCore/html/HTMLTemplateElement.cpp:77
>> +    return clone;
> 
> clone.release()

Done

>> Source/WebCore/html/HTMLTemplateElement.idl:34
>> +    [Custom] readonly attribute DocumentFragment content;
> 
> [CacheAttributeForGC] ?  See http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm#L1026 for where you'd teach the CodeGenerator to read this attribute.

Done. That if statement is now completely crazy (since this new attribute is totally disjoint from all the other tests), but you're right that it does saving that code into a custom file.

I take it you're ok with the custom approach for JSC? I don't think they have any codegen to handle this, though the normal approach would be to use [JSCustomIsReachable] on the DocumentFragment.

[Adam Barth]

Comment on attachment 178058 [details]
Patch

Great.  Thanks.

[WebKit Review Bot]

Comment on attachment 178058 [details]
Patch

Rejecting attachment 178058 [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:
commit-queue/Source/WebKit/chromium/third_party/v8-i18n --revision 159 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
52>At revision 159.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

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

[Adam Klein]

Created attachment 178088 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 178088 [details]
Patch for landing

Clearing flags on attachment: 178088

Committed r136903: <http://trac.webkit.org/changeset/136903>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rafael Weinstein]

Thanks for finishing this.