API functions returning Promises should not throw exceptions.

API functions returning Promises should not throw exceptions.

http://heycam.github.io/webidl/#es-operations states that an API returning
a Promise should return a rejected Promise when an error occurs rather than
throwing an exception.

BUG=359386, 362992
R=bashi@chromium.org, eroman@chromium.org

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

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

[Nils Barth (inactive), 2014-04-10 07:48:02]

Generally looks fine and elegant;
some style nits, main concern is code duplication in
v8_value_to_local_cpp_value_async
I'll post a refactoring CL that should make reusing
v8_value_to_local_cpp_value
easier.

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

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:264: vector_type = 'WillBeHeapVector' if
idl_type.is_will_be_garbage_collected else 'Vector'
Can use the new
v8_types.cpp_ptr_type
that haraken@ just added, as:
v8_types.cpp_ptr_type('Vector', 'HeapVector', idl_type.gc_type)

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:273: return
idl_type.v8_value_to_local_cpp_value_async(argument.extended_attributes,
nit:
can write:
extended_attributes
instead of:
argument.extended_attributes

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

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:418: def
v8_value_to_local_cpp_value_async(idl_type, extended_attributes, v8_value,
variable_name, index=None):
You're just adding 'info' here, right?
If so, could you just add it to the above v8_value_to_local_cpp_value
with a flag?
I think it would be clearer to refactor the previous to use an 'arguments'
variable instead; I'll do that separately.

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

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/template...
Source/bindings/templates/methods.cpp:27: {{throw_from_exception_state(method) |
indent}}
No indent here (it's a 1-line macro, right), and ';' outside macro
(In any case, if this were a multi-line macro, you'd need indent(12) here.)

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/template...
Source/bindings/templates/methods.cpp:35: {{throw_from_exception_state(method) |
indent}}
no indent, ; at call

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/template...
Source/bindings/templates/methods.cpp:42: {{throw_from_exception_state(method) |
indent}}
no indent, ; at call

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/template...
Source/bindings/templates/methods.cpp:272: 
2 lines between macros

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/template...
Source/bindings/templates/methods.cpp:281: 
2 lines between macros

[Nils Barth (inactive), 2014-04-10 07:59:47]

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

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:418: def
v8_value_to_local_cpp_value_async(idl_type, extended_attributes, v8_value,
variable_name, index=None):
Oh, and you're tacking on _PROMISE.
Anyway, I've posted a refactoring CL:
Refactor v8_types.v8_value_to_local_cpp_value for clarity and to make Promises
easier to implement
https://codereview.chromium.org/231833005/
...so I think you can just add:
async=False
async: If true, the generated code returns a rejected Promise when it sees an
error rather than throwing an exception.

if async:
    args.append('info')
...
if async:
    v8trycatch_void += '_PROMISE'

[eroman, 2014-04-10 09:28:14]

Thanks!

Is it necessary to rebaseline any of the LayoutTests? (the crypto/ tests for
instance currently expect type errors to be synchronous).

Otherwise deferring review to nbarth

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
File LayoutTests/fast/js/Promise-bindings-check-exception.html (right):

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
LayoutTests/fast/js/Promise-bindings-check-exception.html:17: return
promise.then(function() {
nit; should the javascript indentation be 4 spaces?

[Nils Barth (inactive), 2014-04-10 10:02:47]

Test comments

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
File LayoutTests/fast/js/Promise-bindings-check-exception.html (right):

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
LayoutTests/fast/js/Promise-bindings-check-exception.html:2: <html>
nit: <html>, <head>, and <body> can all be omitted, and this is recommended in
Google style:
http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=Op...

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
LayoutTests/fast/js/Promise-bindings-check-exception.html:10: description('Test
whether the generated code handles exceptions correctly.');
generated *bindings* ?
Also, perhaps linking to the spec:
// http://heycam.github.io/webidl/#es-operations
...and using more explicit wording:
description('Operations that return a promise type should handle exceptions ' +
            'by rejecting the promise with the exception as the reason.')

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
LayoutTests/fast/js/Promise-bindings-check-exception.html:86: 
? Trailing newline ?

[Nils Barth (inactive), 2014-04-10 10:03:15]

haraken@, could you take a look at the bindings/v8 changes?
Thanks!

[haraken, 2014-04-10 12:09:17]

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/Excep...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/Excep...
Source/bindings/v8/ExceptionState.h:96: ScriptPromise rejectedPromise()

Shall we just call it "reject", just like other reject methods in
ScriptPromise.h?

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8BindingMacros.h:102: #define
V8TRYCATCH_EXCEPTION_VOID_PROMISE(type, var, value, info, exceptionState) \

The macro names in this file are getting broken... What does V8TRYCATCH_A_B_C
mean?

I'd be happy if you could clean up the names before landing this CL.

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8BindingMacros.h:110: if
(UNLIKELY(exceptionState.hadException())) {                            \

Shouldn't this be exceptionState.throwIfNeeded()?

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Str...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Str...
Source/bindings/v8/V8StringResource.h:204: return !m_v8Object.IsEmpty();

Won't this change existing behavior?

[Nils Barth (inactive), 2014-04-11 01:03:41]

Macro naming; I'll fix.

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8BindingMacros.h:102: #define
V8TRYCATCH_EXCEPTION_VOID_PROMISE(type, var, value, info, exceptionState) \
On 2014/04/10 12:09:17, haraken wrote:
> 
> The macro names in this file are getting broken... What does V8TRYCATCH_A_B_C
> mean?
> 
> I'd be happy if you could clean up the names before landing this CL.

I'll grab this.

My suggestion:
V8TRYCATCH[_TYPE][_args...]
...i.e.:
V8TRYCATCH[_TYPE][_EXCEPTION][_RETURN]

Thus we'd rename:
V8TRYCATCH_EXCEPTION_VOID => V8TRYCATCH_VOID_EXCEPTION

Also, V8TRYCATCH_FOR_V8STRINGRESOURCE* does *not* TRYCATCH,
so these should just be:
V8STRINGRESOURCE_RETURN
V8STRINGRESOURCE_VOID

So for this CL:
V8TRYCATCH_EXCEPTION_VOID_PROMISE
=>
V8TRYCATCH_VOID_INFO_EXCEPTION
etc.

[haraken, 2014-04-11 01:09:40]

> I'll grab this.
> 
> My suggestion:
> V8TRYCATCH[_TYPE][_args...]
> ...i.e.:
> V8TRYCATCH[_TYPE][_EXCEPTION][_RETURN]
> 
> Thus we'd rename:
> V8TRYCATCH_EXCEPTION_VOID => V8TRYCATCH_VOID_EXCEPTION
> 
> Also, V8TRYCATCH_FOR_V8STRINGRESOURCE* does *not* TRYCATCH,
> so these should just be:
> V8STRINGRESOURCE_RETURN
> V8STRINGRESOURCE_VOID

This makes a lot of sense!


> So for this CL:
> V8TRYCATCH_EXCEPTION_VOID_PROMISE
> =>
> V8TRYCATCH_VOID_INFO_EXCEPTION
> etc.

yhirano was saying he's considering further refactoring, so I'm not sure if he
still needs this macro in this CL.

[yhirano, 2014-04-11 01:14:31]

On 2014/04/11 01:09:40, haraken wrote:
> > I'll grab this.
> > 
> > My suggestion:
> > V8TRYCATCH[_TYPE][_args...]
> > ...i.e.:
> > V8TRYCATCH[_TYPE][_EXCEPTION][_RETURN]
> > 
> > Thus we'd rename:
> > V8TRYCATCH_EXCEPTION_VOID => V8TRYCATCH_VOID_EXCEPTION
> > 
> > Also, V8TRYCATCH_FOR_V8STRINGRESOURCE* does *not* TRYCATCH,
> > so these should just be:
> > V8STRINGRESOURCE_RETURN
> > V8STRINGRESOURCE_VOID
> 
Thanks a lot!

> > So for this CL:
> > V8TRYCATCH_EXCEPTION_VOID_PROMISE
> > =>
> > V8TRYCATCH_VOID_INFO_EXCEPTION
> > etc.
> 
> yhirano was saying he's considering further refactoring, so I'm not sure if he
> still needs this macro in this CL.
I would reconsider the change on V8StringResource.h, but the V8TRYCATCHs for
Promises is still needed.
I will change macros I introduced in this CL.

[Nils Barth (inactive), 2014-04-11 02:19:38]

posted name fixing

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8BindingMacros.h:102: #define
V8TRYCATCH_EXCEPTION_VOID_PROMISE(type, var, value, info, exceptionState) \
Posted:
Rename V8TRYCATCH_* macros in v8/V8BindingMacros.h
https://codereview.chromium.org/234403004

[yhirano, 2014-04-11 10:19:04]

Thanks a lot for the comments.
I addressed the most of your comments.

The only unaddressed one is haraken's comment on V8StringResource.h.

>
https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Str...
> Source/bindings/v8/V8StringResource.h:204: return !m_v8Object.IsEmpty();
> 
> Won't this change existing behavior?
>

==

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

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:264: vector_type = 'WillBeHeapVector' if
idl_type.is_will_be_garbage_collected else 'Vector'
On 2014/04/10 07:48:03, Nils Barth wrote:
> Can use the new
> v8_types.cpp_ptr_type
> that haraken@ just added, as:
> v8_types.cpp_ptr_type('Vector', 'HeapVector', idl_type.gc_type)

Done.

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:273: return
idl_type.v8_value_to_local_cpp_value_async(argument.extended_attributes,
On 2014/04/10 07:48:03, Nils Barth wrote:
> nit:
> can write:
> extended_attributes
> instead of:
> argument.extended_attributes

Done.

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

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:418: def
v8_value_to_local_cpp_value_async(idl_type, extended_attributes, v8_value,
variable_name, index=None):
On 2014/04/10 07:59:47, Nils Barth wrote:
> Oh, and you're tacking on _PROMISE.
> Anyway, I've posted a refactoring CL:
> Refactor v8_types.v8_value_to_local_cpp_value for clarity and to make Promises
> easier to implement
> https://codereview.chromium.org/231833005/
> ...so I think you can just add:
> async=False
> async: If true, the generated code returns a rejected Promise when it sees an
> error rather than throwing an exception.
> 
> if async:
>     args.append('info')
> ...
> if async:
>     v8trycatch_void += '_PROMISE'

Thanks, done

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

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/template...
Source/bindings/templates/methods.cpp:27: {{throw_from_exception_state(method) |
indent}}
On 2014/04/10 07:48:03, Nils Barth wrote:
> No indent here (it's a 1-line macro, right), and ';' outside macro
> (In any case, if this were a multi-line macro, you'd need indent(12) here.)

Done.

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/template...
Source/bindings/templates/methods.cpp:35: {{throw_from_exception_state(method) |
indent}}
On 2014/04/10 07:48:03, Nils Barth wrote:
> no indent, ; at call

Done.

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/template...
Source/bindings/templates/methods.cpp:42: {{throw_from_exception_state(method) |
indent}}
On 2014/04/10 07:48:03, Nils Barth wrote:
> no indent, ; at call

Done.

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/template...
Source/bindings/templates/methods.cpp:272: 
On 2014/04/10 07:48:03, Nils Barth wrote:
> 2 lines between macros

Done.

https://codereview.chromium.org/232563003/diff/50001/Source/bindings/template...
Source/bindings/templates/methods.cpp:281: 
On 2014/04/10 07:48:03, Nils Barth wrote:
> 2 lines between macros

Done.

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
File LayoutTests/fast/js/Promise-bindings-check-exception.html (right):

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
LayoutTests/fast/js/Promise-bindings-check-exception.html:2: <html>
On 2014/04/10 10:02:47, Nils Barth wrote:
> nit: <html>, <head>, and <body> can all be omitted, and this is recommended in
> Google style:
>
http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=Op...

Done.

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
LayoutTests/fast/js/Promise-bindings-check-exception.html:10: description('Test
whether the generated code handles exceptions correctly.');
On 2014/04/10 10:02:47, Nils Barth wrote:
> generated *bindings* ?
> Also, perhaps linking to the spec:
> // http://heycam.github.io/webidl/#es-operations
> ...and using more explicit wording:
> description('Operations that return a promise type should handle exceptions '
+
>             'by rejecting the promise with the exception as the reason.')

Done.

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
LayoutTests/fast/js/Promise-bindings-check-exception.html:17: return
promise.then(function() {
On 2014/04/10 09:28:15, eroman wrote:
> nit; should the javascript indentation be 4 spaces?

Done.

https://codereview.chromium.org/232563003/diff/70001/LayoutTests/fast/js/Prom...
LayoutTests/fast/js/Promise-bindings-check-exception.html:86: 
On 2014/04/10 10:02:47, Nils Barth wrote:
> ? Trailing newline ?

Done.

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/Excep...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/Excep...
Source/bindings/v8/ExceptionState.h:96: ScriptPromise rejectedPromise()
On 2014/04/10 12:09:17, haraken wrote:
> 
> Shall we just call it "reject", just like other reject methods in
> ScriptPromise.h?

Done.

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8BindingMacros.h:102: #define
V8TRYCATCH_EXCEPTION_VOID_PROMISE(type, var, value, info, exceptionState) \
On 2014/04/11 02:19:38, Nils Barth wrote:
> Posted:
> Rename V8TRYCATCH_* macros in v8/V8BindingMacros.h
> https://codereview.chromium.org/234403004

Done.

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8BindingMacros.h:110: if
(UNLIKELY(exceptionState.hadException())) {                            \
On 2014/04/10 12:09:17, haraken wrote:
> 
> Shouldn't this be exceptionState.throwIfNeeded()?

What I want to do is returning a Promise rejected with the exception that the
exceptionState has. So I think its' OK.

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Str...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/232563003/diff/70001/Source/bindings/v8/V8Str...
Source/bindings/v8/V8StringResource.h:204: return !m_v8Object.IsEmpty();
On 2014/04/10 12:09:17, haraken wrote:
> 
> Won't this change existing behavior?

Hm. I changed here because next v8::TryCatchs doesn't work correctly. I filed a
bug http://crbug.com/362388 . If v8 guys fix it, I can revert the change. If
not, I will find another way...

[Nils Barth (inactive), 2014-04-11 10:26:38]

Thanks for revisions!
1 Python request (merge 2 almost identical functions),
otherwise LGTM from CG.

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

https://codereview.chromium.org/232563003/diff/130001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:266: def
v8_value_to_local_cpp_value_async(argument, index):
Could you merge this with above, only adding an async=False argument?
The only difference is the macro name in the variadic case, right?
So for the variadic case you can put:
macro = 'TONATIVE_VOID_ASYNC' if async else 'TONATIVE_VOID'
and then have the return expression be something like:
        return '{macro}({vector_type}<{cpp_type}>, {name},
toNativeArguments<{cpp_type}>(info, {index}), info)'.format(
            macro=macro, vector_type=vector_type, cpp_type=idl_type.cpp_type,
            name=name, index=index)

[yhirano, 2014-04-11 10:54:21]

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

https://codereview.chromium.org/232563003/diff/130001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:266: def
v8_value_to_local_cpp_value_async(argument, index):
On 2014/04/11 10:26:39, Nils Barth wrote:
> Could you merge this with above, only adding an async=False argument?
> The only difference is the macro name in the variadic case, right?
> So for the variadic case you can put:
> macro = 'TONATIVE_VOID_ASYNC' if async else 'TONATIVE_VOID'
> and then have the return expression be something like:
>         return '{macro}({vector_type}<{cpp_type}>, {name},
> toNativeArguments<{cpp_type}>(info, {index}), info)'.format(
>             macro=macro, vector_type=vector_type, cpp_type=idl_type.cpp_type,
>             name=name, index=index)

Done. I split the variadic case to another function.

[Nils Barth (inactive), 2014-04-11 11:08:39]

Thanks for factoring out the variadic case!
Suggestion to simplify string formatting, otherwise fine.

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

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:253: macro_args_format =
'{vector_type}<{cpp_type}>, {name}, toNativeArguments<{cpp_type}>(info,
{index})' + (', info' if async else '')
I think this would be clearer as a list, followed by ', '.join, i.e.,

cpp_type = idl_type.cpp_type
macro_ars = ['%s<%s>' % (vector_type, cpp_type),
             name,
             'toNativeArguments<%s>(info, %s)' % (cpp_type, index)]
if async:
    macro_args.append('info')

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:260: return '{0}({1})'.format(macro,
macro_args_format.format(**macro_args))
For simple positional format we prefer %, so this becomes:
return '%s(%s)' % (macro, ', '.join(macro_args))

[haraken, 2014-04-11 13:57:12]

LGTM.

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/v8/Exce...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/v8/Exce...
Source/bindings/v8/ExceptionState.h:100: return
ScriptPromise::reject(V8ThrowException::createError(v8GeneralError, "Unknown
Error", m_isolate), m_isolate);

Is this behavior speced? Which should we use, v8GeneralError or DOMError?

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/v8/V8St...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/v8/V8St...
Source/bindings/v8/V8StringResource.h:204: return !m_v8Object.IsEmpty();

I double-checked and confirmed that the new code looks equivalent to the old
code. So, OK.

https://codereview.chromium.org/232563003/diff/170001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/232563003/diff/170001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:2445: ScriptPromise
Internals::promiseCheck(ExecutionContext* context, long arg1, bool arg2, const
Dictionary& arg3, const String& arg4, ExceptionState& es)

es => exceptionState

[yhirano, 2014-04-14 01:03:37]

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

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:253: macro_args_format =
'{vector_type}<{cpp_type}>, {name}, toNativeArguments<{cpp_type}>(info,
{index})' + (', info' if async else '')
On 2014/04/11 11:08:40, Nils Barth wrote:
> I think this would be clearer as a list, followed by ', '.join, i.e.,
> 
> cpp_type = idl_type.cpp_type
> macro_ars = ['%s<%s>' % (vector_type, cpp_type),
>              name,
>              'toNativeArguments<%s>(info, %s)' % (cpp_type, index)]
> if async:
>     macro_args.append('info')

Done.

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:260: return '{0}({1})'.format(macro,
macro_args_format.format(**macro_args))
On 2014/04/11 11:08:40, Nils Barth wrote:
> For simple positional format we prefer %, so this becomes:
> return '%s(%s)' % (macro, ', '.join(macro_args))

Done.

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/v8/Exce...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/v8/Exce...
Source/bindings/v8/ExceptionState.h:100: return
ScriptPromise::reject(V8ThrowException::createError(v8GeneralError, "Unknown
Error", m_isolate), m_isolate);
On 2014/04/11 13:57:13, haraken wrote:
> 
> Is this behavior speced? Which should we use, v8GeneralError or DOMError?
Domenic says[1] DOMException is more preferable than DOMError.

1:
https://github.com/domenic/promises-unwrapping/blob/master/docs/writing-speci...

https://codereview.chromium.org/232563003/diff/170001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/232563003/diff/170001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:2445: ScriptPromise
Internals::promiseCheck(ExecutionContext* context, long arg1, bool arg2, const
Dictionary& arg3, const String& arg4, ExceptionState& es)
On 2014/04/11 13:57:13, haraken wrote:
> 
> es => exceptionState

Done.

[Nils Barth (inactive), 2014-04-14 01:18:55]

(A few nits.)

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

https://codereview.chromium.org/232563003/diff/200001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:175: 'v8_value_to_local_cpp_value':
v8_value_to_local_cpp_value(argument, index, async=False),
Could you remove the async=False, since it's a default value?
(We don't want to clutter call sites just to support a special case.)

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

https://codereview.chromium.org/232563003/diff/200001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:267: {%- else %}
Could you comment the outer statements instead, or in addition?
{%- else %}{# method.is_constructor #}

(It's easy to keep track of the immediate inner block,
but the outer block is what's confusing.)

Also no need to repeat the 'if':
{%- endif %}{# method.idl_type == 'Promise' #}

https://codereview.chromium.org/232563003/diff/200001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:297: {%- else %}
Could you put {# method.idl_type != 'Promise' #} here,
due to the nested ifs?

[yhirano, 2014-04-14 06:12:41]

Because my changes in V8StringResource.h caused some layout test failures, I
fixed them. haraken, can you take a look at it again?

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/v8/V8St...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/232563003/diff/170001/Source/bindings/v8/V8St...
Source/bindings/v8/V8StringResource.h:204: return !m_v8Object.IsEmpty();
On 2014/04/11 13:57:13, haraken wrote:
> 
> I double-checked and confirmed that the new code looks equivalent to the old
> code. So, OK.

Actually, layout tests show that some regressions were introduced by this change
(fast/events/before/unload-return-bad-value.html, for example).

To fix these regressions, I added v8::TryCatch to TOSTRING_* macros.
On the other hand, in toNativeArguments and toNativeArray, I use prepare()
directly and don't call ReThrow(), because they are used in async cases as well.
It is OK because all existing toNativeArguments and toNativeArray calls are
enclosed by TONATIVE_*.

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

https://codereview.chromium.org/232563003/diff/200001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:175: 'v8_value_to_local_cpp_value':
v8_value_to_local_cpp_value(argument, index, async=False),
On 2014/04/14 01:18:55, Nils Barth wrote:
> Could you remove the async=False, since it's a default value?
> (We don't want to clutter call sites just to support a special case.)

Done.

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

https://codereview.chromium.org/232563003/diff/200001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:267: {%- else %}
On 2014/04/14 01:18:55, Nils Barth wrote:
> Could you comment the outer statements instead, or in addition?
> {%- else %}{# method.is_constructor #}
> 
> (It's easy to keep track of the immediate inner block,
> but the outer block is what's confusing.)
> 
> Also no need to repeat the 'if':
> {%- endif %}{# method.idl_type == 'Promise' #}

Done.

https://codereview.chromium.org/232563003/diff/200001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:297: {%- else %}
On 2014/04/14 01:18:55, Nils Barth wrote:
> Could you put {# method.idl_type != 'Promise' #} here,
> due to the nested ifs?

Done. I think using 'method.idl_type == 'Promise' for both else and endif
statements is better.

[Nils Barth (inactive), 2014-04-14 06:17:43]

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

https://codereview.chromium.org/232563003/diff/200001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:297: {%- else %}
On 2014/04/14 06:12:43, yhirano wrote:
> On 2014/04/14 01:18:55, Nils Barth wrote:
> > Could you put {# method.idl_type != 'Promise' #} here,
> > due to the nested ifs?
> 
> Done. I think using 'method.idl_type == 'Promise' for both else and endif
> statements is better.

That's fine, thanks!

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

https://codereview.chromium.org/232563003/diff/240001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:297: {%- else %}{# methods.idl_type ==
'Promise #}
Missing ' here.

[haraken, 2014-04-14 06:19:56]

On 2014/04/14 06:12:41, yhirano wrote:
> Because my changes in V8StringResource.h caused some layout test failures, I
> fixed them. haraken, can you take a look at it again?
> 
>
https://codereview.chromium.org/232563003/diff/170001/Source/bindings/v8/V8St...
> File Source/bindings/v8/V8StringResource.h (right):
> 
>
https://codereview.chromium.org/232563003/diff/170001/Source/bindings/v8/V8St...
> Source/bindings/v8/V8StringResource.h:204: return !m_v8Object.IsEmpty();
> On 2014/04/11 13:57:13, haraken wrote:
> > 
> > I double-checked and confirmed that the new code looks equivalent to the old
> > code. So, OK.
> 
> Actually, layout tests show that some regressions were introduced by this
change
> (fast/events/before/unload-return-bad-value.html, for example).
> 
> To fix these regressions, I added v8::TryCatch to TOSTRING_* macros.
> On the other hand, in toNativeArguments and toNativeArray, I use prepare()
> directly and don't call ReThrow(), because they are used in async cases as
well.
> It is OK because all existing toNativeArguments and toNativeArray calls are
> enclosed by TONATIVE_*.

Would you explain a bit more about why it caused regressions? I'm confused
because V8 already has a bug about TryCatch. I'd like to understand what part is
an expected implementation and what part is a hack to cover the V8 bug.

[yhirano, 2014-04-14 06:20:17]

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

https://codereview.chromium.org/232563003/diff/240001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:297: {%- else %}{# methods.idl_type ==
'Promise #}
On 2014/04/14 06:17:43, Nils Barth wrote:
> Missing ' here.
Thanks, done.

[yhirano, 2014-04-14 11:09:02]

Discussed with haraken: v8::Isolate detects if there are enclosing try-catchs
from callee side (see v8::internal::Isolate::ShuoldReportException for details).
PS14 moves TryCatch from V8ScriptResource<T>::prepare to TOSTRING_*. It is
consistent with other macros because all TO* macros other than _ASYNC ones place
TryCatch in it.

[yhirano, 2014-04-14 11:09:12]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-04-14 11:09:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/232563003/260001

[commit-bot: I haz the power, 2014-04-14 11:21:59]

Change committed as 171450

[haraken, 2014-04-14 12:17:37]

Discussed offline. LGTM.

[rmacnak, 2014-04-16 18:40:41]

https://codereview.chromium.org/232563003/diff/260001/Source/bindings/v8/Exce...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/232563003/diff/260001/Source/bindings/v8/Exce...
Source/bindings/v8/ExceptionState.h:96: ScriptPromise reject()
DBC: Should this be asserted to be unreachable for the four subclasses of
ExceptionState?

[yhirano, 2014-04-17 07:57:03]

Reopen: This change was reverted.
https://src.chromium.org/viewvc/blink?view=rev&revision=171848

[pneubeck (no reviews), 2014-04-17 09:27:04]

https://codereview.chromium.org/232563003/diff/260001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/232563003/diff/260001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8BindingMacros.h:108: #define
TONATIVE_BOOL_EXCEPTIONSTATE(type, var, value, exceptionState, retVal) \
As discussed with haraken@, this (and the other _BOOL_ macros) should be
_DEFAULT_ instead of _BOOL_, because there are/can be usages where retVal is not
boolean.

[Nils Barth (inactive), 2014-04-18 05:27:55]

https://codereview.chromium.org/232563003/diff/260001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/232563003/diff/260001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8BindingMacros.h:108: #define
TONATIVE_BOOL_EXCEPTIONSTATE(type, var, value, exceptionState, retVal) \
On 2014/04/17 09:27:04, pneubeck wrote:
> As discussed with haraken@, this (and the other _BOOL_ macros) should be
> _DEFAULT_ instead of _BOOL_, because there are/can be usages where retVal is
not
> boolean.

Fix posted:
Rename macros TONATIVE_BOOL* and TOSTRING_BOOL to *_DEFAULT
https://codereview.chromium.org/242223004/

[yhirano, 2014-04-18 16:55:53]

https://codereview.chromium.org/232563003/diff/260001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8BindingMacros.h (right):

https://codereview.chromium.org/232563003/diff/260001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8BindingMacros.h:108: #define
TONATIVE_BOOL_EXCEPTIONSTATE(type, var, value, exceptionState, retVal) \
On 2014/04/18 05:27:56, Nils Barth wrote:
> On 2014/04/17 09:27:04, pneubeck wrote:
> > As discussed with haraken@, this (and the other _BOOL_ macros) should be
> > _DEFAULT_ instead of _BOOL_, because there are/can be usages where retVal is
> not
> > boolean.
> 
> Fix posted:
> Rename macros TONATIVE_BOOL* and TOSTRING_BOOL to *_DEFAULT
> https://codereview.chromium.org/242223004/

Thanks!

[yhirano, 2014-04-21 09:08:57]

The V8StringResource.h change was landed separately as
https://src.chromium.org/viewvc/blink?view=rev&revision=172013 .
If the perf bots don't complain at the change, I would like to land this CL
again.

[haraken, 2014-04-21 09:10:49]

Sounds good. Let's give it another try :) LGTM.

[yhirano, 2014-07-30 10:04:03]

-nbarth
+bashi

I've updated the CL.
As mstarzinger's CL https://code.google.com/p/v8/source/detail?r=22481 landed, I
simplified my CL.

Can you take a look?

https://codereview.chromium.org/232563003/diff/430001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (left):

https://codereview.chromium.org/232563003/diff/430001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:966: class V8ResetTryCatchScope FINAL {
This class was introduced to cancel v8::TryCatch rethrow, but it turns out this
class is useless for the purpose. So I delete it.

[haraken, 2014-07-30 10:05:39]

+jl

[Jens Widell, 2014-07-30 10:29:11]

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:77: rethrow.cancel();                 
                                                     \
Would it work to do block.Reset() here instead? I think that would be marginally
nicer, since the rethrower them remains a simple stateless modifier.

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:85: V8RethrowTryCatchScope
rethrow(block);          \
Additionally, if we skip rethrow.cancel(), we could drop this rethrower, which
is entirely pointless (always cancelled.)

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:518: suffix = ''
This line duplicates the one above.

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:329: {% if method.idl_type == 'Promise' %}
It would be really nice if we could come up with something here (and below) that
leads to less duplication. In all cases, we create an exception and then either
use it construct a promise or just throw it. It would be good if the exception
creation was done by shared code.

(Doing that in a follow-up would be fine, of course.)

[bashi, 2014-07-30 11:47:35]

On 2014/07/30 10:04:03, yhirano wrote:
> -nbarth
> +bashi
> 
> I've updated the CL.
> As mstarzinger's CL https://code.google.com/p/v8/source/detail?r=22481 landed,
I
> simplified my CL.
> 
> Can you take a look?
> 
I'll take a look tomorrow. Sorry for the delay...

[bashi, 2014-07-31 02:02:25]

IDL compiler mostly looks good.

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:66: # use a v8::TryCatch.
Could you update the comment to describe why we need v8::TryCatch when async is
True?

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:195:
{{argument.v8_value_to_local_cpp_value_async}};
Could you explain why we need v8_value_to_local_cpp_value_async in addition to
v8_value_to_local_cpp_value? It seems that these variables are used only here so
I think we can merge them.

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/tests/i...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/tests/i...
Source/bindings/tests/idls/TestObject.idl:369: Promise promiseMethod(long arg1,
Dictionary arg2, DOMString arg3, DOMString... variadic);
Is it possible to use enums instead of Dictionary for arg2? I'm planning to
remove Dictionary from IDL files so it would be good if we can avoid adding
Dictionary.

[yhirano, 2014-07-31 03:03:25]

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:77: rethrow.cancel();                 
                                                     \
On 2014/07/30 10:29:10, Jens Lindström wrote:
> Would it work to do block.Reset() here instead? I think that would be
marginally
> nicer, since the rethrower them remains a simple stateless modifier.

Sadly, No.
When an exception is thrown and is caught by a v8::TryCatch block, the block has
to keep the exception object to swallow it.
Otherwise, the cancellation on the v8::TryCatch destructor doesn't take effect.
Hence we can't use Reset here.

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:85: V8RethrowTryCatchScope
rethrow(block);          \
On 2014/07/30 10:29:10, Jens Lindström wrote:
> Additionally, if we skip rethrow.cancel(), we could drop this rethrower, which
> is entirely pointless (always cancelled.)

I think we can't skip rethrow.cancel() at present.

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:518: suffix = ''
On 2014/07/30 10:29:10, Jens Lindström wrote:
> This line duplicates the one above.

Thanks, done.

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:329: {% if method.idl_type == 'Promise' %}
On 2014/07/30 10:29:11, Jens Lindström wrote:
> It would be really nice if we could come up with something here (and below)
that
> leads to less duplication. In all cases, we create an exception and then
either
> use it construct a promise or just throw it. It would be good if the exception
> creation was done by shared code.
> 
> (Doing that in a follow-up would be fine, of course.)

I would do that in a future CL. Added a FIXME.

[yhirano, 2014-07-31 03:38:03]

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:66: # use a v8::TryCatch.
On 2014/07/31 02:02:24, bashi1 wrote:
> Could you update the comment to describe why we need v8::TryCatch when async
is
> True?

Done.

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:195:
{{argument.v8_value_to_local_cpp_value_async}};
On 2014/07/31 02:02:24, bashi1 wrote:
> Could you explain why we need v8_value_to_local_cpp_value_async in addition to
> v8_value_to_local_cpp_value? It seems that these variables are used only here
so
> I think we can merge them.

Done.

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/tests/i...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/tests/i...
Source/bindings/tests/idls/TestObject.idl:369: Promise promiseMethod(long arg1,
Dictionary arg2, DOMString arg3, DOMString... variadic);
On 2014/07/31 02:02:24, bashi1 wrote:
> Is it possible to use enums instead of Dictionary for arg2? I'm planning to
> remove Dictionary from IDL files so it would be good if we can avoid adding
> Dictionary.

Can you tell me your plan?
I added Dictionary test case because Dictionary is specially handled.
Will the Dictionary specific code be deleted?
Will another non-general code be added?

[bashi, 2014-07-31 04:38:42]

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/tests/i...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/tests/i...
Source/bindings/tests/idls/TestObject.idl:369: Promise promiseMethod(long arg1,
Dictionary arg2, DOMString arg3, DOMString... variadic);
On 2014/07/31 03:38:03, yhirano wrote:
> On 2014/07/31 02:02:24, bashi1 wrote:
> > Is it possible to use enums instead of Dictionary for arg2? I'm planning to
> > remove Dictionary from IDL files so it would be good if we can avoid adding
> > Dictionary.
> 
> Can you tell me your plan?
In my plan, Dictionary will be replaced by specific IDL dictionaries. Here is an
example:
https://codereview.chromium.org/429853002/diff/20001/Source/core/css/FontFace...
https://codereview.chromium.org/429853002/diff/20001/Source/core/css/FontFace...

> I added Dictionary test case because Dictionary is specially handled.

Yes. It seems that enums are also handled in a similar way in
generate_argument() macro in method.cpp template, so I thought we may be able to
use enums instead.

> Will the Dictionary specific code be deleted?
> Will another non-general code be added?

"Dictionary" class specific code should be gone, but we will treat IDL
dictionaries(of which idl_type.is_dictionary is True) in a similar way of
"Dictionary" if necessary.

Anyway, I'm totally fine using Dictionary here if it's suitable for your
purpose.

[haraken, 2014-07-31 07:54:48]

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:953: class V8RethrowTryCatchScope FINAL {

Just to confirm: Do you have a plan to remove V8RethrowTryCatchScope once the
nested try-catch issue is fixed?

It's confusing to have V8RethrowTryCatchScope, cancel, etc.

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:955: explicit
V8RethrowTryCatchScope(v8::TryCatch& block) : m_block(block),
m_isCanceled(false) { }

Drop explicit.

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:85: V8RethrowTryCatchScope
rethrow(block);          \

Do we need this? It looks redundant to set a V8RethrowTryCatchScope and cancel
it later.

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:117: rethrow.cancel();                
                                                         \

Shall we add a comment and mention why we need to cancel it?

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:130:
TONATIVE_VOID_EXCEPTIONSTATE_INTERNAL(scriptState, var, value, exceptionState,
info);   \

Why doesn't this call TONATIVE_VOID_EXCEPTIONSTATE_ASYNC_INTERNAL?

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:154: rethrow.cancel();                
                                                         \

Just for consistency, let's move rethrow.cancel() to below line 155.

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:67: (base_type == 'DOMString' and not
argument.is_variadic and not async))

This condition is getting to a mess... but would be OK in this CL.

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:86: async = idl_type.name == 'Promise'

Shall we rename |async| to |return_promise| for clarity?

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:556: if macro ==
'TONATIVE_VOID_EXCEPTIONSTATE':

A slightly better condition would be:

  if idl_type.may_raise_exception_on_conversion:

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:557: args.insert(0,
'ScriptState::current(%s)' % isolate)

Can we move the ScriptState argument to the last argument and just use
args.append() ? (Basically we put the ScriptState at the first argument, but it
would be more important to keep the IDL compiler logic simpler.)

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:326: {# FIXME: reduce code duplication
between sync / async exception handling. #}

Yeah... would it be any way to avoid duplicating rejectXXXError code for
Promise?

[Jens Widell, 2014-07-31 08:29:09]

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:77: rethrow.cancel();                 
                                                     \
On 2014/07/31 03:03:25, yhirano wrote:
> On 2014/07/30 10:29:10, Jens Lindström wrote:
> > Would it work to do block.Reset() here instead? I think that would be
> marginally
> > nicer, since the rethrower them remains a simple stateless modifier.
> 
> Sadly, No.
> When an exception is thrown and is caught by a v8::TryCatch block, the block
has
> to keep the exception object to swallow it.
> Otherwise, the cancellation on the v8::TryCatch destructor doesn't take
effect.
> Hence we can't use Reset here.

That is unfortunate, and sounds like a bug in TryCatch to me. If I understand
correctly, calling ReThrow() after Reset() "resurrects" the exception? If that's
the case, then what *does* Reset() do?

Another aspect to consider here is that V8RethrowTryCatchScope is used in many
generated functions, so adding a little bit of logic to it may lead to a
noticable increase in binary size. (Or may not, since it's all inlined.)

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:953: class V8RethrowTryCatchScope FINAL {
On 2014/07/31 07:54:47, haraken wrote:
> 
> Just to confirm: Do you have a plan to remove V8RethrowTryCatchScope once the
> nested try-catch issue is fixed?

V8RethrowTryCatchScope isn't a workaround for the nested try-catch problem; it's
a more convenient way to call ReThrow() before returning. I don't see that we
have a reason to remove it, really.

[haraken, 2014-07-31 08:40:29]

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:953: class V8RethrowTryCatchScope FINAL {
On 2014/07/31 08:29:09, Jens Lindström wrote:
> On 2014/07/31 07:54:47, haraken wrote:
> > 
> > Just to confirm: Do you have a plan to remove V8RethrowTryCatchScope once
the
> > nested try-catch issue is fixed?
> 
> V8RethrowTryCatchScope isn't a workaround for the nested try-catch problem;
it's
> a more convenient way to call ReThrow() before returning. I don't see that we
> have a reason to remove it, really.
> 

Understood. Then it makes sense to have a cancel() method on
V8RethrowTryCatchScope.

[yhirano, 2014-08-01 02:27:58]

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:955: explicit
V8RethrowTryCatchScope(v8::TryCatch& block) : m_block(block),
m_isCanceled(false) { }
On 2014/07/31 07:54:47, haraken wrote:
> 
> Drop explicit.

It should be explicit, shouldn't it?

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:85: V8RethrowTryCatchScope
rethrow(block);          \
On 2014/07/31 07:54:47, haraken wrote:
> 
> Do we need this? It looks redundant to set a V8RethrowTryCatchScope and
cancel
> it later.

We need this to reuse the x_ASYNC_INTERNAL macro in a x_ASYNC macro.
I can delete it if I expand the x_ASYNC_INTERNAL macro.
What do you think?

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:117: rethrow.cancel();                
                                                         \
On 2014/07/31 07:54:47, haraken wrote:
> 
> Shall we add a comment and mention why we need to cancel it?

Done.

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:130:
TONATIVE_VOID_EXCEPTIONSTATE_INTERNAL(scriptState, var, value, exceptionState,
info);   \
On 2014/07/31 07:54:47, haraken wrote:
> 
> Why doesn't this call TONATIVE_VOID_EXCEPTIONSTATE_ASYNC_INTERNAL?

You're right. Thanks, done.

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:154: rethrow.cancel();                
                                                         \
On 2014/07/31 07:54:47, haraken wrote:
> 
> Just for consistency, let's move rethrow.cancel() to below line 155.

Done.

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:86: async = idl_type.name == 'Promise'
On 2014/07/31 07:54:47, haraken wrote:
> 
> Shall we rename |async| to |return_promise| for clarity?

Done.

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:556: if macro ==
'TONATIVE_VOID_EXCEPTIONSTATE':
On 2014/07/31 07:54:47, haraken wrote:
> 
> A slightly better condition would be:
> 
>   if idl_type.may_raise_exception_on_conversion:

That condition can be true when macro is 'TOSTRING_DEFAULT', right?
I don't want to provide the ScriptState in such a case.

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/scripts...
Source/bindings/scripts/v8_types.py:557: args.insert(0,
'ScriptState::current(%s)' % isolate)
On 2014/07/31 07:54:47, haraken wrote:
> 
> Can we move the ScriptState argument to the last argument and just use
> args.append() ? (Basically we put the ScriptState at the first argument, but
it
> would be more important to keep the IDL compiler logic simpler.)

Done.

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:326: {# FIXME: reduce code duplication
between sync / async exception handling. #}
On 2014/07/31 07:54:47, haraken wrote:
> 
> Yeah... would it be any way to avoid duplicating rejectXXXError code for
> Promise?

I think so. My current plan is to decouple exception creation and exception
throwing.
That way we can share the exception creation in both sync / async cases and we
can reduce reject variants in ScriptPromise.

[haraken, 2014-08-01 02:42:41]

(I'll take another close look in hours.)

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:955: explicit
V8RethrowTryCatchScope(v8::TryCatch& block) : m_block(block),
m_isCanceled(false) { }
On 2014/08/01 02:27:57, yhirano wrote:
> On 2014/07/31 07:54:47, haraken wrote:
> > 
> > Drop explicit.
> 
> It should be explicit, shouldn't it?

Sorry, I misread the code.

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:85: V8RethrowTryCatchScope
rethrow(block);          \
On 2014/08/01 02:27:57, yhirano wrote:
> On 2014/07/31 07:54:47, haraken wrote:
> > 
> > Do we need this? It looks redundant to set a V8RethrowTryCatchScope and
cancel
> > it later.
> 
> We need this to reuse the x_ASYNC_INTERNAL macro in a x_ASYNC macro.
> I can delete it if I expand the x_ASYNC_INTERNAL macro.
> What do you think?

Makes sense. Let's keep the x_ASYNC_INTERNAL macro as is for consistency with
other INTERNAL macros.

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

https://codereview.chromium.org/232563003/diff/490001/Source/bindings/templat...
Source/bindings/templates/methods.cpp:326: {# FIXME: reduce code duplication
between sync / async exception handling. #}
On 2014/08/01 02:27:57, yhirano wrote:
> On 2014/07/31 07:54:47, haraken wrote:
> > 
> > Yeah... would it be any way to avoid duplicating rejectXXXError code for
> > Promise?
> 
> I think so. My current plan is to decouple exception creation and exception
> throwing.
> That way we can share the exception creation in both sync / async cases and we
> can reduce reject variants in ScriptPromise.

Thanks, it's great you could fix it in a follow-up. I'm OK to have the
duplication in this CL.

[yhirano, 2014-08-01 03:52:54]

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/tests/i...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/tests/i...
Source/bindings/tests/idls/TestObject.idl:369: Promise promiseMethod(long arg1,
Dictionary arg2, DOMString arg3, DOMString... variadic);
On 2014/07/31 04:38:42, bashi1 wrote:
> On 2014/07/31 03:38:03, yhirano wrote:
> > On 2014/07/31 02:02:24, bashi1 wrote:
> > > Is it possible to use enums instead of Dictionary for arg2? I'm planning
to
> > > remove Dictionary from IDL files so it would be good if we can avoid
adding
> > > Dictionary.
> > 
> > Can you tell me your plan?
> In my plan, Dictionary will be replaced by specific IDL dictionaries. Here is
an
> example:
>
https://codereview.chromium.org/429853002/diff/20001/Source/core/css/FontFace...
>
https://codereview.chromium.org/429853002/diff/20001/Source/core/css/FontFace...
> 
> > I added Dictionary test case because Dictionary is specially handled.
> 
> Yes. It seems that enums are also handled in a similar way in
> generate_argument() macro in method.cpp template, so I thought we may be able
to
> use enums instead.
> 
> > Will the Dictionary specific code be deleted?
> > Will another non-general code be added?
> 
> "Dictionary" class specific code should be gone, but we will treat IDL
> dictionaries(of which idl_type.is_dictionary is True) in a similar way of
> "Dictionary" if necessary.
> 
> Anyway, I'm totally fine using Dictionary here if it's suitable for your
> purpose.

Thank you.
Let me leave it be to keep the test coverage.

[haraken, 2014-08-01 17:04:24]

LGTM

https://codereview.chromium.org/232563003/diff/510001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptPromise.h (right):

https://codereview.chromium.org/232563003/diff/510001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptPromise.h:121: // static functions each of which
returns a ScriptPromise rejected with

Add a FIXME to remove these functions.

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

https://codereview.chromium.org/232563003/diff/510001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:74: #define
TONATIVE_VOID_ASYNC_INTERNAL(var, value, info)                                  
          \

Given that this logic is specific to Promise, shall we rename ASYNC to PROMISE
for clarity?

https://codereview.chromium.org/232563003/diff/510001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/232563003/diff/510001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:853: arguments_need_try_catch =
any(v8_methods.argument_needs_try_catch(argument, async=False)

async => return_promise ?

The same comment for other parts of the IDL compiler.

[yhirano, 2014-08-04 01:42:08]

https://codereview.chromium.org/232563003/diff/510001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptPromise.h (right):

https://codereview.chromium.org/232563003/diff/510001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptPromise.h:121: // static functions each of which
returns a ScriptPromise rejected with
On 2014/08/01 17:04:24, haraken wrote:
> 
> Add a FIXME to remove these functions.

Done.

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

https://codereview.chromium.org/232563003/diff/510001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:74: #define
TONATIVE_VOID_ASYNC_INTERNAL(var, value, info)                                  
          \
On 2014/08/01 17:04:24, haraken wrote:
> 
> Given that this logic is specific to Promise, shall we rename ASYNC to PROMISE
> for clarity?

Done.

https://codereview.chromium.org/232563003/diff/510001/Source/bindings/scripts...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/232563003/diff/510001/Source/bindings/scripts...
Source/bindings/scripts/v8_interface.py:853: arguments_need_try_catch =
any(v8_methods.argument_needs_try_catch(argument, async=False)
On 2014/08/01 17:04:24, haraken wrote:
> 
> async => return_promise ?
> 
> The same comment for other parts of the IDL compiler.

Done.

[bashi, 2014-08-05 01:14:22]

LGTM

[yhirano, 2014-08-06 03:17:00]

jl@, do you have other comments?

[yhirano, 2014-08-06 03:21:13]

On 2014/08/06 03:17:00, yhirano wrote:
> jl@, do you have other comments?

Oh, sorry, I found I overlooked your comment.

[yhirano, 2014-08-06 03:21:17]

On 2014/08/06 03:17:00, yhirano wrote:
> jl@, do you have other comments?

Oh, sorry, I found I overlooked your comment.

[yhirano, 2014-08-06 03:21:20]

On 2014/08/06 03:17:00, yhirano wrote:
> jl@, do you have other comments?

Oh, sorry, I found I overlooked your comment.

[yhirano, 2014-08-06 05:48:48]

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:77: rethrow.cancel();                 
                                                     \
On 2014/07/31 08:29:09, Jens Lindström wrote:
> On 2014/07/31 03:03:25, yhirano wrote:
> > On 2014/07/30 10:29:10, Jens Lindström wrote:
> > > Would it work to do block.Reset() here instead? I think that would be
> > marginally
> > > nicer, since the rethrower them remains a simple stateless modifier.
> > 
> > Sadly, No.
> > When an exception is thrown and is caught by a v8::TryCatch block, the block
> has
> > to keep the exception object to swallow it.
> > Otherwise, the cancellation on the v8::TryCatch destructor doesn't take
> effect.
> > Hence we can't use Reset here.
> 
> That is unfortunate, and sounds like a bug in TryCatch to me. If I understand
> correctly, calling ReThrow() after Reset() "resurrects" the exception? If
that's
> the case, then what *does* Reset() do?
> 
> Another aspect to consider here is that V8RethrowTryCatchScope is used in many
> generated functions, so adding a little bit of logic to it may lead to a
> noticable increase in binary size. (Or may not, since it's all inlined.)
I've just created a V8 patch. https://codereview.chromium.org/443853002/
If it is accepted, we will be able to replace |rethrow.cancel()| with
|block.ReThrow()|.

[yhirano, 2014-08-11 06:29:35]

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

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:77: rethrow.cancel();                 
                                                     \
On 2014/08/06 05:48:47, yhirano wrote:
> On 2014/07/31 08:29:09, Jens Lindström wrote:
> > On 2014/07/31 03:03:25, yhirano wrote:
> > > On 2014/07/30 10:29:10, Jens Lindström wrote:
> > > > Would it work to do block.Reset() here instead? I think that would be
> > > marginally
> > > > nicer, since the rethrower them remains a simple stateless modifier.
> > > 
> > > Sadly, No.
> > > When an exception is thrown and is caught by a v8::TryCatch block, the
block
> > has
> > > to keep the exception object to swallow it.
> > > Otherwise, the cancellation on the v8::TryCatch destructor doesn't take
> > effect.
> > > Hence we can't use Reset here.
> > 
> > That is unfortunate, and sounds like a bug in TryCatch to me. If I
understand
> > correctly, calling ReThrow() after Reset() "resurrects" the exception? If
> that's
> > the case, then what *does* Reset() do?
> > 
> > Another aspect to consider here is that V8RethrowTryCatchScope is used in
many
> > generated functions, so adding a little bit of logic to it may lead to a
> > noticable increase in binary size. (Or may not, since it's all inlined.)
> I've just created a V8 patch. https://codereview.chromium.org/443853002/
> If it is accepted, we will be able to replace |rethrow.cancel()| with
> |block.ReThrow()|.

Done.

https://codereview.chromium.org/232563003/diff/450001/Source/bindings/core/v8...
Source/bindings/core/v8/V8BindingMacros.h:85: V8RethrowTryCatchScope
rethrow(block);          \
On 2014/07/30 10:29:10, Jens Lindström wrote:
> Additionally, if we skip rethrow.cancel(), we could drop this rethrower, which
> is entirely pointless (always cancelled.)

Done.

[Jens Widell, 2014-08-11 06:52:00]

On 2014/08/11 06:29:35, yhirano wrote:
> > I've just created a V8 patch. https://codereview.chromium.org/443853002/
> > If it is accepted, we will be able to replace |rethrow.cancel()| with
> > |block.ReThrow()|.
> 
> Done.
> 
>
https://codereview.chromium.org/232563003/diff/450001/Source/bindings/core/v8...
> Source/bindings/core/v8/V8BindingMacros.h:85: V8RethrowTryCatchScope
> rethrow(block);          \
> On 2014/07/30 10:29:10, Jens Lindström wrote:
> > Additionally, if we skip rethrow.cancel(), we could drop this rethrower,
which
> > is entirely pointless (always cancelled.)
> 
> Done.

Thanks a lot, that's awesome!

LGTM

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

https://codereview.chromium.org/232563003/diff/630001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:68: not return_promise))
Very minor nit: indent this line by one space.

[yhirano, 2014-08-11 07:00:26]

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

https://codereview.chromium.org/232563003/diff/630001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:68: not return_promise))
On 2014/08/11 06:52:00, Jens Lindström wrote:
> Very minor nit: indent this line by one space.

Done.

[yhirano, 2014-08-11 07:00:37]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-08-11 07:01:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/232563003/650001

[commit-bot: I haz the power, 2014-08-11 08:08:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21975)

[commit-bot: I haz the power, 2014-08-11 08:45:53]

Change committed as 179925