IDL: Drop value conversion (V8 -> C++) macros from generated code

IDL: Drop value conversion (V8 -> C++) macros from generated code

Instead of using an ever growing set of per-type-and-situation macros
to perform the conversion from V8 value to C++ value (e.g. for method
arguments and in attribute setters), just generate regular C++ code
that does the same job. In many cases, this leads to roughly the same
amount of code, and is arguable easier to read unless one is very
familiar with all the different macros and their definitions.

Technically, the v8_types.v8_value_to_local_cpp_value() function now
returns a small template context dictionary instead of a "single-
expression" string, and a new template macro is used to generate the
multi-line conversion code using this context.

All now unused macros in V8BindingMacros.h are removed. The remaining
macros are used from custom bindings code, which will be rewritten to
not use them too and thus better match the corresponding generated
code. (There was already a fair amount of mismatch between the two.)

BUG=

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

[Jens Widell, 2015-02-23 16:51:10]

jl@opera.com changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, yhirano@chromium.org

[Jens Widell, 2015-02-23 16:51:11]

PTAL

Sorry about the big CL.

Context: https://codereview.chromium.org/938733003/#msg3 (total complexity might
be unchanged, but the *macros* are certainly less complex with this CL.)

Note that this CL is somewhat poorly tested; I'm doing an "end of the day"
upload of this so that you can have a look, before I spend too much more time on
this change, in case you all hate it. :-)

[Jens Widell, 2015-02-23 16:52:59]

On 2015/02/23 16:51:11, Jens Widell wrote:
> Context: https://codereview.chromium.org/938733003/#msg3 (total complexity
might
> be unchanged, but the *macros* are certainly less complex with this CL.)

Oh, BTW, I didn't bother to clean up the old macros (yet). The two new macros
added in this CL are the only ones used in generated code. Some of the old
macros are used in non-generated C++ code (e.g. custom bindings code) but most
or all could be removed.

[haraken, 2015-02-23 17:20:59]

On 2015/02/23 16:52:59, Jens Widell wrote:
> On 2015/02/23 16:51:11, Jens Widell wrote:
> > Context: https://codereview.chromium.org/938733003/#msg3 (total complexity
> might
> > be unchanged, but the *macros* are certainly less complex with this CL.)
> 
> Oh, BTW, I didn't bother to clean up the old macros (yet). The two new macros
> added in this CL are the only ones used in generated code. Some of the old
> macros are used in non-generated C++ code (e.g. custom bindings code) but most
> or all could be removed.

Thanks for doing this! At a first glance, this looks good to me.

[bashi, 2015-02-23 23:44:06]

I think this is a nice cleanup. Thank you for working on this!

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.h:379: bool convertAndThrow(T, ExceptionState&
exceptionState)
convertOrThrow?

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:366: return '%s(%s)' % (macro, ',
'.join(arguments))
Just use 'TONATIVE_CONVERT' and remove |macro|?

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:622: if not use_exception_state:
When could |use_exception_state| be False for dictionary and union types?

[yhirano, 2015-02-24 03:51:24]

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ExceptionState.h (right):

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ExceptionState.h:99: void throwException();
Can you tell me why you want this function in addition to throwIfNeeded?
I don't like this function name, because it is very confusing (what
throwTypeError does is very different from what throwException does). I think
this function should not be public.

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8BindingMacros.h:40: #define TONATIVE_DECLARE(type,
var, expression, bailout) \
[optional] This is not a strong opinion, but I don't see a strong benefit of
this macro: In most (if not all) of occurrences, |expression| contains the
variable name (e.g. TONATIVE_DECLARE(V8StringResource<>, cppValue, (cppValue =
v8Value).prepare(exceptionState), return false)).

It doesn't save symbols from manual declaration with TONATIVE_CONVERT (e.g.
V8Stringresource<> cppValue; TONATIVE_CONVERT((cppValue =
v8Value).prepare(exceptionState), return false)). I feel the latter easier to
read.

[Jens Widell, 2015-02-24 07:31:43]

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ExceptionState.h (right):

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ExceptionState.h:99: void throwException();
On 2015/02/24 03:51:24, yhirano wrote:
> Can you tell me why you want this function in addition to throwIfNeeded?
> I don't like this function name, because it is very confusing (what
> throwTypeError does is very different from what throwException does). I think
> this function should not be public.

I use it in a situation where I know an exception was thrown, in the "stored in
this object" sense, and just need it thrown in the "throw in V8" sense. Using
throwIfNeeded() would have worked, but would have been somewhat "dishonest"
since there's no question that throwing is needed.

