Replace several uses of toWebCoreString() by V8TRYCATCH_FOR_V8STRINGRESOURCE() macro

Replace several uses of toWebCoreString() by V8TRYCATCH_FOR_V8STRINGRESOURCE() macro

Replace several uses of toWebCoreString() by V8TRYCATCH_FOR_V8STRINGRESOURCE() macro so
that we return as soon as the string conversion fails and to avoid passing a null
string to the following code.

R=haraken
BUG=

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

[haraken, 2013-09-12 17:33:51]

Ideally we want to have tests for each exception you're adding, but it will be a
substantial amount of work for you. Since this is a minor change, I'm OK with
landing the change without tests, if there is no objection from other reviewers.

https://codereview.chromium.org/23526039/diff/1/Source/bindings/v8/V8Binding.h
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/23526039/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:394:
V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN(V8StringResource<>, stringValue, value,
String());

This change wouldn't be helpful until you fix all caller sites of nativeValue()
so that the caller sites return immediately when nativeValue() throws
exceptions.

However, I'm afraid that it will mess up the code base. Do you have any good
idea?

https://codereview.chromium.org/23526039/diff/1/Source/bindings/v8/custom/V8H...
File Source/bindings/v8/custom/V8HTMLAllCollectionCustom.cpp (right):

https://codereview.chromium.org/23526039/diff/1/Source/bindings/v8/custom/V8H...
Source/bindings/v8/custom/V8HTMLAllCollectionCustom.cpp:65:
V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN(V8StringResource<>, name, argument,
v8::Undefined(callbackInfo.GetIsolate()));

Ditto.

https://codereview.chromium.org/23526039/diff/1/Source/bindings/v8/custom/V8H...
File Source/bindings/v8/custom/V8HTMLDocumentCustom.cpp (right):

https://codereview.chromium.org/23526039/diff/1/Source/bindings/v8/custom/V8H...
Source/bindings/v8/custom/V8HTMLDocumentCustom.cpp:64:
V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN(V8StringResource<>, stringArgument,
args[i], String());

Ditto.

[do-not-use, 2013-09-12 17:39:11]

On 2013/09/12 17:33:51, haraken wrote:
> Ideally we want to have tests for each exception you're adding, but it will be
a
> substantial amount of work for you. Since this is a minor change, I'm OK with
> landing the change without tests, if there is no objection from other
reviewers.

I don't understand why I am adding new exceptions. AFAIK, toWebCoreString()
already throws on JS side. The only difference is that we the macro, we can
return early on native side instead of continuing with a null String.

[haraken, 2013-09-12 18:14:09]

On 2013/09/12 17:39:11, Christophe Dumez wrote:
> On 2013/09/12 17:33:51, haraken wrote:
> > Ideally we want to have tests for each exception you're adding, but it will
be
> a
> > substantial amount of work for you. Since this is a minor change, I'm OK
with
> > landing the change without tests, if there is no objection from other
> reviewers.
> 
> I don't understand why I am adding new exceptions. AFAIK, toWebCoreString()
> already throws on JS side. The only difference is that we the macro, we can
> return early on native side instead of continuing with a null String.

Sorry, right. I should have said that we might want to have tests because we're
changing existing behavior (not because we're adding new exceptions).

[do-not-use, 2013-09-13 14:55:49]

https://codereview.chromium.org/23526039/diff/1/Source/bindings/v8/V8Binding.h
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/23526039/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.h:394:
V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN(V8StringResource<>, stringValue, value,
String());
On 2013/09/12 17:33:52, haraken wrote:
> 
> This change wouldn't be helpful until you fix all caller sites of
nativeValue()
> so that the caller sites return immediately when nativeValue() throws
> exceptions.
> 
> However, I'm afraid that it will mess up the code base. Do you have any good
> idea?

The reason I used V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN() here even though it
does not change the behavior is because I was planning to have toWebCoreString()
take a v8::String instead of a v8::Value in a follow-up CL.

[haraken, 2013-09-13 15:09:28]

> The reason I used V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN() here even though it
> does not change the behavior is because I was planning to have
toWebCoreString()
> take a v8::String instead of a v8::Value in a follow-up CL.

This sounds like a good idea.

As mentioned above, using V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN() in
nativeValue (and other two places) isn't helpful until you fix all call sites of
nativeValue. However, this CL is not making anything worse and is a necessary
step to make something better, so LGTM.

[commit-bot: I haz the power, 2013-09-13 17:44:07]

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

[commit-bot: I haz the power, 2013-09-13 19:37:32]

Change committed as 157772