Pass isolate to ScriptPromise and ScriptString constructors

Pass isolate to ScriptPromise and ScriptString constructors

Pass isolate to ScriptPromise and ScriptString constructors to avoid calls
to v8::Isolate::GetCurrent(). Also add an m_isolate member to ScriptValue
to avoid avoid calls to v8::Isolate::GetCurrent() and simplify the API.

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

[haraken, 2013-09-18 00:25:12]

LGTM with comments.

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
File Source/bindings/v8/ScriptValue.h (right):

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
Source/bindings/v8/ScriptValue.h:61: : m_isolate(0)

Is there any reason why we can't call Isolate::GetCurrent() here and ensure that
m_isolate is not 0 ?

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
Source/bindings/v8/ScriptValue.h:78: v8::Isolate* isolate() const { return
m_isolate; }

Even if we cannot ensure that m_isolate is not 0 for some reason, we want to
ensure that isolate() doesn't return 0. Hence we want to write:

  v8::Isolate* isolate() const { if(!m_isolate) m_isolate =
Isolate::GetCurrent(); return m_isolate; }

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
Source/bindings/v8/ScriptValue.h:167: m_isolate = 0;

Do we need to clear an isolate? In other words, is it possible that we reuse the
same ScriptPromise in different isolates?

[do-not-use, 2013-09-18 06:35:15]

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
File Source/bindings/v8/ScriptValue.h (right):

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
Source/bindings/v8/ScriptValue.h:61: : m_isolate(0)
On 2013/09/18 00:25:12, haraken wrote:
> 
> Is there any reason why we can't call Isolate::GetCurrent() here and ensure
that
> m_isolate is not 0 ?

My understanding was that we wanted to avoid calls to Isolate::GetCurrent() as
much as possible. I expect there are many cases where we construct an empty
StringValue and don't need an isolate at all.

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
Source/bindings/v8/ScriptValue.h:78: v8::Isolate* isolate() const { return
m_isolate; }
On 2013/09/18 00:25:12, haraken wrote:
> 
> Even if we cannot ensure that m_isolate is not 0 for some reason, we want to
> ensure that isolate() doesn't return 0. Hence we want to write:
> 
>   v8::Isolate* isolate() const { if(!m_isolate) m_isolate =
> Isolate::GetCurrent(); return m_isolate; }

Yes, this sounds like a good idea.

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
Source/bindings/v8/ScriptValue.h:167: m_isolate = 0;
On 2013/09/18 00:25:12, haraken wrote:
> 
> Do we need to clear an isolate? In other words, is it possible that we reuse
the
> same ScriptPromise in different isolates?

I honestly have no idea. I chose the "safe" approach and reset everything.

[haraken, 2013-09-18 07:02:15]

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
File Source/bindings/v8/ScriptValue.h (right):

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
Source/bindings/v8/ScriptValue.h:61: : m_isolate(0)
On 2013/09/18 06:35:15, Christophe Dumez wrote:
> On 2013/09/18 00:25:12, haraken wrote:
> > 
> > Is there any reason why we can't call Isolate::GetCurrent() here and ensure
> that
> > m_isolate is not 0 ?
> 
> My understanding was that we wanted to avoid calls to Isolate::GetCurrent() as
> much as possible. I expect there are many cases where we construct an empty
> StringValue and don't need an isolate at all.

Understood.

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
Source/bindings/v8/ScriptValue.h:167: m_isolate = 0;
On 2013/09/18 06:35:15, Christophe Dumez wrote:
> On 2013/09/18 00:25:12, haraken wrote:
> > 
> > Do we need to clear an isolate? In other words, is it possible that we reuse
> the
> > same ScriptPromise in different isolates?
> 
> I honestly have no idea. I chose the "safe" approach and reset everything.

I believe you can remove m_isolate=0. V8 heaps are completely separated among
isolates, so it's not possible to share one ScriptValue among isolates.

[do-not-use, 2013-09-18 07:18:46]

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
File Source/bindings/v8/ScriptValue.h (right):

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
Source/bindings/v8/ScriptValue.h:78: v8::Isolate* isolate() const { return
m_isolate; }
On 2013/09/18 00:25:12, haraken wrote:
> 
> Even if we cannot ensure that m_isolate is not 0 for some reason, we want to
> ensure that isolate() doesn't return 0. Hence we want to write:
> 
>   v8::Isolate* isolate() const { if(!m_isolate) m_isolate =
> Isolate::GetCurrent(); return m_isolate; }

Done.

https://codereview.chromium.org/23513066/diff/4001/Source/bindings/v8/ScriptV...
Source/bindings/v8/ScriptValue.h:167: m_isolate = 0;
On 2013/09/18 07:02:15, haraken wrote:
> On 2013/09/18 06:35:15, Christophe Dumez wrote:
> > On 2013/09/18 00:25:12, haraken wrote:
> > > 
> > > Do we need to clear an isolate? In other words, is it possible that we
reuse
> > the
> > > same ScriptPromise in different isolates?
> > 
> > I honestly have no idea. I chose the "safe" approach and reset everything.
> 
> I believe you can remove m_isolate=0. V8 heaps are completely separated among
> isolates, so it's not possible to share one ScriptValue among isolates.

Done.

[haraken, 2013-09-18 07:26:50]

Still LGTM

[commit-bot: I haz the power, 2013-09-18 07:28:55]

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

[commit-bot: I haz the power, 2013-09-18 09:52:12]

Change committed as 157950

[marja, 2013-09-18 12:39:04]

dcarney@, iirc you said at some point "we shouldn't add m_isolate to
ScriptValue"; this (committed) CL does it... (Do I remember right?)

[dcarney, 2013-09-18 19:12:20]

On 2013/09/18 12:39:04, marja wrote:
> dcarney@, iirc you said at some point "we shouldn't add m_isolate to
> ScriptValue"; this (committed) CL does it... (Do I remember right?)

Yeah, but that was before the Isolate::GetCurrent deprecation kick we're in now.
 At the moment, we don't have another way around this.