I agree that the name of this function is unfortunate. But the same problem
exists in "throwIfNeeded", I think, since the problem is that "throw" is
overloaded with two different meanings, and "throwIfNeeded" and "throwException"
use it the same way.

The best naming scheme I can think of would be to change "throwTypeError" (and
friends) to "scheduleTypeError", leaving these the only "throw"ing functions.

Personally, I'm not convinced we must change the naming, or that this function
must be private. It's name may be somewhat misleading, but it's fairly obvious
from its signature (no arguments) that it can't be used to "schedule" a new
exception. (It also asserts if you try to use it that way.)

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8BindingMacros.h:40: #define TONATIVE_DECLARE(type,
var, expression, bailout) \
On 2015/02/24 03:51:24, yhirano wrote:
> [optional] This is not a strong opinion, but I don't see a strong benefit of
> this macro: In most (if not all) of occurrences, |expression| contains the
> variable name (e.g. TONATIVE_DECLARE(V8StringResource<>, cppValue, (cppValue =
> v8Value).prepare(exceptionState), return false)).
> 
> It doesn't save symbols from manual declaration with TONATIVE_CONVERT (e.g.
> V8Stringresource<> cppValue; TONATIVE_CONVERT((cppValue =
> v8Value).prepare(exceptionState), return false)). I feel the latter easier to
> read.

Essentially, I don't see a strong benefit to either of these macros. They are
effectively only here so that the Python scripts can provide single-line
"expression-like" code snippets for the Jinja templates to use.

If we were okay with the Python scripts generating (simple) multi-line code
snippets for the Jinja templates to use, we can easily get rid fo these macros
entirely.

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:622: if not use_exception_state:
On 2015/02/23 23:44:06, bashi1 wrote:
> When could |use_exception_state| be False for dictionary and union types?

(Sorry about the poor documentation.)

Here, 'idl_type' talks about the thing we're converting, and
'use_exception_state' talks about the context in which the conversion code
executes.

Specifically, use_exception_state=True means "report exception (only) in the
ExceptionState" while use_exception_state=False means "throw exceptions to V8".

When converting the members of dictionaries and unions, use_exception_state is
true, and when converting arguments to methods it is false (unless the method
returns a promise.)

[haraken, 2015-02-24 07:56:16]

I think this is a right way to go overall, so we might want to split the CL into
pieces and start landing them.

[Jens Widell, 2015-02-24 07:58:54]

PS#3 drops the macros entirely and instead emits multi-line code from the Python
scripts (i.e. from v8_types.py:v8_value_to_local_cpp_value()).

This is perhaps a bit controversial, but the generated code in return is very
nice and straight-forward to read.

[Jens Widell, 2015-02-24 11:56:17]

PS#4 rewrites the patch again so that v8_types.py:v8_value_to_local_cpp_value()
returns a small Jinja context object which a small new Jinja macro converts into
the required multi-line conversion code.

The generated code is the same as for PS#3 (except that a couple of errors were
fixed along the way).

On 2015/02/24 07:56:16, haraken wrote:
> I think this is a right way to go overall, so we might want to split the CL
into
> pieces and start landing them.

I've been trying to come up with meaningful ways to split it, but it's a bit
hard. The only change that is easy to split out (and land first) is the change
that dictionary/union toImpl() functions return bool, but that's a rather
uninteresting change to land individually.

The rest is unfortunately pretty inter-dependent.

[haraken, 2015-02-24 12:44:57]

I cannot find TONATIVE_* and TOSTRING_* in patch set 5. Where did they go?

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/template...
File Source/bindings/templates/conversions.cpp (right):

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/template...
Source/bindings/templates/conversions.cpp:8: {% set item =
thing.v8_value_to_local_cpp_value or thing %}

I'm not sure if we want to add conversions.cpp, but I cannot find a better place
for this macro either...

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/template...
Source/bindings/templates/methods.cpp:7: {% if method.returns_promise and
method.has_exception_state %}

Is it possible that method returns a promise but does not have an
ExceptionState?

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:814: if
(!cppValue.prepare(exceptionState))

Would it be possible to write this like:

  V8StringResource<> cppValue = toV8StringResource(v8value, exceptionState);
  if (exceptionState.hadException())
    return false;

for consistency with other toXXX conversions?

[Jens Widell, 2015-02-24 13:36:08]

On 2015/02/24 12:44:57, haraken wrote:
> I cannot find TONATIVE_* and TOSTRING_* in patch set 5. Where did they go?

