Pass isolate to v8::Local<>::New() factory function

Pass isolate to v8::Local<>::New() factory function

Pass isolate to v8::Local<>::New() factory function to avoid implicit calls
to v8::Isolate::GetCurrent().

R=haraken
BUG=263412
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157774

[do-not-use, 2013-09-12 07:05:12]

Adding abarth for Source/web

[marja, 2013-09-12 07:41:20]

drive-by, lg except these 2

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

https://codereview.chromium.org/23876015/diff/1/Source/bindings/v8/V8WorkerGl...
Source/bindings/v8/V8WorkerGlobalScopeEventListener.cpp:117: v8::Isolate*
isolate = toV8Context(context, world())->GetIsolate();
I think you should use getIsolateFromScriptExecutionContext() here.

https://codereview.chromium.org/23876015/diff/1/Source/web/WebFrameImpl.cpp
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/23876015/diff/1/Source/web/WebFrameImpl.cpp#n...
Source/web/WebFrameImpl.cpp:860: v8Results[i] =
v8::Local<v8::Value>::New(frame()->script()->currentWorldContext()->GetIsolate(),
scriptResults[i].v8Value());
Oops, we should also have getIsolateFromFrame, at least I claim in
https://chromiumcodereview.appspot.com/23788005/ that I added it, but something
went wrong :/ Anyhow, I'm fixing it here:
https://codereview.chromium.org/24119002/ and this CL should use that func.

[haraken, 2013-09-12 09:10:36]

On 2013/09/12 07:41:20, marja wrote:
>
https://codereview.chromium.org/23876015/diff/1/Source/web/WebFrameImpl.cpp#n...
> Source/web/WebFrameImpl.cpp:860: v8Results[i] =
>
v8::Local<v8::Value>::New(frame()->script()->currentWorldContext()->GetIsolate(),
> scriptResults[i].v8Value());
> Oops, we should also have getIsolateFromFrame, at least I claim in
> https://chromiumcodereview.appspot.com/23788005/ that I added it, but
something
> went wrong :/ Anyhow, I'm fixing it here:
> https://codereview.chromium.org/24119002/ and this CL should use that func.

It's a bit annoying to have getIsolateFromScriptExecutionContext(),
getIsolateFromFrame(), getIsolateFromXXX() etc. How about just unifying them to
overloaded getIsolate() ? i.e., getIsolate(ScriptExecutionContext*),
getIsolate(Frame*), getIsolate(XXX*). Then caller sites will look nicer. You can
add more XXX to simplify the code base. WDYT?

LGTM anyway. You can make the getIsolate() fix in follow-up CLs (if we agree
with getIsolate()).

[marja, 2013-09-12 09:12:09]

pfeldman pointed out that Blink coding style forbids the "get" prefix.

So the functions are now "isolateForScriptExecutionContext" and
"isolateForFrame".

[haraken, 2013-09-12 09:21:32]

On 2013/09/12 09:12:09, marja wrote:
> pfeldman pointed out that Blink coding style forbids the "get" prefix.
> 
> So the functions are now "isolateForScriptExecutionContext" and
> "isolateForFrame".

Right. That sounds nice. Then we won't want to introduce
isolate(ScriptExecutionContext*), isolate(Frame*) etc because they will conflict
with a variable name 'isolate'.

[do-not-use, 2013-09-12 10:16:04]

Ok, I will clean up the patch once Marja's CL lands:
https://codereview.chromium.org/24119002/

[do-not-use, 2013-09-12 12:07:31]

https://codereview.chromium.org/23876015/diff/1/Source/web/WebFrameImpl.cpp
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/23876015/diff/1/Source/web/WebFrameImpl.cpp#n...
Source/web/WebFrameImpl.cpp:860: v8Results[i] =
v8::Local<v8::Value>::New(frame()->script()->currentWorldContext()->GetIsolate(),
scriptResults[i].v8Value());
On 2013/09/12 07:41:21, marja wrote:
> Oops, we should also have getIsolateFromFrame, at least I claim in
> https://chromiumcodereview.appspot.com/23788005/ that I added it, but
something
> went wrong :/ Anyhow, I'm fixing it here:
> https://codereview.chromium.org/24119002/ and this CL should use that func.

isolateForFrame() does:
frame->script()->isolate()

while this file does:
frame()->script()->currentWorldContext()->GetIsolate()

Is this really the same? Can I safely replace?

[marja, 2013-09-12 12:27:57]

Afaics, they should return the same isolate.

[do-not-use, 2013-09-12 12:55:43]

I updated the patch to use isolateForScriptExecutionContext() /
isolateForFrame(). PTAL.

[marja, 2013-09-12 12:58:13]

lgtm

[do-not-use, 2013-09-13 16:01:55]

abarth@, could you please take a look for Source/web ?

[abarth-chromium, 2013-09-13 17:47:08]

lgtm

https://codereview.chromium.org/23876015/diff/17001/Source/web/WebFrameImpl.cpp
File Source/web/WebFrameImpl.cpp (right):

https://codereview.chromium.org/23876015/diff/17001/Source/web/WebFrameImpl.c...
Source/web/WebFrameImpl.cpp:882: return
toV8(DOMFileSystem::create(frame()->document(), name,
static_cast<WebCore::FileSystemType>(type), KURL(ParsedURLString,
path.utf8().data())), v8::Handle<v8::Object>(), isolateForFrame(frame()));
Should we just call isolateForFrame toIsolate?  We can use C++ overloading to
select the right implementation based on the argument type.

[commit-bot: I haz the power, 2013-09-13 17:47:18]

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

[commit-bot: I haz the power, 2013-09-13 18:23:30]

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

[commit-bot: I haz the power, 2013-09-13 19:38:22]

Change committed as 157774