Remove unsafe access hacks from ScopedPersistent.

Remove unsafe access hacks from ScopedPersistent.

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

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

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

jyasskin, here's a CL removing the unsafe access hacks from ScopedPersistent -
ptal.

[Jeffrey Yasskin, 2013-09-09 17:45:07]

LGTM after the one change in the test.

https://codereview.chromium.org/23636015/diff/1/chrome/renderer/extensions/ac...
File chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc
(right):

https://codereview.chromium.org/23636015/diff/1/chrome/renderer/extensions/ac...
chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc:36:
context_.NewHandle(v8::Isolate::GetCurrent())));
Could you add a method "context()" or "context_handle()" to shorten these calls?

https://codereview.chromium.org/23636015/diff/1/chrome/renderer/extensions/sc...
File chrome/renderer/extensions/scoped_persistent.h (right):

https://codereview.chromium.org/23636015/diff/1/chrome/renderer/extensions/sc...
chrome/renderer/extensions/scoped_persistent.h:50: return
v8::Local<T>::New(GetIsolate(handle_), handle_);
I assume this method is going away soon too?

https://codereview.chromium.org/23636015/diff/1/chrome/renderer/extensions/sc...
chrome/renderer/extensions/scoped_persistent.h:53: v8::Handle<T>
NewHandle(v8::Isolate* isolate) const {
I've never quite understood when to use Local vs Handle. I assume you've gotten
this return type right.

[marja, 2013-09-09 18:16:18]

thanks for review!

https://codereview.chromium.org/23636015/diff/1/chrome/renderer/extensions/ac...
File chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc
(right):

https://codereview.chromium.org/23636015/diff/1/chrome/renderer/extensions/ac...
chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc:36:
context_.NewHandle(v8::Isolate::GetCurrent())));
On 2013/09/09 17:45:07, Jeffrey Yasskin wrote:
> Could you add a method "context()" or "context_handle()" to shorten these
calls?

Done (though, I noticed there is also isolate_ so this function is now shorter).

https://codereview.chromium.org/23636015/diff/1/chrome/renderer/extensions/sc...
File chrome/renderer/extensions/scoped_persistent.h (right):

https://codereview.chromium.org/23636015/diff/1/chrome/renderer/extensions/sc...
chrome/renderer/extensions/scoped_persistent.h:50: return
v8::Local<T>::New(GetIsolate(handle_), handle_);
On 2013/09/09 17:45:07, Jeffrey Yasskin wrote:
> I assume this method is going away soon too?

v8::Isolate::GetCurrent is going away; ideally we'd always give an isolate here
(and get rid of the GetIsolate funcs below - I'm not quite sure why they exist).

https://codereview.chromium.org/23636015/diff/1/chrome/renderer/extensions/sc...
chrome/renderer/extensions/scoped_persistent.h:53: v8::Handle<T>
NewHandle(v8::Isolate* isolate) const {
On 2013/09/09 17:45:07, Jeffrey Yasskin wrote:
> I've never quite understood when to use Local vs Handle. I assume you've
gotten
> this return type right.

They're basically the same thing - ATM Local is the only subclass of Handle. At
some point those classes will merge (but there's no decision yet if it'll be
called Local or Handle).

[commit-bot: I haz the power, 2013-09-09 18:16:37]

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

[commit-bot: I haz the power, 2013-09-09 21:48:59]

Change committed as 222102

[marja, 2013-09-10 06:50:54]

This was reverted, but it looks like it wasn't guilty after all, see
https://codereview.chromium.org/23724028/ . I'll try to re-land.

[commit-bot: I haz the power, 2013-09-10 06:51:43]

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

[commit-bot: I haz the power, 2013-09-10 08:48:40]

Change committed as 222225