PS#5 doesn't add any new macros (i.e. this CL no longer modifies
V8BindingMacros.h) and eliminates all uses of any macros in the generated code.

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/template...
File Source/bindings/templates/conversions.cpp (right):

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/template...
Source/bindings/templates/conversions.cpp:8: {% set item =
thing.v8_value_to_local_cpp_value or thing %}
On 2015/02/24 12:44:57, haraken wrote:
> 
> I'm not sure if we want to add conversions.cpp, but I cannot find a better
place
> for this macro either...

What are the arguments against adding it?

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/template...
Source/bindings/templates/methods.cpp:7: {% if method.returns_promise and
method.has_exception_state %}
On 2015/02/24 12:44:57, haraken wrote:
> 
> Is it possible that method returns a promise but does not have an
> ExceptionState?

This is the case if the method can't raise exceptions (i.e. has no argument
conversions that can, and doesn't have [RaisesException]).

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/tests/re...
File Source/bindings/tests/results/core/UnionTypesCore.cpp (right):

https://codereview.chromium.org/946973005/diff/80001/Source/bindings/tests/re...
Source/bindings/tests/results/core/UnionTypesCore.cpp:814: if
(!cppValue.prepare(exceptionState))
On 2015/02/24 12:44:57, haraken wrote:
> 
> Would it be possible to write this like:
> 
>   V8StringResource<> cppValue = toV8StringResource(v8value, exceptionState);
>   if (exceptionState.hadException())
>     return false;
> 
> for consistency with other toXXX conversions?

It would. But do you mean only this form of prepare() that already uses an
ExceptionState, or both?

There is a slight optimization (although I haven't verified to what degree it
is) in the other form of prepare() in that it doesn't have a v8::TryCatch, and
thus in practice just returns whether there's a scheduled exception in V8,
without extracting that exception from V8.

[haraken, 2015-02-24 14:05:41]

> > I cannot find TONATIVE_* and TOSTRING_* in patch set 5. Where did they go?
> 
> PS#5 doesn't add any new macros (i.e. this CL no longer modifies
> V8BindingMacros.h) and eliminates all uses of any macros in the generated
code.

Makes sense. If you're planning to completely remove the macros from the code
base (I think that's a good idea), this looks like a good starting point.

> > Would it be possible to write this like:
> > 
> >   V8StringResource<> cppValue = toV8StringResource(v8value, exceptionState);
> >   if (exceptionState.hadException())
> >     return false;
> > 
> > for consistency with other toXXX conversions?
> 
> It would. But do you mean only this form of prepare() that already uses an
> ExceptionState, or both?

Ah, got it. It seems not easy to force the consistency for the two prepare()s.

My concern is that people may miss calling .prepare(), but we can just add some
ASSERT to prepare to prevent that.

[Jens Widell, 2015-02-24 14:13:00]

On 2015/02/24 14:05:41, haraken wrote:
> > > I cannot find TONATIVE_* and TOSTRING_* in patch set 5. Where did they go?
> > 
> > PS#5 doesn't add any new macros (i.e. this CL no longer modifies
> > V8BindingMacros.h) and eliminates all uses of any macros in the generated
> code.
> 
> Makes sense. If you're planning to completely remove the macros from the code
> base (I think that's a good idea), this looks like a good starting point.
> 
> > > Would it be possible to write this like:
> > > 
> > >   V8StringResource<> cppValue = toV8StringResource(v8value,
exceptionState);
> > >   if (exceptionState.hadException())
> > >     return false;
> > > 
> > > for consistency with other toXXX conversions?
> > 
> > It would. But do you mean only this form of prepare() that already uses an
> > ExceptionState, or both?
> 
> Ah, got it. It seems not easy to force the consistency for the two prepare()s.
> 
> My concern is that people may miss calling .prepare(), but we can just add
some
> ASSERT to prepare to prevent that.

Missing .prepare() would really only be a problem in custom bindings code and
such. There's not a lot of that, and it still uses (a small subset of) the
macros.

And in such code we could most likely get away with the kind of code you
suggest, or even just introduce and use a "String toString(v8::Local<v8::Value>,
ExceptionState&)" that completely hides away the V8StringResource.
V8StringResource solves problems that non-generated code typically doesn't have
to begin with.

[haraken, 2015-02-24 14:15:49]

