Have Dictionary use V8TRYCATCH_FOR_V8STRINGRESOURCE() macro

Have Dictionary use V8TRYCATCH_FOR_V8STRINGRESOURCE() macro

Have Dictionary use V8TRYCATCH_FOR_V8STRINGRESOURCE() macro instead of
the deprecated toWebCoreString().

R=haraken
BUG=

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

[do-not-use, 2013-09-10 11:50:13]

https://codereview.chromium.org/23812006/diff/1/Source/bindings/v8/V8StringRe...
File Source/bindings/v8/V8StringResource.h (right):

https://codereview.chromium.org/23812006/diff/1/Source/bindings/v8/V8StringRe...
Source/bindings/v8/V8StringResource.h:184: operator String() const { return
toString<String>(); }
I made these const because otherwise the following would not compile:
Vector<String>::append(V8StringResource) unless casting the argument to "const
String&".

Those operator overloads do not alter the state of the object anyway AFAIK.

[marja, 2013-09-10 11:58:58]

I don't know this part of the code well enough to review this; I hope
haraken@ will review.

[haraken, 2013-09-10 14:01:28]

This CL looks correct but changes behavior. We will throw exceptions for code
that hasn't thrown exceptions before. Would you add a test case?

[do-not-use, 2013-09-10 14:06:54]

On 2013/09/10 14:01:28, haraken wrote:
> This CL looks correct but changes behavior. We will throw exceptions for code
> that hasn't thrown exceptions before. Would you add a test case?

Fair enough :)

[do-not-use, 2013-09-10 14:26:14]

On 2013/09/10 14:01:28, haraken wrote:
> This CL looks correct but changes behavior. We will throw exceptions for code
> that hasn't thrown exceptions before. Would you add a test case?

I added a test case.

[haraken, 2013-09-10 16:34:26]

LGTM, thanks for the test.

We should replace almost all toWebCoreString with V8_TRYCATCH :)

[commit-bot: I haz the power, 2013-09-10 19:24:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/23812006/10001

[commit-bot: I haz the power, 2013-09-10 20:21:31]

Change committed as 157541

[do-not-use, 2013-09-11 07:03:04]

On 2013/09/10 16:34:26, haraken wrote:
> LGTM, thanks for the test.
> 
> We should replace almost all toWebCoreString with V8_TRYCATCH :)

Looking at the other toWebCoreString() uses, there are a lot of cases where the
input is already a v8::String or a v8::Value for which we already checked that
IsString() returns true. In such case, I don't believe it can throw an exception
and using V8_TRYCATCH would only decrease code readability IMHO (also do a
useless IsString() check).

v8StringToWebCoreString() may be a suitable alternative in this case?

     if (value->IsString())
-        return IDBKey::createString(toWebCoreString(value));
+        return
IDBKey::createString(v8StringToWebCoreString<String>(value.As<v8::String>(),
Externalize));

[haraken, 2013-09-11 07:12:49]

On 2013/09/11 07:03:04, Christophe Dumez wrote:
> On 2013/09/10 16:34:26, haraken wrote:
> > LGTM, thanks for the test.
> > 
> > We should replace almost all toWebCoreString with V8_TRYCATCH :)
> 
> Looking at the other toWebCoreString() uses, there are a lot of cases where
the
> input is already a v8::String or a v8::Value for which we already checked that
> IsString() returns true. In such case, I don't believe it can throw an
exception
> and using V8_TRYCATCH would only decrease code readability IMHO (also do a
> useless IsString() check).
> 
> v8StringToWebCoreString() may be a suitable alternative in this case?
> 
>      if (value->IsString())
> -        return IDBKey::createString(toWebCoreString(value));
> +        return
> IDBKey::createString(v8StringToWebCoreString<String>(value.As<v8::String>(),
> Externalize));

Yeah, you're right.

I just thought there are a lot of places where we're calling
ToString/ToNumber/ToInt etc without guarding the call with a TRYCATCH block.
However, I've not seriously checked the code base, so I'm not sure how much
we're really missing TRYCATH's.