[ABANDONED] Add exception handling options to V8StringResource<T>::prepare.

Add exception handling options to V8StringResource<T>::prepare.

Provide a way to change how to deal with an exception from ToString.
This is a preparation for another CL that enables Promise-related functions
to handle exceptions appropriately.
This CL is a refactoring CL and does not change behavior.

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

[yhirano, 2014-04-21 03:45:31]

I ran dromaeo on Mac and saw no performance regression.

[haraken, 2014-04-21 03:57:02]

LGTM with nits.

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

https://codereview.chromium.org/242913002/diff/60001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8BindingMacros.h:98: if
(UNLIKELY(!var.setStringAsPrimitive())) { \

Shall we rename as follows?

setStringAsPrimitive => toStringFast
setStringWithToString => toString

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

https://codereview.chromium.org/242913002/diff/60001/Source/bindings/v8/V8Str...
Source/bindings/v8/V8StringResource.h:197: bool setStringUsingToString();

What's this?

[yhirano, 2014-04-21 04:08:05]

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

https://codereview.chromium.org/242913002/diff/60001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8BindingMacros.h:98: if
(UNLIKELY(!var.setStringAsPrimitive())) { \
On 2014/04/21 03:57:02, haraken wrote:
> 
> Shall we rename as follows?
> 
> setStringAsPrimitive => toStringFast
> setStringWithToString => toString

Done.

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

https://codereview.chromium.org/242913002/diff/60001/Source/bindings/v8/V8Str...
Source/bindings/v8/V8StringResource.h:197: bool setStringUsingToString();
On 2014/04/21 03:57:02, haraken wrote:
> 
> What's this?

Thanks, I deleted it.

[yhirano, 2014-04-21 04:08:58]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-04-21 04:09:02]

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

[commit-bot: I haz the power, 2014-04-21 05:14:29]

Change committed as 172013

[dominicc (has gone to gerrit), 2014-04-22 05:54:26]

On 2014/04/21 05:14:29, I haz the power (commit-bot) wrote:
> Change committed as 172013

This regressed sizes >100K (~150K); see
<https://code.google.com/p/chromium/issues/detail?id=365265>

[haraken, 2014-04-22 05:58:18]

On 2014/04/22 05:54:26, dominicc wrote:
> On 2014/04/21 05:14:29, I haz the power (commit-bot) wrote:
> > Change committed as 172013
> 
> This regressed sizes >100K (~150K); see
> <https://code.google.com/p/chromium/issues/detail?id=365265>

We're recently sensitive about the binding binary size, so 150 KB increase looks
unacceptable.

yhirano: would you take a look at this?

[yhirano, 2014-04-22 07:13:10]

Reopen

[yhirano, 2014-04-24 06:52:50]

PTAL
PS19 doesn't show significant size / speed regressions in my local environment.

https://codereview.chromium.org/242913002/diff/340001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8Binding.h (left):

https://codereview.chromium.org/242913002/diff/340001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8Binding.h:538: static inline String nativeValue(const
v8::Handle<v8::Value>& value, v8::Isolate* isolate)
Because this function is used in another v8::TryCatch, we don't have to detect
the exception here.

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

https://codereview.chromium.org/242913002/diff/340001/Source/bindings/v8/V8St...
Source/bindings/v8/V8StringResource.h:168: struct V8StringResourceToString;
I know this is a bit complex, but It is needed to avoid size / speed
regressions.

