Fix V8 callback binding for "CallWith=ThisValue" attribute.

Fix V8 callback binding for "CallWith=ThisValue" attribute.

ThisValue should support "any" type other than "object".

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

[xiang, 2014-06-13 03:13:26]

The CQ bit was checked by xiang.long@intel.com

[xiang, 2014-06-13 03:13:27]

The CQ bit was unchecked by xiang.long@intel.com

[haraken, 2014-06-14 03:24:27]

LGTM

https://codereview.chromium.org/330163004/diff/1/Source/bindings/templates/ca...
File Source/bindings/templates/callback_interface.cpp (right):

https://codereview.chromium.org/330163004/diff/1/Source/bindings/templates/ca...
Source/bindings/templates/callback_interface.cpp:63: {% set
this_handle_parameter = 'thisHandle, ' if method.call_with_this_handle else ''
%}

jsbell@ was saying that v8::Handle<v8::Value>::Cast(thisHandle) is needed here
to make some compilers happy. However, I'm not sure why it's needed, so if this
CL passes all builds without the cast, I'm fine with this CL.

[jsbell, 2014-06-14 03:34:57]

On 2014/06/14 03:24:27, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/330163004/diff/1/Source/bindings/templates/ca...
> File Source/bindings/templates/callback_interface.cpp (right):
> 
>
https://codereview.chromium.org/330163004/diff/1/Source/bindings/templates/ca...
> Source/bindings/templates/callback_interface.cpp:63: {% set
> this_handle_parameter = 'thisHandle, ' if method.call_with_this_handle else ''
> %}
> 
> jsbell@ was saying that v8::Handle<v8::Value>::Cast(thisHandle) is needed here
> to make some compilers happy. However, I'm not sure why it's needed, so if
this
> CL passes all builds without the cast, I'm fine with this CL.

Sorry, I don't remember either (and I'm not at a machine I can build with for a
few days now). I kicked off a try job on this CL - we'll see.

Assuming it passes, then I'm all for xiang.long taking credit for the fix and
landing this CL, since it was here first!

(But please include the extra layout test cases from my CL)

[xiang, 2014-06-16 01:30:40]

The CQ bit was checked by xiang.long@intel.com

[xiang, 2014-06-16 01:30:50]

The CQ bit was unchecked by xiang.long@intel.com

[commit-bot: I haz the power, 2014-06-16 01:30:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xiang.long@intel.com/330163004/1

[xwang, 2014-06-16 01:33:59]

The CQ bit was checked by xingnan.wang@chromium.org

[xwang, 2014-06-16 01:34:01]

The CQ bit was unchecked by xingnan.wang@chromium.org

[xwang, 2014-06-16 01:34:32]

The CQ bit was checked by xingnan.wang@chromium.org

[xwang, 2014-06-16 01:34:32]

The CQ bit was unchecked by xingnan.wang@chromium.org

[xiang, 2014-06-16 03:13:42]

On 2014/06/14 03:34:57, jsbell wrote:
> On 2014/06/14 03:24:27, haraken wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/330163004/diff/1/Source/bindings/templates/ca...
> > File Source/bindings/templates/callback_interface.cpp (right):
> > 
> >
>
https://codereview.chromium.org/330163004/diff/1/Source/bindings/templates/ca...
> > Source/bindings/templates/callback_interface.cpp:63: {% set
> > this_handle_parameter = 'thisHandle, ' if method.call_with_this_handle else
''
> > %}
> > 
> > jsbell@ was saying that v8::Handle<v8::Value>::Cast(thisHandle) is needed
here
> > to make some compilers happy. However, I'm not sure why it's needed, so if
> this
> > CL passes all builds without the cast, I'm fine with this CL.
> 
> Sorry, I don't remember either (and I'm not at a machine I can build with for
a
> few days now). I kicked off a try job on this CL - we'll see.
> 
> Assuming it passes, then I'm all for xiang.long taking credit for the fix and
> landing this CL, since it was here first!
> 
> (But please include the extra layout test cases from my CL)

@jsbell @haraken, thanks. I have updated the CL to include these TCs, and an
extra "null" thisValue check TC is added.
Please take a look again.

[haraken, 2014-06-16 05:50:15]

On 2014/06/16 03:13:42, xiang wrote:
> On 2014/06/14 03:34:57, jsbell wrote:
> > On 2014/06/14 03:24:27, haraken wrote:
> > > LGTM
> > > 
> > >
> >
>
https://codereview.chromium.org/330163004/diff/1/Source/bindings/templates/ca...
> > > File Source/bindings/templates/callback_interface.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/330163004/diff/1/Source/bindings/templates/ca...
> > > Source/bindings/templates/callback_interface.cpp:63: {% set
> > > this_handle_parameter = 'thisHandle, ' if method.call_with_this_handle
else
> ''
> > > %}
> > > 
> > > jsbell@ was saying that v8::Handle<v8::Value>::Cast(thisHandle) is needed
> here
> > > to make some compilers happy. However, I'm not sure why it's needed, so if
> > this
> > > CL passes all builds without the cast, I'm fine with this CL.
> > 
> > Sorry, I don't remember either (and I'm not at a machine I can build with
for
> a
> > few days now). I kicked off a try job on this CL - we'll see.
> > 
> > Assuming it passes, then I'm all for xiang.long taking credit for the fix
and
> > landing this CL, since it was here first!
> > 
> > (But please include the extra layout test cases from my CL)
> 
> @jsbell @haraken, thanks. I have updated the CL to include these TCs, and an
> extra "null" thisValue check TC is added.
> Please take a look again.

Looks like the try bots are red.

Also I'm not sure why your message on this issue is not sent to reviewers. Are
you checking "Send mail to reviewers"?

[xiang, 2014-06-16 06:09:48]

On 2014/06/16 05:50:15, haraken wrote:
> Looks like the try bots are red.

I wanted to start these bots but all of them go to red as I don't have the
permission.
Previous Linux try bot started by @jsbell was green. Would you help to kick off
them? Thanks.

> 
> Also I'm not sure why your message on this issue is not sent to reviewers. Are
> you checking "Send mail to reviewers"?

Sorry I can't remember that, I just git-blame one of these files and added two
reviewers from it.
I'll pay my attention next time.

[haraken, 2014-06-16 06:42:49]

On 2014/06/16 06:09:48, xiang wrote:
> On 2014/06/16 05:50:15, haraken wrote:
> > Looks like the try bots are red.
> 
> I wanted to start these bots but all of them go to red as I don't have the
> permission.
> Previous Linux try bot started by @jsbell was green. Would you help to kick
off
> them? Thanks.

Done.

[xiang, 2014-06-17 00:21:43]

The CQ bit was checked by xiang.long@intel.com

[commit-bot: I haz the power, 2014-06-17 00:22:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xiang.long@intel.com/330163004/20001

[commit-bot: I haz the power, 2014-06-17 01:55:01]

Change committed as 176262