Bug 82319

Summary: Remove custom bindings form Internals.idl of attribute type Array.
Product: WebKit Reporter: Vineet Chaudhary (vineetc) <code.vineet>
Component: New BugsAssignee: Vineet Chaudhary (vineetc) <code.vineet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric.carlson, haraken, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 82442    
Bug Blocks: 84540    
Attachments:
Description Flags
proposed patch
none
Updated Patch
haraken: review-
Patch
none
Updated Patch none

Description Vineet Chaudhary (vineetc) 2012-03-27 04:17:51 PDT
Remove custom bindings for attribute type Array from JavaScriptCallFrame.idl.
Replace attribute [Custom] Array userPreferredLanguages;
To      attribute sequence<String> userPreferredLanguages;
Comment 1 Vineet Chaudhary (vineetc) 2012-03-27 04:35:50 PDT
Created attachment 134024 [details]
proposed patch

Attaching proposed patch to remove custom bindings from Internals.idl.

Here I have added :: template<> inline JSC::JSValue jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Vector<String>& iterator) 
For string type for the reasons :: String types needs jsString() rather than toJS() where jsString() takes String parameter.

If we add specialize function template for String it would be also helpful in removing bindings from Clipboard.idl too.

Please let me know review comments on this. If this patch looks oke I will remove JSInternalsCustom.cpp/.h in next patch.
Comment 2 Vineet Chaudhary (vineetc) 2012-03-27 04:43:12 PDT
(In reply to comment #0)
> Remove custom bindings for attribute type Array from JavaScriptCallFrame.idl.
Again Copy Paste Error :( . Should be 
Remove custom bindings for attribute type Array from Internals.idl.
Comment 3 Kentaro Hara 2012-03-27 05:03:15 PDT
Comment on attachment 134024 [details]
proposed patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:283
> +    inline JSC::JSValue jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Iterable& iterator)

Why do you want to make this inline? Did you confirm any performance gain? (Otherwise we do not want to inline methods since it just increases the code size, which might result in high cache miss rate.)

Also, shall we change Iterable to Vector, for consistency with JSC::JSValue jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Vector<String>& iterator)?

> Source/WebCore/bindings/js/JSDOMBinding.h:298
> +        if (iterator.isEmpty())
> +            return JSC::jsNull();

Generally speaking, I think this is wrong. If possible, we do not want to write this here, since jsArray() should be a generalized implementation. Is it really an expected behavior for Internals.idl to return null when the vector is empty? (If it is not an expected behavior, we can fix the behavior first.)

> Source/WebCore/bindings/v8/V8Binding.h:304
> +        if (iterator.isEmpty())
> +            return v8::Null();

Ditto.
Comment 4 Vineet Chaudhary (vineetc) 2012-03-28 01:41:25 PDT
(In reply to comment #3)
> (From update of attachment 134024 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134024&action=review
> 
> > Source/WebCore/bindings/js/JSDOMBinding.h:283
> > +    inline JSC::JSValue jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Iterable& iterator)
> 
> Why do you want to make this inline? Did you confirm any performance gain? (Otherwise we do not want to inline methods since it just increases the code size, which might result in high cache miss rate.)

Here it has 3 inline functions 
1) template <typename T> inline JSC::JSValue jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Vector<T>& iterator)
2) template <class T> inline Vector<T> toNativeArray(JSC::ExecState* exec, JSC::JSValue value)

I tried running Dromaeo performance test but there is no significant difference. So we can removing inline.

3) template<> inline JSC::JSValue jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Vector<String>& iterator)
This still needs to have inline because of "One definition rule."

> Also, shall we change Iterable to Vector, for consistency with JSC::JSValue jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Vector<String>& iterator)?
Done.

> > Source/WebCore/bindings/js/JSDOMBinding.h:298
> > +        if (iterator.isEmpty())
> > +            return JSC::jsNull();
> 
> Generally speaking, I think this is wrong. If possible, we do not want to write this here, since jsArray() should be a generalized implementation. Is it really an expected behavior for Internals.idl to return null when the vector is empty? (If it is not an expected behavior, we can fix the behavior first.)
> 
> > Source/WebCore/bindings/v8/V8Binding.h:304
> > +        if (iterator.isEmpty())
> > +            return v8::Null();
> 
> Ditto.

I tried finding specification for Internals.idl but could not find on w3c. But you are right jsArray() should be a generalized implementation, also as per comment https://bugs.webkit.org/show_bug.cgi?id=80696#c35 we should remove this.

I will upload patch with these comments thanks.
Comment 5 Vineet Chaudhary (vineetc) 2012-03-28 02:15:04 PDT
Created attachment 134241 [details]
Updated Patch

Updated patch for review comments.
Comment 6 Kentaro Hara 2012-03-28 02:28:19 PDT
Comment on attachment 134241 [details]
Updated Patch

The change looks OK to me, but this patch changes the existing behavior of Internals. First, we need to reach consensus on removing jsNull()/v8::Null() with Internals folks.

Option1: We can ask it in this bug.
Option2: You can make a patch that simply removes jsNull()/v8::Null() from JSInternalsCustom.cpp/V8InternalsCustom.cpp as another bug. It will update some layout tests, because it changes the existing behavior. After the patch is landed, you can commit this patch.

Both options are fine (but I would prefer Option2.)
Comment 7 Vineet Chaudhary (vineetc) 2012-03-28 03:09:34 PDT
(In reply to comment #6)
> (From update of attachment 134241 [details])
> The change looks OK to me, but this patch changes the existing behavior of Internals. First, we need to reach consensus on removing jsNull()/v8::Null() with Internals folks.
> 
> Option1: We can ask it in this bug.
> Option2: You can make a patch that simply removes jsNull()/v8::Null() from JSInternalsCustom.cpp/V8InternalsCustom.cpp as another bug. It will update some layout tests, because it changes the existing behavior. After the patch is landed, you can commit this patch.
> 
> Both options are fine (but I would prefer Option2.)

Thanks haraken for suggestins.
I have Filed bug https://bugs.webkit.org/show_bug.cgi?id=82442
Comment 8 Vineet Chaudhary (vineetc) 2012-03-28 23:46:28 PDT
Created attachment 134509 [details]
Patch
Comment 9 Kentaro Hara 2012-03-28 23:58:49 PDT
Comment on attachment 134509 [details]
Patch

The patch looks OK. Please rebase your patch with the latest WebKit trunk. (It seems that your previous patch is not yet applied.)
Comment 10 Vineet Chaudhary (vineetc) 2012-03-29 00:24:48 PDT
Created attachment 134514 [details]
Updated Patch

Re-based  patch fixing efl build issue too.
Comment 11 Kentaro Hara 2012-03-29 00:26:14 PDT
Comment on attachment 134514 [details]
Updated Patch

OK. Thanks for the patch!
Comment 12 Vineet Chaudhary (vineetc) 2012-03-29 00:31:59 PDT
(In reply to comment #11)
> (From update of attachment 134514 [details])
> OK. Thanks for the patch!

Thanks haraken for support.
Comment 13 WebKit Review Bot 2012-03-29 01:33:06 PDT
Comment on attachment 134514 [details]
Updated Patch

Clearing flags on attachment: 134514

Committed r112506: <http://trac.webkit.org/changeset/112506>
Comment 14 WebKit Review Bot 2012-03-29 01:33:13 PDT
All reviewed patches have been landed.  Closing bug.