bindings: Throw a type error if converting an interface or callback function fails.

bindings: Throw a type error if converting an interface or callback function fails.

The previous implementation of nativeValue() for IDL interfaces and callback
functions simply called implTypeWithCheck() (or create() for callback
functions) and returned its value. This meant it was not possible for
callers to easily distinguish a nullptr returned here (meaning the
conversion failed) from nullptr being returned by a nativeValue() call for
another type (which can just be a nullptr).

BUG=690428
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2802223002
Cr-Commit-Position: refs/heads/master@{#462890}
Committed: https://chromium.googlesource.com/chromium/src/+/125a3532433159edc1041c7f031d733df0c7682d

[Raphael Kubo da Costa (rakuco), 2017-04-07 13:25:40]

PTAL. This is part of the long road to solving bug 690428.

[Raphael Kubo da Costa (rakuco), 2017-04-07 13:25:50]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 13:26:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 13:27:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 13:27:20]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Yuki, 2017-04-07 13:37:21]

LGTM.

https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
(right):

https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:86:
exceptionState.throwTypeError("Unable to convert value to {{cpp_class}}.");
nit: Can we use {{idl_type}} instead?
I'm not 100% sure if {{idl_type}} is appropriate, but my point is that we should
use an IDL name rather than an internal C++ class' name because this is a
user-visible message.

https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:897:
exceptionState.throwTypeError("Unable to convert value to {{cpp_class}}.");
nit: Can we use {{interface_name}} instead?
This is a user-visible message, so an internal C++ class' name is not preferred.

[Raphael Kubo da Costa (rakuco), 2017-04-07 13:52:30]

On 2017/04/07 13:37:21, Yuki wrote:
> LGTM.
> 
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
> (right):
> 
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:86:
> exceptionState.throwTypeError("Unable to convert value to {{cpp_class}}.");
> nit: Can we use {{idl_type}} instead?
> I'm not 100% sure if {{idl_type}} is appropriate, but my point is that we
should
> use an IDL name rather than an internal C++ class' name because this is a
> user-visible message.
> 
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):
> 
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:897:
> exceptionState.throwTypeError("Unable to convert value to {{cpp_class}}.");
> nit: Can we use {{interface_name}} instead?
> This is a user-visible message, so an internal C++ class' name is not
preferred.

The suggestion for interface.cpp.tmpl makes a lot of sense, but I wonder if
changing callback_function.cpp.tmpl does: {{idl_type}} corresponds to the
callback's return type, so we're essentially replacing a message such as "Unable
to convert value to VoidCallbackFunctionInterfaceArg" with "Unable to convert
value to void", which seems less helpful and more confusing.

[haraken, 2017-04-07 13:55:33]

LGTM

[Raphael Kubo da Costa (rakuco), 2017-04-07 13:56:49]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 13:56:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 13:57:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 13:57:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Yuki, 2017-04-07 14:14:14]

On 2017/04/07 13:52:30, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/04/07 13:37:21, Yuki wrote:
> > LGTM.
> > 
> >
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
> > (right):
> > 
> >
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:86:
> > exceptionState.throwTypeError("Unable to convert value to {{cpp_class}}.");
> > nit: Can we use {{idl_type}} instead?
> > I'm not 100% sure if {{idl_type}} is appropriate, but my point is that we
> should
> > use an IDL name rather than an internal C++ class' name because this is a
> > user-visible message.
> > 
> >
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl
(right):
> > 
> >
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:897:
> > exceptionState.throwTypeError("Unable to convert value to {{cpp_class}}.");
> > nit: Can we use {{interface_name}} instead?
> > This is a user-visible message, so an internal C++ class' name is not
> preferred.
> 
> The suggestion for interface.cpp.tmpl makes a lot of sense, but I wonder if
> changing callback_function.cpp.tmpl does: {{idl_type}} corresponds to the
> callback's return type, so we're essentially replacing a message such as
"Unable
> to convert value to VoidCallbackFunctionInterfaceArg" with "Unable to convert
> value to void", which seems less helpful and more confusing.

Oh, then, could you choose the best one?

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip...
I didn't check it for myself, but |callback_function.name| looks like the best
one?

If so, you can add a new entry for example:
  'function_name': callback_function.name,