On 2015/02/24 14:13:00, Jens Widell wrote:
> On 2015/02/24 14:05:41, haraken wrote:
> > > > I cannot find TONATIVE_* and TOSTRING_* in patch set 5. Where did they
go?
> > > 
> > > PS#5 doesn't add any new macros (i.e. this CL no longer modifies
> > > V8BindingMacros.h) and eliminates all uses of any macros in the generated
> > code.
> > 
> > Makes sense. If you're planning to completely remove the macros from the
code
> > base (I think that's a good idea), this looks like a good starting point.
> > 
> > > > Would it be possible to write this like:
> > > > 
> > > >   V8StringResource<> cppValue = toV8StringResource(v8value,
> exceptionState);
> > > >   if (exceptionState.hadException())
> > > >     return false;
> > > > 
> > > > for consistency with other toXXX conversions?
> > > 
> > > It would. But do you mean only this form of prepare() that already uses an
> > > ExceptionState, or both?
> > 
> > Ah, got it. It seems not easy to force the consistency for the two
prepare()s.
> > 
> > My concern is that people may miss calling .prepare(), but we can just add
> some
> > ASSERT to prepare to prevent that.
> 
> Missing .prepare() would really only be a problem in custom bindings code and
> such. There's not a lot of that, and it still uses (a small subset of) the
> macros.

Just to confirm: Are you planning to replace those macros in custom bindings
with .prepare() (or some better helper method)?

[Jens Widell, 2015-02-24 14:21:16]

On 2015/02/24 14:15:49, haraken wrote:
> On 2015/02/24 14:13:00, Jens Widell wrote:
> > On 2015/02/24 14:05:41, haraken wrote:
> > > > > I cannot find TONATIVE_* and TOSTRING_* in patch set 5. Where did they
> go?
> > > > 
> > > > PS#5 doesn't add any new macros (i.e. this CL no longer modifies
> > > > V8BindingMacros.h) and eliminates all uses of any macros in the
generated
> > > code.
> > > 
> > > Makes sense. If you're planning to completely remove the macros from the
> code
> > > base (I think that's a good idea), this looks like a good starting point.
> > > 
> > > > > Would it be possible to write this like:
> > > > > 
> > > > >   V8StringResource<> cppValue = toV8StringResource(v8value,
> > exceptionState);
> > > > >   if (exceptionState.hadException())
> > > > >     return false;
> > > > > 
> > > > > for consistency with other toXXX conversions?
> > > > 
> > > > It would. But do you mean only this form of prepare() that already uses
an
> > > > ExceptionState, or both?
> > > 
> > > Ah, got it. It seems not easy to force the consistency for the two
> prepare()s.
> > > 
> > > My concern is that people may miss calling .prepare(), but we can just add
> > some
> > > ASSERT to prepare to prevent that.
> > 
> > Missing .prepare() would really only be a problem in custom bindings code
and
> > such. There's not a lot of that, and it still uses (a small subset of) the
> > macros.
> 
> Just to confirm: Are you planning to replace those macros in custom bindings
> with .prepare() (or some better helper method)?

I'm prepared to do it, but open to discuss what is the best approach. Macros
might serve a greater purpose in simplifying code writing in the manually
written code.

In the more varied generated code, as we saw, trying to have a macro for every
case ended up quite messy; and C++ macros are a way to generate code anyway,
which we don't need in the code generator. The manually written code only use
five different macros, and only two of those expand to anything remotely
complicated.

It's worth noting also that the set of macros used in the generated code and the
set of macros used in hand-written code largely didn't overlap. It turns out the
two contexts had different needs and different problems to solve, I guess.

[haraken, 2015-02-24 15:20:50]

On 2015/02/24 14:21:16, Jens Widell wrote:
> On 2015/02/24 14:15:49, haraken wrote:
> > On 2015/02/24 14:13:00, Jens Widell wrote:
> > > On 2015/02/24 14:05:41, haraken wrote:
> > > > > > I cannot find TONATIVE_* and TOSTRING_* in patch set 5. Where did
they
> > go?
> > > > > 
> > > > > PS#5 doesn't add any new macros (i.e. this CL no longer modifies
> > > > > V8BindingMacros.h) and eliminates all uses of any macros in the
> generated
> > > > code.
> > > > 
> > > > Makes sense. If you're planning to completely remove the macros from the
> > code
> > > > base (I think that's a good idea), this looks like a good starting
point.
> > > > 
> > > > > > Would it be possible to write this like:
> > > > > > 
> > > > > >   V8StringResource<> cppValue = toV8StringResource(v8value,
> > > exceptionState);
> > > > > >   if (exceptionState.hadException())
> > > > > >     return false;
> > > > > > 
> > > > > > for consistency with other toXXX conversions?
> > > > > 
> > > > > It would. But do you mean only this form of prepare() that already
uses
> an
> > > > > ExceptionState, or both?
> > > > 
> > > > Ah, got it. It seems not easy to force the consistency for the two
> > prepare()s.
> > > > 
> > > > My concern is that people may miss calling .prepare(), but we can just
add
> > > some
> > > > ASSERT to prepare to prevent that.
> > > 
> > > Missing .prepare() would really only be a problem in custom bindings code
> and
> > > such. There's not a lot of that, and it still uses (a small subset of) the
> > > macros.
> > 
> > Just to confirm: Are you planning to replace those macros in custom bindings
> > with .prepare() (or some better helper method)?
> 
> I'm prepared to do it, but open to discuss what is the best approach. Macros
> might serve a greater purpose in simplifying code writing in the manually
> written code.
> 
> In the more varied generated code, as we saw, trying to have a macro for every
> case ended up quite messy; and C++ macros are a way to generate code anyway,
> which we don't need in the code generator. The manually written code only use
> five different macros, and only two of those expand to anything remotely
> complicated.
> 
> It's worth noting also that the set of macros used in the generated code and
the
> set of macros used in hand-written code largely didn't overlap. It turns out
the
> two contexts had different needs and different problems to solve, I guess.

Yeah, I got your point. That said, people write custom bindings based on
auto-generated code, so it is important that both follow the same rule. If we
don't use macros in auto-generated code but use macros in custom bindings, we
will end up getting wrong custom bindings.

Thus if we remove macros from auto-generated code, I'd prefer removing macros
from custom bindings as well. If it will complicate the custom bindings, it will
be worth considering keeping a couple of macros (e.g., TONATIVE_DECLARE() and
TONATIVE_CONVERT()) for both custom bindings and auto-generated code.

[Jens Widell, 2015-02-24 15:30:35]

On 2015/02/24 15:20:50, haraken wrote:
> On 2015/02/24 14:21:16, Jens Widell wrote:
> > On 2015/02/24 14:15:49, haraken wrote:
> > > On 2015/02/24 14:13:00, Jens Widell wrote:
> > > > On 2015/02/24 14:05:41, haraken wrote:
> > > > > > > I cannot find TONATIVE_* and TOSTRING_* in patch set 5. Where did
> they
> > > go?
> > > > > > 
> > > > > > PS#5 doesn't add any new macros (i.e. this CL no longer modifies
> > > > > > V8BindingMacros.h) and eliminates all uses of any macros in the
> > generated
> > > > > code.
> > > > > 
> > > > > Makes sense. If you're planning to completely remove the macros from
the
> > > code
> > > > > base (I think that's a good idea), this looks like a good starting
> point.
> > > > > 
> > > > > > > Would it be possible to write this like:
> > > > > > > 
> > > > > > >   V8StringResource<> cppValue = toV8StringResource(v8value,
> > > > exceptionState);
> > > > > > >   if (exceptionState.hadException())
> > > > > > >     return false;
> > > > > > > 
> > > > > > > for consistency with other toXXX conversions?
> > > > > > 
> > > > > > It would. But do you mean only this form of prepare() that already
> uses
> > an
> > > > > > ExceptionState, or both?
> > > > > 
> > > > > Ah, got it. It seems not easy to force the consistency for the two
> > > prepare()s.
> > > > > 
> > > > > My concern is that people may miss calling .prepare(), but we can just
> add
> > > > some
> > > > > ASSERT to prepare to prevent that.
> > > > 
> > > > Missing .prepare() would really only be a problem in custom bindings
code
> > and
> > > > such. There's not a lot of that, and it still uses (a small subset of)
the
> > > > macros.
> > > 
> > > Just to confirm: Are you planning to replace those macros in custom
bindings
> > > with .prepare() (or some better helper method)?
> > 
> > I'm prepared to do it, but open to discuss what is the best approach. Macros
> > might serve a greater purpose in simplifying code writing in the manually
> > written code.
> > 
> > In the more varied generated code, as we saw, trying to have a macro for
every
> > case ended up quite messy; and C++ macros are a way to generate code anyway,
> > which we don't need in the code generator. The manually written code only
use
> > five different macros, and only two of those expand to anything remotely
> > complicated.
> > 
> > It's worth noting also that the set of macros used in the generated code and
> the
> > set of macros used in hand-written code largely didn't overlap. It turns out
> the
> > two contexts had different needs and different problems to solve, I guess.
> 
> Yeah, I got your point. That said, people write custom bindings based on
> auto-generated code, so it is important that both follow the same rule. If we
> don't use macros in auto-generated code but use macros in custom bindings, we
> will end up getting wrong custom bindings.

True, of course.

But we'll also inevitably end up getting wrong custom bindings as long as we
have them at all. :-)

