Have V8WorkerGlobalScopeEventListener::callListenerFunction() use V8TRYCATCH_FOR_V8STRINGRESOURCE_R…

Have V8WorkerGlobalScopeEventListener::callListenerFunction() use V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN()

Have V8WorkerGlobalScopeEventListener::callListenerFunction() use V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN()
macro instead of toWebCoreString(v8::Handle<v8::Value>) so that we abort the function in case
v8::Value::ToString() threw an exception.

R=haraken

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

[haraken, 2013-11-15 16:30:00]

How many call sites are you going to change?

If it's many, I'm OK with landing CLs without tests (because it would take a
substantial amount of time to write a test case for this kind of change).
Otherwise, we might want to add a test case for each CL.

https://codereview.chromium.org/61933006/diff/1/Source/bindings/v8/V8WorkerGl...
File Source/bindings/v8/V8WorkerGlobalScopeEventListener.cpp (right):

https://codereview.chromium.org/61933006/diff/1/Source/bindings/v8/V8WorkerGl...
Source/bindings/v8/V8WorkerGlobalScopeEventListener.cpp:94:
V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN(V8StringResource<>, stringResourceName,
origin.ResourceName(), v8::Local<v8::Value>());

ah, I see. In this case we want to return v8::Local<v8::Value>().

[Inactive, 2013-11-15 16:35:31]

On 2013/11/15 16:30:00, haraken wrote:
> How many call sites are you going to change?
> 
> If it's many, I'm OK with landing CLs without tests (because it would take a
> substantial amount of time to write a test case for this kind of change).
> Otherwise, we might want to add a test case for each CL.

We still have quite a few call sites of toWebCoreString(v8::Handle<v8::Value>)
that we
need to get rid of. I don't mind writing test cases when possible. The issue
here is
that I am not sure how to test it:

How do I make v8::ScriptOrigin::ResourceName()::ToString() throw? I honestly
don't even know
by v8 is not returning a v8::String for the ResourceName.

Also this code path seems to be specific to the InspectorInstrumentation
timelineAgent.

[haraken, 2013-11-15 18:04:29]

> How do I make v8::ScriptOrigin::ResourceName()::ToString() throw? I honestly
> don't even know
> by v8 is not returning a v8::String for the ResourceName.
> 
> Also this code path seems to be specific to the InspectorInstrumentation
> timelineAgent.

This sounds hard to test. LGTM.

[commit-bot: I haz the power, 2013-11-15 19:07:37]

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

[commit-bot: I haz the power, 2013-11-15 21:19:33]

Change committed as 162135

[yurys, 2014-04-30 08:46:51]

https://codereview.chromium.org/61933006/diff/1/Source/bindings/v8/V8WorkerGl...
File Source/bindings/v8/V8WorkerGlobalScopeEventListener.cpp (right):

https://codereview.chromium.org/61933006/diff/1/Source/bindings/v8/V8WorkerGl...
Source/bindings/v8/V8WorkerGlobalScopeEventListener.cpp:94:
V8TRYCATCH_FOR_V8STRINGRESOURCE_RETURN(V8StringResource<>, stringResourceName,
origin.ResourceName(), v8::Local<v8::Value>());
This change looks wrong to me: if we fail to covert resource name to String here
the code will return and prevent execution of the instrumented functions. The
instrumentation must not affect normal control flow.

Next time you make changes that affect DevTools functionality please make sure
to include people from DevTools team who worked with the code, if you are not
sure who to ask feel free to add pfeldman or myself.