I knew that {{function_name}}'s value is the same as {{cpp_class}}'s value, but
conceptually they're different.  Especially if we supported [ImplementedAs=]
extended attribute for callback functions, they will be actually different.  So
I'd like to have {{function_name}} or {{idl_function_name}} or
{{calllback_function_name}} or whatever separated from {{cpp_class}}.

[Raphael Kubo da Costa (rakuco), 2017-04-07 14:48:45]

On 2017/04/07 14:14:14, Yuki wrote:
> On 2017/04/07 13:52:30, Raphael Kubo da Costa (rakuco) wrote:
> > On 2017/04/07 13:37:21, Yuki wrote:
> > > LGTM.
> > > 
> > >
> >
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> > > File
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> > >
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:86:
> > > exceptionState.throwTypeError("Unable to convert value to
{{cpp_class}}.");
> > > nit: Can we use {{idl_type}} instead?
> > > I'm not 100% sure if {{idl_type}} is appropriate, but my point is that we
> > should
> > > use an IDL name rather than an internal C++ class' name because this is a
> > > user-visible message.
> > > 
> > >
> >
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2802223002/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:897:
> > > exceptionState.throwTypeError("Unable to convert value to
{{cpp_class}}.");
> > > nit: Can we use {{interface_name}} instead?
> > > This is a user-visible message, so an internal C++ class' name is not
> > preferred.
> > 
> > The suggestion for interface.cpp.tmpl makes a lot of sense, but I wonder if
> > changing callback_function.cpp.tmpl does: {{idl_type}} corresponds to the
> > callback's return type, so we're essentially replacing a message such as
> "Unable
> > to convert value to VoidCallbackFunctionInterfaceArg" with "Unable to
convert
> > value to void", which seems less helpful and more confusing.
> 
> Oh, then, could you choose the best one?
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip...
> I didn't check it for myself, but |callback_function.name| looks like the best
> one?
> 
> If so, you can add a new entry for example:
>   'function_name': callback_function.name,
> 
> I knew that {{function_name}}'s value is the same as {{cpp_class}}'s value,
but
> conceptually they're different.  Especially if we supported [ImplementedAs=]
> extended attribute for callback functions, they will be actually different. 
So
> I'd like to have {{function_name}} or {{idl_function_name}} or
> {{calllback_function_name}} or whatever separated from {{cpp_class}}.

Done in patch v3: I've added |callback_function_name| and an explanation about
why two variables have the same value at the moment.

[Raphael Kubo da Costa (rakuco), 2017-04-07 14:48:56]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 14:49:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-04-07 15:00:19]

LGTM.

[commit-bot: I haz the power, 2017-04-07 16:24:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 16:24:54]

Dry run: This issue passed the CQ dry run.

[Raphael Kubo da Costa (rakuco), 2017-04-07 16:26:47]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[Raphael Kubo da Costa (rakuco), 2017-04-07 16:26:47]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2802223002/#ps40001
(title: "Add |callback_function_name| to the callback function template")

[commit-bot: I haz the power, 2017-04-07 16:27:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 16:36:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491582407333220,
"parent_rev": "3a2a470eed665050d284f1e5dd0e7a1c4949d190", "commit_rev":
"125a3532433159edc1041c7f031d733df0c7682d"}

[commit-bot: I haz the power, 2017-04-07 16:37:52]

Description was changed from

==========
bindings: Throw a type error if converting an interface or callback function
fails.

The previous implementation of nativeValue() for IDL interfaces and callback
functions simply called implTypeWithCheck() (or create() for callback
functions) and returned its value. This meant it was not possible for
callers to easily distinguish a nullptr returned here (meaning the
conversion failed) from nullptr being returned by a nativeValue() call for
another type (which can just be a nullptr).

BUG=690428
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org
==========

to

==========
bindings: Throw a type error if converting an interface or callback function
fails.

The previous implementation of nativeValue() for IDL interfaces and callback
functions simply called implTypeWithCheck() (or create() for callback
functions) and returned its value. This meant it was not possible for
callers to easily distinguish a nullptr returned here (meaning the
conversion failed) from nullptr being returned by a nativeValue() call for
another type (which can just be a nullptr).

BUG=690428
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2802223002
Cr-Commit-Position: refs/heads/master@{#462890}
Committed:
https://chromium.googlesource.com/chromium/src/+/125a3532433159edc1041c7f031d...
==========

[commit-bot: I haz the power, 2017-04-07 16:37:54]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/125a3532433159edc1041c7f031d...