> Thus if we remove macros from auto-generated code, I'd prefer removing macros
> from custom bindings as well. If it will complicate the custom bindings, it
will
> be worth considering keeping a couple of macros (e.g., TONATIVE_DECLARE() and
> TONATIVE_CONVERT()) for both custom bindings and auto-generated code.

I think the TONATIVE_DECLARE()/TONATIVE_CONVERT() macros were little more than
really odd ways of writing code. I doubt they would have helped avoid any
particular problems in custom bindings, and rather would have added problems in
being themselves odd and tricky to use.

If we want to have as similar code as possible between generated and custom
code, which I agree we do, I think we're mostly left with one option: use no
macros in custom code either.

[Jens Widell, 2015-02-24 15:34:57]

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ExceptionState.h (right):

https://codereview.chromium.org/946973005/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ExceptionState.h:99: void throwException();
On 2015/02/24 07:31:43, Jens Widell wrote:
> On 2015/02/24 03:51:24, yhirano wrote:
> > Can you tell me why you want this function in addition to throwIfNeeded?
> > I don't like this function name, because it is very confusing (what
> > throwTypeError does is very different from what throwException does). I
think
> > this function should not be public.
> 
> I use it in a situation where I know an exception was thrown, in the "stored
in
> this object" sense, and just need it thrown in the "throw in V8" sense. Using
> throwIfNeeded() would have worked, but would have been somewhat "dishonest"
> since there's no question that throwing is needed.
> 
> I agree that the name of this function is unfortunate. But the same problem
> exists in "throwIfNeeded", I think, since the problem is that "throw" is
> overloaded with two different meanings, and "throwIfNeeded" and
"throwException"
> use it the same way.
> 
> The best naming scheme I can think of would be to change "throwTypeError" (and
> friends) to "scheduleTypeError", leaving these the only "throw"ing functions.
> 
> Personally, I'm not convinced we must change the naming, or that this function
> must be private. It's name may be somewhat misleading, but it's fairly obvious
> from its signature (no arguments) that it can't be used to "schedule" a new
> exception. (It also asserts if you try to use it that way.)

