Bug 102870 – [V8] Replace V8Parameter::prepare() with V8Parameter::operator=()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

This is an incremental step for V8Parameter refactoring.

By replacing V8Parameter::prepare() with V8Parameter::operator=(), we can make EXCEPTION_BLOCK() and STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() equivalent (except for a return value).

In the next patch, I will remove STRING_TO_V8PARAMETER_EXCEPTION_BLOCK().

[Kentaro Hara]

Created attachment 175334 [details]
Patch

[Adam Barth]

Comment on attachment 175334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175334&action=review

> Source/WebCore/bindings/v8/V8BindingMacros.h:71
> -    type var(value);                                                 \
> +    type var;                                                        \
>      {                                                                \
>          v8::TryCatch block;                                          \
> -        var.prepare();                                               \
> +        var = (value);                                               \

Does it matter that you're moving the evaluation of (value) inside the TryCatch block?  Perhaps it matters if evaluating (value) throws an exception?

[Kentaro Hara]

(In reply to comment #2)
> > -    type var(value);                                                 \
> > +    type var;                                                        \
> >      {                                                                \
> >          v8::TryCatch block;                                          \
> > -        var.prepare();                                               \
> > +        var = (value);                                               \
> 
> Does it matter that you're moving the evaluation of (value) inside the TryCatch block?  Perhaps it matters if evaluating (value) throws an exception?

'=' throws an exception.

[Adam Barth]

> '=' throws an exception.

I understand that.  The issue is that evaluating (value) itself might throw and exception.  Because this is a macro and not an inline function, you've changed the time at which the expression (value) is evaluated.  Now it is evaluated in the context of the try block.

[Kentaro Hara]

(In reply to comment #4)
> > '=' throws an exception.
> 
> I understand that.  The issue is that evaluating (value) itself might throw and exception.  Because this is a macro and not an inline function, you've changed the time at which the expression (value) is evaluated.  Now it is evaluated in the context of the try block.

Ah, got it... So, to guarantee that this patch won't change any behavior, I should check that there is no STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() that is inside any TryCatch block, right?

c.f. EXCEPTION_BLOCK() evaluates (value) inside the try block.

[Adam Barth]

> Ah, got it... So, to guarantee that this patch won't change any behavior, I should check that there is no STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() that is inside any TryCatch block, right?
> 
> c.f. EXCEPTION_BLOCK() evaluates (value) inside the try block.

I doubt it's a problem, I just wanted to make sure you were aware of the (small) change you were making to the behavior.

[Kentaro Hara]

(In reply to comment #6)
> > Ah, got it... So, to guarantee that this patch won't change any behavior, I should check that there is no STRING_TO_V8PARAMETER_EXCEPTION_BLOCK() that is inside any TryCatch block, right?
> > 
> > c.f. EXCEPTION_BLOCK() evaluates (value) inside the try block.
> 
> I doubt it's a problem, I just wanted to make sure you were aware of the (small) change you were making to the behavior.

Thanks. Currently there is no (STRING_TO_V8PARAMETER_)EXCEPTION_BLOCK() that is inside a TryCatch block (Note: Due to a V8 bug (http://code.google.com/p/v8/issues/detail?id=2166), currently we cannot nest TryCatch blocks), so this change would be safe.

[WebKit Review Bot]

Comment on attachment 175334 [details]
Patch

Clearing flags on attachment: 175334

Committed r135359: <http://trac.webkit.org/changeset/135359>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.