[bindings] [devtools] Migrate the usage of ScriptValue.toJSONValue() to ScriptValue::to<JSONValuePt…

[bindings] [devtools] Migrate the usage of ScriptValue.toJSONValue() to ScriptValue::to<JSONValuePtr>()

Now that the CL [1] added method, ScriptValue::to<T>, supports conversion of values using 
the NativeValueTraits<T>, we should replace the usages of toJSONValue with the above API.

R=haraken@chromium.org, yurys@chromium.org

[1] https://codereview.chromium.org/1009503003

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

[vivekg, 2015-03-20 15:12:38]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-03-20 15:12:39]

This is a WIP patch. 

haraken@: I tried removing the ScriptState::Scope creation but that results in
all the inspector tests reporting "Renderer crashed". I think this is due to the
way inspector operates with various ScriptState's. 

Not sure, but either you or yurys@ can throw some light. Thanks.

[vivekg, 2015-03-24 04:46:11]

ping!

[haraken, 2015-03-24 04:57:29]

What's the crash trace?

[vivekg, 2015-03-24 05:21:49]

https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
File Source/core/inspector/InjectedScriptBase.cpp (right):

https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
Source/core/inspector/InjectedScriptBase.cpp:51: ScriptState::Scope
scope(scriptState);
When I remove above scope, we get this crash log:
http://paste.ubuntu.com/10666566/ 
I just ran a sample test: inspector/console/alert-toString-exception.html but
the crash log is same for all the tests related to inspector.

[vivekg, 2015-03-24 15:07:33]

On 2015/03/24 at 05:21:49, vivekg_ wrote:
>
https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
> File Source/core/inspector/InjectedScriptBase.cpp (right):
> 
>
https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
> Source/core/inspector/InjectedScriptBase.cpp:51: ScriptState::Scope
scope(scriptState);
> When I remove above scope, we get this crash log:
http://paste.ubuntu.com/10666566/ 
> I just ran a sample test: inspector/console/alert-toString-exception.html but
the crash log is same for all the tests related to inspector.

haraken@: Any idea about why are we hitting this crash without the Scope
creation?

[haraken, 2015-03-24 15:23:10]

https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
File Source/core/inspector/InjectedScript.cpp (right):

https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
Source/core/inspector/InjectedScript.cpp:283: RefPtr<JSONValue> result =
toJSONValue(scriptState(), callFramesValue);

I think we are not in a ScriptState here. We need to enter the ScriptState. For
experiment, you can insert ASSERT(scriptState() == ScriptState::current()).

So in summary, I think the ScriptState::Scope in toJSONValue would be needed.

https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
Source/core/inspector/InjectedScript.cpp:301: RefPtr<JSONObject> rawResult =
toJSONValue(scriptState(), r)->asObject();

Ditto.

https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
File Source/core/inspector/InjectedScriptBase.cpp (right):

https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
Source/core/inspector/InjectedScriptBase.cpp:48: PassRefPtr<JSONValue>
toJSONValue(ScriptState* scriptState, const ScriptValue& value)

BTW, what's a relationship between scriptState and value.scriptState()? Are they
the same?

[vivekg, 2015-03-25 04:20:35]

On 2015/03/24 at 15:23:10, haraken wrote:
>
https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
> File Source/core/inspector/InjectedScript.cpp (right):
> 
>
https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
> Source/core/inspector/InjectedScript.cpp:283: RefPtr<JSONValue> result =
toJSONValue(scriptState(), callFramesValue);
> 
> I think we are not in a ScriptState here. We need to enter the ScriptState.
For experiment, you can insert ASSERT(scriptState() == ScriptState::current()).
> 
> So in summary, I think the ScriptState::Scope in toJSONValue would be needed.
> 

Sure, we retain as is.


>
https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
> File Source/core/inspector/InjectedScriptBase.cpp (right):
> 
>
https://codereview.chromium.org/1020233002/diff/1/Source/core/inspector/Injec...
> Source/core/inspector/InjectedScriptBase.cpp:48: PassRefPtr<JSONValue>
toJSONValue(ScriptState* scriptState, const ScriptValue& value)
> 
> BTW, what's a relationship between scriptState and value.scriptState()? Are
they the same?

These are same so removing the explicit passage of ScriptState arg.

Updated the patch, PTAL.

[haraken, 2015-03-25 04:39:16]

LGTM

https://codereview.chromium.org/1020233002/diff/20001/Source/core/inspector/I...
File Source/core/inspector/InjectedScriptBase.cpp (right):

https://codereview.chromium.org/1020233002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptBase.cpp:54: return
ScriptValue::to<JSONValuePtr>(scriptState->isolate(), value, exceptionState);