I've reverted this change, as it wasn't really required.

[haraken, 2015-02-24 15:46:49]

On 2015/02/24 15:30:35, Jens Widell wrote:
> On 2015/02/24 15:20:50, haraken wrote:
> > On 2015/02/24 14:21:16, Jens Widell wrote:
> > > On 2015/02/24 14:15:49, haraken wrote:
> > > > On 2015/02/24 14:13:00, Jens Widell wrote:
> > > > > On 2015/02/24 14:05:41, haraken wrote:
> > > > > > > > I cannot find TONATIVE_* and TOSTRING_* in patch set 5. Where
did
> > they
> > > > go?
> > > > > > > 
> > > > > > > PS#5 doesn't add any new macros (i.e. this CL no longer modifies
> > > > > > > V8BindingMacros.h) and eliminates all uses of any macros in the
> > > generated
> > > > > > code.
> > > > > > 
> > > > > > Makes sense. If you're planning to completely remove the macros from
> the
> > > > code
> > > > > > base (I think that's a good idea), this looks like a good starting
> > point.
> > > > > > 
> > > > > > > > Would it be possible to write this like:
> > > > > > > > 
> > > > > > > >   V8StringResource<> cppValue = toV8StringResource(v8value,
> > > > > exceptionState);
> > > > > > > >   if (exceptionState.hadException())
> > > > > > > >     return false;
> > > > > > > > 
> > > > > > > > for consistency with other toXXX conversions?
> > > > > > > 
> > > > > > > It would. But do you mean only this form of prepare() that already
> > uses
> > > an
> > > > > > > ExceptionState, or both?
> > > > > > 
> > > > > > Ah, got it. It seems not easy to force the consistency for the two
> > > > prepare()s.
> > > > > > 
> > > > > > My concern is that people may miss calling .prepare(), but we can
just
> > add
> > > > > some
> > > > > > ASSERT to prepare to prevent that.
> > > > > 
> > > > > Missing .prepare() would really only be a problem in custom bindings
> code
> > > and
> > > > > such. There's not a lot of that, and it still uses (a small subset of)
> the
> > > > > macros.
> > > > 
> > > > Just to confirm: Are you planning to replace those macros in custom
> bindings
> > > > with .prepare() (or some better helper method)?
> > > 
> > > I'm prepared to do it, but open to discuss what is the best approach.
Macros
> > > might serve a greater purpose in simplifying code writing in the manually
> > > written code.
> > > 
> > > In the more varied generated code, as we saw, trying to have a macro for
> every
> > > case ended up quite messy; and C++ macros are a way to generate code
anyway,
> > > which we don't need in the code generator. The manually written code only
> use
> > > five different macros, and only two of those expand to anything remotely
> > > complicated.
> > > 
> > > It's worth noting also that the set of macros used in the generated code
and
> > the
> > > set of macros used in hand-written code largely didn't overlap. It turns
out
> > the
> > > two contexts had different needs and different problems to solve, I guess.
> > 
> > Yeah, I got your point. That said, people write custom bindings based on
> > auto-generated code, so it is important that both follow the same rule. If
we
> > don't use macros in auto-generated code but use macros in custom bindings,
we
> > will end up getting wrong custom bindings.
> 
> True, of course.
> 
> But we'll also inevitably end up getting wrong custom bindings as long as we
> have them at all. :-)
> 
> > Thus if we remove macros from auto-generated code, I'd prefer removing
macros
> > from custom bindings as well. If it will complicate the custom bindings, it
> will
> > be worth considering keeping a couple of macros (e.g., TONATIVE_DECLARE()
and
> > TONATIVE_CONVERT()) for both custom bindings and auto-generated code.
> 
> I think the TONATIVE_DECLARE()/TONATIVE_CONVERT() macros were little more than
> really odd ways of writing code. I doubt they would have helped avoid any
> particular problems in custom bindings, and rather would have added problems
in
> being themselves odd and tricky to use.
> 
> If we want to have as similar code as possible between generated and custom
> code, which I agree we do, I think we're mostly left with one option: use no
> macros in custom code either.

