[bindings] Support CallWith=ThisValue for generic interface methods

[bindings] Support CallWith=ThisValue for generic interface methods

As per [1], the extended attribute, CallWith=ThisValue should be only used with a callback interface method.

We need to support the CallWith=ThisValue on a generic interface method for Location.idl which will be done in the followup patch.

BUG=345519
R=haraken@chromium.org, jl@opera.com

[1] https://sites.google.com/a/chromium.org/dev/blink/webidl/blink-idl-extended-attributes#TOC-CallWith-ThisValue-m-

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192831

[vivekg, 2015-03-31 06:26:15]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-03-31 06:26:16]

This is a WIP patch and would like to know your opinion about supporting
CallWith=ThisValue for generic interface methods. PTAL and let me know your
comments. Thanks.

[vivekg, 2015-03-31 09:29:49]

On 2015/03/31 06:26:16, vivekg_ wrote:
> This is a WIP patch and would like to know your opinion about supporting
> CallWith=ThisValue for generic interface methods. PTAL and let me know your
> comments. Thanks.

If agreed, we can split this into two separate patches: one for binding support
and other for location custom binding removal. WDYT?

[haraken, 2015-03-31 09:35:22]

https://codereview.chromium.org/1035023005/diff/1/Source/bindings/core/v8/cus...
File Source/bindings/core/v8/custom/V8LocationCustom.cpp (left):

https://codereview.chromium.org/1035023005/diff/1/Source/bindings/core/v8/cus...
Source/bindings/core/v8/custom/V8LocationCustom.cpp:42: // conversion behavior
of location objects.

Keep this comment.

https://codereview.chromium.org/1035023005/diff/1/Source/bindings/tests/resul...
File Source/bindings/tests/results/core/V8TestObject.cpp (right):

https://codereview.chromium.org/1035023005/diff/1/Source/bindings/tests/resul...
Source/bindings/tests/results/core/V8TestObject.cpp:9974:
impl->callWithThisValue(ScriptValue(scriptState, info.This()));

Help me understand: Where is the 'ScriptValue(scriptState, info.This())'
generated? I'm wondering this because I couldn't find
method.is_call_with_this_value in methods.cpp (other than the place that
generates ScriptState::current()).

[vivekg, 2015-03-31 09:47:46]

On 2015/03/31 at 09:35:22, haraken wrote:
>
https://codereview.chromium.org/1035023005/diff/1/Source/bindings/core/v8/cus...
> File Source/bindings/core/v8/custom/V8LocationCustom.cpp (left):
> 
>
https://codereview.chromium.org/1035023005/diff/1/Source/bindings/core/v8/cus...
> Source/bindings/core/v8/custom/V8LocationCustom.cpp:42: // conversion behavior
of location objects.
> 
> Keep this comment.

Will do, thanks. 

> 
>
https://codereview.chromium.org/1035023005/diff/1/Source/bindings/tests/resul...
> File Source/bindings/tests/results/core/V8TestObject.cpp (right):
> 
>
https://codereview.chromium.org/1035023005/diff/1/Source/bindings/tests/resul...
> Source/bindings/tests/results/core/V8TestObject.cpp:9974:
impl->callWithThisValue(ScriptValue(scriptState, info.This()));
> 
> Help me understand: Where is the 'ScriptValue(scriptState, info.This())'
generated? I'm wondering this because I couldn't find
method.is_call_with_this_value in methods.cpp (other than the place that
generates ScriptState::current()).

This is indeed coming from cpp_value conversions from here
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[vivekg, 2015-03-31 09:56:34]

Done the changes. PTAL.

[Jens Widell, 2015-03-31 11:15:43]

This looks good to me, but would probably be better split into two CLs.

Support for CallWith=ThisValue on regular methods was actually added by
https://codereview.chromium.org/807263007 and is used for the forEach() method
of iterable/maplike/setlike interfaces.

That fix was incomplete, though, and only really works when combined as
CallWith=(ScriptState, ThisValue). Thanks for fixing that!

[vivekg, 2015-03-31 11:33:43]

On 2015/03/31 at 11:15:43, jl wrote:
> This looks good to me, but would probably be better split into two CLs.
> 
> Support for CallWith=ThisValue on regular methods was actually added by
https://codereview.chromium.org/807263007 and is used for the forEach() method
of iterable/maplike/setlike interfaces.
> 
> That fix was incomplete, though, and only really works when combined as
CallWith=(ScriptState, ThisValue). Thanks for fixing that!

Thanks for your feedback. Splitting the patch into two. Location patch will
followup.

[vivekg, 2015-03-31 13:27:52]

Jens, good to go?

[haraken, 2015-03-31 13:33:27]

LGTM

[vivekg, 2015-03-31 13:37:14]

The CQ bit was checked by vivekg@chromium.org

[vivekg, 2015-03-31 13:37:28]

On 2015/03/31 at 13:33:27, haraken wrote:
> LGTM

Thanks ;)

[commit-bot: I haz the power, 2015-03-31 13:37:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1035023005/40001

[commit-bot: I haz the power, 2015-03-31 13:41:19]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192831

[Jens Widell, 2015-03-31 14:05:42]

LGTM