https://codereview.chromium.org/242913002/diff/340001/Source/bindings/v8/V8St...
Source/bindings/v8/V8StringResource.h:196: struct
V8StringResourceToString<V8StringResourceStoreException> {
Nobody uses this class yet, but I wrote it as a preparation for the following CL
https://codereview.chromium.org/232563003/ .

[haraken, 2014-04-24 08:03:30]

Hmm, it's a shame that we need to introduce such complexity into Blink because
V8 doesn't support nestable TryCatch.

A right fix would be to make v8::TryCatch nestable. I'm asking it to the V8 team
here: https://code.google.com/p/chromium/issues/detail?id=362388

[haraken, 2014-05-16 14:20:51]

On 2014/04/24 08:03:30, haraken wrote:
> Hmm, it's a shame that we need to introduce such complexity into Blink because
> V8 doesn't support nestable TryCatch.
> 
> A right fix would be to make v8::TryCatch nestable. I'm asking it to the V8
team
> here: https://code.google.com/p/chromium/issues/detail?id=362388

yhirano@: Michael made a CL that makes TryCatch nestable for us.
https://codereview.chromium.org/287133005/

The CL is a bit hacky, so we want to make sure that the CL really fixes your
issue before landing. Would you mind checking that the CL resolves the TryCatch
issue?

[yhirano, 2014-05-19 08:02:13]

On 2014/05/16 14:20:51, haraken wrote:
> On 2014/04/24 08:03:30, haraken wrote:
> > Hmm, it's a shame that we need to introduce such complexity into Blink
because
> > V8 doesn't support nestable TryCatch.
> > 
> > A right fix would be to make v8::TryCatch nestable. I'm asking it to the V8
> team
> > here: https://code.google.com/p/chromium/issues/detail?id=362388
> 
> yhirano@: Michael made a CL that makes TryCatch nestable for us.
> https://codereview.chromium.org/287133005/
> 
> The CL is a bit hacky, so we want to make sure that the CL really fixes your
> issue before landing. Would you mind checking that the CL resolves the
TryCatch
> issue?

Thanks, I will confirm.

[yhirano, 2014-05-20 05:49:49]

On 2014/05/19 08:02:13, yhirano wrote:
> On 2014/05/16 14:20:51, haraken wrote:
> > On 2014/04/24 08:03:30, haraken wrote:
> > > Hmm, it's a shame that we need to introduce such complexity into Blink
> because
> > > V8 doesn't support nestable TryCatch.
> > > 
> > > A right fix would be to make v8::TryCatch nestable. I'm asking it to the
V8
> > team
> > > here: https://code.google.com/p/chromium/issues/detail?id=362388
> > 
> > yhirano@: Michael made a CL that makes TryCatch nestable for us.
> > https://codereview.chromium.org/287133005/
> > 
> > The CL is a bit hacky, so we want to make sure that the CL really fixes your
> > issue before landing. Would you mind checking that the CL resolves the
> TryCatch
> > issue?
> 
> Thanks, I will confirm.

Confirmed.
Only fast/js/Promise-bindings-check-exception.html fails with [1] (see the try
result), but it passes with [1] + [2] in my local environment.

1: https://codereview.chromium.org/232563003/#ps370001
2: https://codereview.chromium.org/287133005/#ps1

[haraken, 2014-05-20 06:41:31]

On 2014/05/20 05:49:49, yhirano wrote:
> On 2014/05/19 08:02:13, yhirano wrote:
> > On 2014/05/16 14:20:51, haraken wrote:
> > > On 2014/04/24 08:03:30, haraken wrote:
> > > > Hmm, it's a shame that we need to introduce such complexity into Blink
> > because
> > > > V8 doesn't support nestable TryCatch.
> > > > 
> > > > A right fix would be to make v8::TryCatch nestable. I'm asking it to the
> V8
> > > team
> > > > here: https://code.google.com/p/chromium/issues/detail?id=362388
> > > 
> > > yhirano@: Michael made a CL that makes TryCatch nestable for us.
> > > https://codereview.chromium.org/287133005/
> > > 
> > > The CL is a bit hacky, so we want to make sure that the CL really fixes
your
> > > issue before landing. Would you mind checking that the CL resolves the
> > TryCatch
> > > issue?
> > 
> > Thanks, I will confirm.
> 
> Confirmed.
> Only fast/js/Promise-bindings-check-exception.html fails with [1] (see the try
> result), but it passes with [1] + [2] in my local environment.
> 
> 1: https://codereview.chromium.org/232563003/#ps370001
> 2: https://codereview.chromium.org/287133005/#ps1

Thank you! I'll chat with Micheal and land the V8 side CL.

[haraken, 2014-05-21 14:01:43]

On 2014/05/20 06:41:31, haraken wrote:
> On 2014/05/20 05:49:49, yhirano wrote:
> > On 2014/05/19 08:02:13, yhirano wrote:
> > > On 2014/05/16 14:20:51, haraken wrote:
> > > > On 2014/04/24 08:03:30, haraken wrote:
> > > > > Hmm, it's a shame that we need to introduce such complexity into Blink
> > > because
> > > > > V8 doesn't support nestable TryCatch.
> > > > > 
> > > > > A right fix would be to make v8::TryCatch nestable. I'm asking it to
the
> > V8
> > > > team
> > > > > here: https://code.google.com/p/chromium/issues/detail?id=362388
> > > > 
> > > > yhirano@: Michael made a CL that makes TryCatch nestable for us.
> > > > https://codereview.chromium.org/287133005/
> > > > 
> > > > The CL is a bit hacky, so we want to make sure that the CL really fixes
> your
> > > > issue before landing. Would you mind checking that the CL resolves the
> > > TryCatch
> > > > issue?
> > > 
> > > Thanks, I will confirm.
> > 
> > Confirmed.
> > Only fast/js/Promise-bindings-check-exception.html fails with [1] (see the
try
> > result), but it passes with [1] + [2] in my local environment.
> > 
> > 1: https://codereview.chromium.org/232563003/#ps370001
> > 2: https://codereview.chromium.org/287133005/#ps1
> 
> Thank you! I'll chat with Micheal and land the V8 side CL.

FYI: The V8-side fix is going to land just right now. The V8 roll will be made
by the end of this week.

[yhirano, 2014-08-11 12:34:15]

Close this CL as the problem was solved in another way.