Bug 104181 - [HTMLTemplateElement] make content readonly and cloneNode(deep) clone content
Summary: [HTMLTemplateElement] make content readonly and cloneNode(deep) clone content
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks: 103547
  Show dependency treegraph
 
Reported: 2012-12-05 16:00 PST by Rafael Weinstein
Modified: 2012-12-06 19:05 PST (History)
10 users (show)

See Also:


Attachments
Patch (30.54 KB, patch)
2012-12-05 16:07 PST, Rafael Weinstein
no flags Details | Formatted Diff | Diff
Patch (29.68 KB, patch)
2012-12-06 12:19 PST, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (29.05 KB, patch)
2012-12-06 15:09 PST, Adam Klein
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-12-05 16:07:53 PST
Created attachment 177855 [details]
Patch
Comment 2 Adam Barth 2012-12-05 18:31:28 PST
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.
Comment 3 Adam Barth 2012-12-05 18:32:04 PST
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.
Comment 4 Adam Klein 2012-12-06 12:19:42 PST
Created attachment 178058 [details]
Patch
Comment 5 Adam Klein 2012-12-06 12:20:13 PST
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.
Comment 6 Adam Barth 2012-12-06 12:40:56 PST
Comment on attachment 178058 [details]
Patch

Great.  Thanks.
Comment 7 WebKit Review Bot 2012-12-06 13:35:04 PST
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
Comment 8 Adam Klein 2012-12-06 15:09:22 PST
Created attachment 178088 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-12-06 15:48:53 PST
Comment on attachment 178088 [details]
Patch for landing

Clearing flags on attachment: 178088

Committed r136903: <http://trac.webkit.org/changeset/136903>
Comment 10 WebKit Review Bot 2012-12-06 15:48:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Rafael Weinstein 2012-12-06 19:05:47 PST
Thanks for finishing this.