Bug 82126 – Remove custom bindings form JavaScriptCallFrame.idl of attribute type Array.

• Status: ASSIGNED
• Component: New Bugs

[Vineet Chaudhary (vineetc)]

Remove custom bindings for attribute type Array from JavaScriptCallFrame.idl.
Replace readonly attribute [CustomGetter] Array scopeChain; 
To         readonly attribute sequence<ScopeChainNode> scopeChain;

[Vineet Chaudhary (vineetc)]

Created attachment 133628 [details]
patch

Patch for review comments.

In bugzilla I found old issue https://bugs.webkit.org/show_bug.cgi?id=38369.
Reiterating the patch here.

[Kentaro Hara]

Comment on attachment 133628 [details]
patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=82126
> +        Remove custom bindings form JavaScriptCallFrame.idl of attribute type Array.

Nit: Please exchange the two lines. The title, and then the bug link.

> Source/WebCore/bindings/js/JSDOMBinding.h:311
> +    // We use this version for JavaScriptCallFrame.scopeChain.
> +    JSC::JSValue jsArray(JSC::ExecState*, JSDOMGlobalObject*, JSC::ScopeChainNode*);

Why can't we use the existing jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Iterable& iterator)? If possible, we do not want to add ScopeChainNode-specific code.

[Vineet Chaudhary (vineetc)]

(In reply to comment #2)
> (From update of attachment 133628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133628&action=review
> > Source/WebCore/bindings/js/JSDOMBinding.h:311
> > +    // We use this version for JavaScriptCallFrame.scopeChain.
> > +    JSC::JSValue jsArray(JSC::ExecState*, JSDOMGlobalObject*, JSC::ScopeChainNode*);
> 
> Why can't we use the existing jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Iterable& iterator)? If possible, we do not want to add ScopeChainNode-specific code.

Actually I tried that but for other bindings the iterator type was Vector in this case it is of type ScopeChainIterator defined in ScopeChain.h.

It would give error : 
JSDOMBinding.h:287: error: ‘JSC::ScopeChainNode*’ is not a class, struct, or union type

[Kentaro Hara]

(In reply to comment #3)
> > Why can't we use the existing jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Iterable& iterator)? If possible, we do not want to add ScopeChainNode-specific code.
> 
> Actually I tried that but for other bindings the iterator type was Vector in this case it is of type ScopeChainIterator defined in ScopeChain.h.

OK, is this pattern used by ScopeChainIterator only? If so, I am afraid that the patch doesn't make much progression. It just removes custom bindings and instead adds special code for ScopeChainIterator to code generators. In total, it seems the change does not remove any code. Maybe we can keep ScopeChain being custom bindings, unless there is more use cases of the pattern.

> for other bindings the iterator type was Vector

Shall we try to replace custom bindings with sequence<T> for the "other bindings" first?

[Vineet Chaudhary (vineetc)]

(In reply to comment #4)
> (In reply to comment #3)
> > > Why can't we use the existing jsArray(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const Iterable& iterator)? If possible, we do not want to add ScopeChainNode-specific code.
> > 
> > Actually I tried that but for other bindings the iterator type was Vector in this case it is of type ScopeChainIterator defined in ScopeChain.h.
> 
> OK, is this pattern used by ScopeChainIterator only? If so, I am afraid that the patch doesn't make much progression. It just removes custom bindings and instead adds special code for ScopeChainIterator to code generators. In total, it seems the change does not remove any code. Maybe we can keep ScopeChain being custom bindings, unless there is more use cases of the pattern.
> 

Thanks haraken for comments.
Unfortunately this pattern is only used by ScopeChainIterator only :(. 

> > for other bindings the iterator type was Vector
> Shall we try to replace custom bindings with sequence<T> for the "other bindings" first?

ok we will come back to this later.
Remaining custom bindings are Internals(Vector of Strings) and Clipboard(HashSet of Strings).

For MessagePort there is already bug logged https://bugs.webkit.org/show_bug.cgi?id=66829 .