This sounds like a good approach.

At the very least, the current macros are out of our control, so I think it's
worth removing them as a step to clean up the binding code. If we find some
macros or helper methods are needed to make the binding code nicer, we can
(re-)introduce them as necessary.

(Taking a final look at the CL.)

[haraken, 2015-02-24 15:59:39]

LGTM

https://codereview.chromium.org/946973005/diff/180001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/946973005/diff/180001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:359: check_expression =
'exceptionState.throwIfNeeded()'

I'm a bit confused about when throwIfNeeded() should be used.

- throwIfNeeded() is used sometimes as check_expression and sometimes as
throw_expression. Why?

- In most value conversions, throw_expression shouldn't be needed because we
don't want to call throwIfNeeded(). When do we need throw_expression?

https://codereview.chromium.org/946973005/diff/180001/Source/bindings/scripts...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/946973005/diff/180001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:633: check_expression =
'exceptionState.throwIfNeeded()'

It seems a bit confusing that we use exceptionState while we're not
use_exception_state.

[Jens Widell, 2015-02-24 16:15:59]

https://codereview.chromium.org/946973005/diff/180001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/946973005/diff/180001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:359: check_expression =
'exceptionState.throwIfNeeded()'
On 2015/02/24 15:59:38, haraken wrote:
> 
> I'm a bit confused about when throwIfNeeded() should be used.
> 
> - throwIfNeeded() is used sometimes as check_expression and sometimes as
> throw_expression. Why?
>
> - In most value conversions, throw_expression shouldn't be needed because we
> don't want to call throwIfNeeded(). When do we need throw_expression?

I use throwIfNeeded() as throw_expression when check_expression is some other
expression that works the same way, IOW returns a true value if an exception was
thrown.

That's only the case for union and dictionary types, and that's only possible
because I also changed their toImpl() to return such a truth value. That change
is not really required; we could call those toImpl() functions in a void
context, and then use throwIfNeeded() as check_expression in that case too.

But calling toImpl() in a void context requires the set_expression that I had
for a while and then removed, so we'd exchange throw_expression for
set_expression. That exchange would actually make things a bit simpler, I think,
so...

Now that you brought it up, I'm inclined to change my mind and do it that way
instead. IOW, revert the return-value-from-toImpl() change, and have
set_expression (used in a single case) instead of throw_expression (currently
used in that same single case).

https://codereview.chromium.org/946973005/diff/180001/Source/bindings/scripts...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/946973005/diff/180001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:633: check_expression =
'exceptionState.throwIfNeeded()'
On 2015/02/24 15:59:38, haraken wrote:
> 
> It seems a bit confusing that we use exceptionState while we're not
> use_exception_state.

