Summary: | Remove custom bindings form Internals.idl of attribute type Array. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||||||
Component: | New Bugs | Assignee: | 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
Vineet Chaudhary (vineetc)
2012-03-27 04:17:51 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.
(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 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. (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. Created attachment 134241 [details]
Updated Patch
Updated patch for review comments.
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.)
(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 Created attachment 134509 [details]
Patch
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.)
Created attachment 134514 [details]
Updated Patch
Re-based patch fixing efl build issue too.
Comment on attachment 134514 [details]
Updated Patch
OK. Thanks for the patch!
(In reply to comment #11) > (From update of attachment 134514 [details]) > OK. Thanks for the patch! Thanks haraken for support. Comment on attachment 134514 [details] Updated Patch Clearing flags on attachment: 134514 Committed r112506: <http://trac.webkit.org/changeset/112506> All reviewed patches have been landed. Closing bug. |