Bug 82319 – Remove custom bindings form Internals.idl of attribute type Array.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vineet Chaudhary (vineetc)]

Remove custom bindings for attribute type Array from JavaScriptCallFrame.idl.
Replace attribute [Custom] Array userPreferredLanguages;
To      attribute sequence<String> userPreferredLanguages;

[Vineet Chaudhary (vineetc)]

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.

[Vineet Chaudhary (vineetc)]

(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.

[Kentaro Hara]

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.

[Vineet Chaudhary (vineetc)]

(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.

[Vineet Chaudhary (vineetc)]

Created attachment 134241 [details]
Updated Patch

Updated patch for review comments.

[Kentaro Hara]

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.)

[Vineet Chaudhary (vineetc)]

(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

[Vineet Chaudhary (vineetc)]

Created attachment 134509 [details]
Patch

[Kentaro Hara]

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.)

[Vineet Chaudhary (vineetc)]

Created attachment 134514 [details]
Updated Patch

Re-based  patch fixing efl build issue too.

[Kentaro Hara]

Comment on attachment 134514 [details]
Updated Patch

OK. Thanks for the patch!

[Vineet Chaudhary (vineetc)]

(In reply to comment #11)
> (From update of attachment 134514 [details])
> OK. Thanks for the patch!

Thanks haraken for support.

[WebKit Review Bot]

Comment on attachment 134514 [details]
Updated Patch

Clearing flags on attachment: 134514

Committed r112506: <http://trac.webkit.org/changeset/112506>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.