Yeah, "use_exception_state" is not a variable name that fully explains its
purpose.

When use_exception_state is true, the generated code should store any exception
thrown in the ExceptionState object, as opposed to throwing in V8.

When it's false, the code may still have an ExceptionState object to pass as
arguments to called conversion functions (for them to throw via) but should in
any case throw exceptions in V8, e.g. by calling exceptionState.throwIfNeeded().

Whether there's an exceptionState local in the C++ scope of the generated code
is taken care of implicitly by someone else; it'll be there if this function
generates code that needs it.

[Jens Widell, 2015-02-24 16:48:34]

On 2015/02/24 16:15:59, Jens Widell wrote:
> Now that you brought it up, I'm inclined to change my mind and do it that way
> instead. IOW, revert the return-value-from-toImpl() change, and have
> set_expression (used in a single case) instead of throw_expression (currently
> used in that same single case).

Done in PS#11. PTAL?

[haraken, 2015-02-24 17:28:13]

LGTM

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/scripts...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:645: 

Although your CL is an improvement, the above exception handling looks still
complicated. It would be great if we could simplify the handling more :)

[yhirano, 2015-02-25 04:00:11]

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:309: {% macro
throw_from_exception_state(method_or_overloads) %}
You changed this macro's meaning a bit: Are "propagate_exception" or
"propagate_error_with_exception_state" more suitable for its name now?

[bashi, 2015-02-25 04:44:35]

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:53: v8::TryCatch block;               
        \
BTW, should we use V8RethrowTryCatchScope here (like TONATIVE_VOID) ?

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
File Source/bindings/templates/conversions.cpp (right):

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
Source/bindings/templates/conversions.cpp:8: {% set item =
thing.v8_value_to_local_cpp_value or thing %}
Just a comment:
I guess you do this to avoid writing |thing.v8_value_to_local_cpp_value| in call
sites, but it may make readers difficult to understand what argument type this
macro takes.

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
Source/bindings/templates/conversions.cpp:33: 
nit: remove the second empty line?

[Jens Widell, 2015-02-25 07:37:11]

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:53: v8::TryCatch block;               
        \
On 2015/02/25 04:44:35, bashi1 wrote:
> BTW, should we use V8RethrowTryCatchScope here (like TONATIVE_VOID) ?

There's no real reason for TONATIVE_VOID and TONATIVE_DEFAULT to use different
patterns, for sure. But I don't think there's really a reason for either to use
V8RethrowTryCatchScope either; they're simple enough that just calling
block.ReThrow() directly makes more sense, IMO.

So I think I'll rather change TONATIVE_VOID() to not use it. (Both these macros
are going to be removed soon, I think.)

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:309: {% macro
throw_from_exception_state(method_or_overloads) %}
On 2015/02/25 04:00:11, yhirano wrote:
> You changed this macro's meaning a bit: Are "propagate_exception" or
> "propagate_error_with_exception_state" more suitable for its name now?

Yes, "propagate_error_with_exception_state" is probably a better name.

[Jens Widell, 2015-02-25 07:49:43]

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:53: v8::TryCatch block;               
        \
On 2015/02/25 07:37:10, Jens Widell wrote:
> So I think I'll rather change TONATIVE_VOID() to not use it.

Done.

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
File Source/bindings/templates/conversions.cpp (right):

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
Source/bindings/templates/conversions.cpp:33: 
On 2015/02/25 04:44:35, bashi1 wrote:
> nit: remove the second empty line?

Done.

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/946973005/diff/200001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:309: {% macro
throw_from_exception_state(method_or_overloads) %}
On 2015/02/25 07:37:11, Jens Widell wrote:
> On 2015/02/25 04:00:11, yhirano wrote:
> > You changed this macro's meaning a bit: Are "propagate_exception" or
> > "propagate_error_with_exception_state" more suitable for its name now?
> 
> Yes, "propagate_error_with_exception_state" is probably a better name.

Done.

[yhirano, 2015-02-25 07:50:51]

lgtm

[Jens Widell, 2015-02-25 09:34:02]

The CQ bit was checked by jl@opera.com

[Jens Widell, 2015-02-25 09:34:03]

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/946973005/#ps210001
(title: "address comments")

[commit-bot: I haz the power, 2015-02-25 09:35:03]

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

[commit-bot: I haz the power, 2015-02-25 09:40:02]

Committed patchset #12 (id:210001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190816