Nit: As far as I see V8Binding.h, toScriptValue (not ScriptValue::to) might be
more consistent with other conversion methods. It seems bindings use Value::from
and toValue.

https://codereview.chromium.org/1020233002/diff/20001/Source/core/inspector/I...
Source/core/inspector/InjectedScriptBase.cpp:179: *result = 
toJSONValue(resultValue);

Unnecessary space.

[vivekg, 2015-03-25 04:49:59]

On 2015/03/25 at 04:39:16, haraken wrote:
> LGTM
> 

Thank you.

>
https://codereview.chromium.org/1020233002/diff/20001/Source/core/inspector/I...
> File Source/core/inspector/InjectedScriptBase.cpp (right):
> 
>
https://codereview.chromium.org/1020233002/diff/20001/Source/core/inspector/I...
> Source/core/inspector/InjectedScriptBase.cpp:54: return
ScriptValue::to<JSONValuePtr>(scriptState->isolate(), value, exceptionState);
> 
> Nit: As far as I see V8Binding.h, toScriptValue (not ScriptValue::to) might be
more consistent with other conversion methods. It seems bindings use Value::from
and toValue.

Sure, I will do it in a follow up patch as this is also used in IDB module.

> 
>
https://codereview.chromium.org/1020233002/diff/20001/Source/core/inspector/I...
> Source/core/inspector/InjectedScriptBase.cpp:179: *result = 
toJSONValue(resultValue);
> 
> Unnecessary space.

Done.

[vivekg, 2015-03-25 05:55:10]

The CQ bit was checked by vivekg@chromium.org

[vivekg, 2015-03-25 05:55:10]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1020233002/#ps40001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-03-25 05:55:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1020233002/40001

[vivekg, 2015-03-25 06:11:00]

On 2015/03/25 at 04:39:16, haraken wrote:
> 
>
https://codereview.chromium.org/1020233002/diff/20001/Source/core/inspector/I...
> Source/core/inspector/InjectedScriptBase.cpp:54: return
ScriptValue::to<JSONValuePtr>(scriptState->isolate(), value, exceptionState);
> 
> Nit: As far as I see V8Binding.h, toScriptValue (not ScriptValue::to) might be
more consistent with other conversion methods. It seems bindings use Value::from
and toValue.
> 

Actually the method ScriptValue::to is returning the T so calling it
toScriptValue would not be appropriate.
I moved it to be a static method of ScriptValue so as the call site can read it
like "ScriptValue::to<T>(...)" in literal terms. :) 
Have you got any better naming here?

[commit-bot: I haz the power, 2015-03-25 06:25:00]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192499

[haraken, 2015-03-25 12:20:45]

On 2015/03/25 06:11:00, vivekg_ wrote:
> On 2015/03/25 at 04:39:16, haraken wrote:
> > 
> >
>
https://codereview.chromium.org/1020233002/diff/20001/Source/core/inspector/I...
> > Source/core/inspector/InjectedScriptBase.cpp:54: return
> ScriptValue::to<JSONValuePtr>(scriptState->isolate(), value, exceptionState);
> > 
> > Nit: As far as I see V8Binding.h, toScriptValue (not ScriptValue::to) might
be
> more consistent with other conversion methods. It seems bindings use
Value::from
> and toValue.
> > 
> 
> Actually the method ScriptValue::to is returning the T so calling it
> toScriptValue would not be appropriate.
> I moved it to be a static method of ScriptValue so as the call site can read
it
> like "ScriptValue::to<T>(...)" in literal terms. :) 
> Have you got any better naming here?

Your point sounds reasonable :)

[vivekg, 2015-03-25 12:37:38]

On 2015/03/25 at 12:20:45, haraken wrote:
> On 2015/03/25 06:11:00, vivekg_ wrote:
> > On 2015/03/25 at 04:39:16, haraken wrote:
> > > 
> > >
> >
https://codereview.chromium.org/1020233002/diff/20001/Source/core/inspector/I...
> > > Source/core/inspector/InjectedScriptBase.cpp:54: return
> > ScriptValue::to<JSONValuePtr>(scriptState->isolate(), value,
exceptionState);
> > > 
> > > Nit: As far as I see V8Binding.h, toScriptValue (not ScriptValue::to)
might be
> > more consistent with other conversion methods. It seems bindings use
Value::from
> > and toValue.
> > > 
> > 
> > Actually the method ScriptValue::to is returning the T so calling it
> > toScriptValue would not be appropriate.
> > I moved it to be a static method of ScriptValue so as the call site can read
it
> > like "ScriptValue::to<T>(...)" in literal terms. :) 
> > Have you got any better naming here?
> 
> Your point sounds reasonable :)

hehe, thank you. So we will retain it as is.