Remove more calls to HandleScope default ctor.

Remove more calls to HandleScope default ctor.

It's replaced by a version which takes an isolate. The default ctor will be
removed soon.

BUG=NONE

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221689

[marja, 2013-09-05 12:01:21]

Could you review the following parts:
jyasskin: chrome/renderer/extensions/*
dmichael: chrome/renderer/pepper/*
sky: chrome/test/base/*
fsamuel: chrome/renderer/browser_plugin/*

[Fady Samuel, 2013-09-05 13:17:40]

browser_plugin LGTM

[dmichael (off chromium), 2013-09-05 15:22:02]

pepper lgtm

[Jeffrey Yasskin, 2013-09-05 17:36:25]

extensions lgtm, with one comment.

https://codereview.chromium.org/23679004/diff/1/chrome/renderer/extensions/ch...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://codereview.chromium.org/23679004/diff/1/chrome/renderer/extensions/ch...
chrome/renderer/extensions/chrome_v8_context.h:120: return isolate_;
I believe you can get this from v8_context()->GetIsolate(), so you wouldn't need
to store isolate_.

[sky, 2013-09-05 20:26:54]

LGTM

[marja, 2013-09-06 08:31:24]

Thanks for review! jyasskin, I'm committing this despite not doing your comment
(see below why), please let me know if you're not convinced :)

https://codereview.chromium.org/23679004/diff/1/chrome/renderer/extensions/ch...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://codereview.chromium.org/23679004/diff/1/chrome/renderer/extensions/ch...
chrome/renderer/extensions/chrome_v8_context.h:120: return isolate_;
On 2013/09/05 17:36:26, Jeffrey Yasskin wrote:
> I believe you can get this from v8_context()->GetIsolate(), so you wouldn't
need
> to store isolate_.

v8_context_ is a ScopedPersistent, and we shouldn't [be able to] call any V8
functions (such as GetIsolate) on it. Currently we can, but it's a hack and I
should remove it asap. So the only way to call a V8 function for a
ScopedPersistent is to create a new local handle based on it, and call the
function on the local handle. But the local handle needs a HandleScope, and the
HandleScope needs an Isolate, and that's a circle. I'm breaking the circle here
by storing the isolate.

[commit-bot: I haz the power, 2013-09-06 08:33:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/23679004/1

[commit-bot: I haz the power, 2013-09-06 12:21:05]

Change committed as 221689

[Jeffrey Yasskin, 2013-09-06 16:10:13]

https://codereview.chromium.org/23679004/diff/1/chrome/renderer/extensions/ch...
File chrome/renderer/extensions/chrome_v8_context.h (right):

https://codereview.chromium.org/23679004/diff/1/chrome/renderer/extensions/ch...
chrome/renderer/extensions/chrome_v8_context.h:120: return isolate_;
On 2013/09/06 08:31:24, marja wrote:
> On 2013/09/05 17:36:26, Jeffrey Yasskin wrote:
> > I believe you can get this from v8_context()->GetIsolate(), so you wouldn't
> need
> > to store isolate_.
> 
> v8_context_ is a ScopedPersistent, and we shouldn't [be able to] call any V8
> functions (such as GetIsolate) on it. Currently we can, but it's a hack and I
> should remove it asap. So the only way to call a V8 function for a
> ScopedPersistent is to create a new local handle based on it, and call the
> function on the local handle. But the local handle needs a HandleScope, and
the
> HandleScope needs an Isolate, and that's a circle. I'm breaking the circle
here
> by storing the isolate.

Oh, I see. Certainly a good answer for this CL. I wonder if there should be a
curated list of v8 functions you can call on Persistents, which would include
GetIsolate but not any type-specific functions. But that's a design question,
and not about the cleanup you're currently doing.

[marja, 2013-09-07 09:31:59]

Yup, one possible way would be to allow functions which cannot cause a GC to be
called on Persistents -- those are safe. For that, we'd need some kind of a
"promise" ("this won't cause GC") from the V8 side. We have been tossing the
idea around, but have no finalized plans on this yet...