[webkit-reviews] review denied: [Bug 104181] [HTMLTemplateElement] make content readonly and cloneNode(deep) clone content : [Attachment 177855] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Rafael Weinstein
<rafaelw at chromium.org>'s request for review:
Bug 104181: [HTMLTemplateElement] make content readonly and cloneNode(deep)
clone content
https://bugs.webkit.org/show_bug.cgi?id=104181

Attachment 177855: Patch
https://bugs.webkit.org/attachment.cgi?id=177855&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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/CodeGenera
torV8.pm#L1026 for where you'd teach the CodeGenerator to read this attribute.