Extensions: pass ChromeV8Context around instead of v8::Handle.

Extensions: pass ChromeV8Context around instead of v8::Handle.

There was a TODO about this in object_backed_native_handler.h.

The passed handle was a Persistent behind the scenes, and we're going to disable
copying Persistents. We'd need to pass a Local handle instead, but this way the
TODO gets done with a similar amount of effort.

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

[marja, 2013-05-29 12:28:34]

(kalman, ptal)

I'm in your code, doing your TODOs. :)

We need this because we're going to make Persistent handles uncopyable and this
code was passing them around.

-----

Note about naming:
v8_context was sometimes used to name the Handle<Context> so I changed the param
name to context. This is not fully unambigous, since context was also used some
places for Handle<Context>.

-----

Also fixed a missing inlude,
#include "chrome/renderer/extensions/native_handler.h"
in chrome/renderer/extensions/module_system.h; it was only working by accident.

-----

ObjectBackedNativeHandler::v8_context needs to be moved to the .cc file, because
having it in the header would need #include
"chrome/renderer/extensions/chrome_v8_context.h" and that creates a circular
#include struture which leads to this failure:
class ModuleSystem : public ObjectBackedNativeHandler { <<
ObjectBackedNativeHandler not yet defined.

[not at google - send to devlin, 2013-05-29 15:36:26]

Quick thing before I review: I was under the impression that copying a
Persistent as a Handle is not the same as copying a Persistent as a Persistent.
I.e. that

Persistent foo() {
  return v8::Persistent::New(...);
}

is bad (for the reasons that you have mentioned), but that

Handle foo() {
  return v8::Persistent::New(...);
}

is ok, at least from an efficiency perspective, because Handle's copy
constructor is defined as

template <class S> Handle(Handle<S> that)
    : val_(reinterpret_cast<T*>(*that)) { ... }

and operator* of even a Persistent just returns the underlying pointer val_. So
the function above which returns the plain Handle is only as expensive as a
pointer-pointer copy, basically free.

That said, we often hang onto these as v8::Persistents anyway (or rather
ScopedPersistents) which involves creating a new Persistent which you are also
trying to avoid? And that is partly where my TODO originated.

Anyway, efficiency aside, this is indeed a cleanup I was wanting to do because
ChromeV8Context is just a more convenient object to pass around. The second
reason for the TODO.

[not at google - send to devlin, 2013-05-29 15:55:13]

thanks so much for doing this, there are a whole bunch of cleanups that this was
blocking.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/api_activity_logger.h:20:
APIActivityLogger(Dispatcher* dispatcher, ChromeV8Context* v8_context);
Yes - I would have much preferred to be calling these |context| not
|v8_context|. The v8_context name was forced on me with a multiple inheritence
thing going on somewhere that made it impossible to call it v8_context. But I
think your patch will make that moot.

Anyway, I'll just point these out. v8_context -> context here.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/app_bindings.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/app_bindings.cc:76: const Extension* extension =
ChromeV8ExtensionHandler::context_->extension();
see comment in ObjectBackedNativeHandler: could you call context() here (and
everywhere else) rather than needing to qualify
ChromeV8ExtensionHandler::context_?

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/app_window_custom_bindings.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/app_window_custom_bindings.h:17: ChromeV8Context*
context);
btw many of these can probably be moved onto a single line if you like.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/content_watcher.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/content_watcher.cc:52: ChromeV8Context* v8_context) {
v8_context -> context

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/content_watcher.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/content_watcher.h:39: scoped_ptr<NativeHandler>
MakeNatives(ChromeV8Context* v8_context);
v8_context -> context

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/object_backed_native_handler.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/object_backed_native_handler.cc:94: context_ = NULL;
can this be moved back above the call to NativeHandler::Invalidate()?

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/object_backed_native_handler.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/object_backed_native_handler.h:42:
v8::Handle<v8::Context> v8_context();
can this be made to return a ChromeV8Context* (and call it context()) rather
than the v8_context()? then the context_ below can be private.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/object_backed_native_handler.h:47: ChromeV8Context*
context_;
then this can be private

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/page_actions_custom_bindings.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/page_actions_custom_bindings.cc:19: Dispatcher*
dispatcher, ChromeV8Context* v8_context)
v8_context -> context

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/page_actions_custom_bindings.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/page_actions_custom_bindings.h:17: ChromeV8Context*
v8_context);
v8_context -> context

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/sync_file_system_custom_bindings.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/sync_file_system_custom_bindings.h:17:
ChromeV8Context* v8_context);
v8_context -> context

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/tts_custom_bindings.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/tts_custom_bindings.h:15:
TTSCustomBindings(Dispatcher* dispatcher, ChromeV8Context* v8_context);
v8_context -> context

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/web_request_custom_bindings.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/web_request_custom_bindings.cc:17: Dispatcher*
dispatcher, ChromeV8Context* v8_context)
v8_context -> context

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/web_request_custom_bindings.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/web_request_custom_bindings.h:15:
WebRequestCustomBindings(Dispatcher* dispatcher, ChromeV8Context* v8_context);
v8_context -> context

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/webstore_bindings.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/webstore_bindings.cc:216:
ChromeV8ExtensionHandler::context_->v8_context());
likewise can this call context() instead of accessing context_.

https://codereview.chromium.org/16032015/diff/2001/chrome/test/base/module_sy...
File chrome/test/base/module_system_test.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/test/base/module_sy...
chrome/test/base/module_system_test.cc:96:
extensions::Feature::BLESSED_EXTENSION_CONTEXT)),
let's make it an undefined context type, I want to maintain that there is always
an Extension present in a blessed context.

https://codereview.chromium.org/16032015/diff/2001/chrome/test/base/module_sy...
File chrome/test/base/module_system_test.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/test/base/module_sy...
chrome/test/base/module_system_test.h:58:
scoped_ptr<extensions::ChromeV8Context> chrome_v8_context_;
It would be nicer if this were just scoped_ptr<e::ChromeV8Context> context_
rather than having both a context_ and a chrome_v8_context_ field.

[marja, 2013-05-29 15:57:42]

We'll also make it so that Persistent doesn't subclass Handle, so copying
Persistent as Handle is no longer possible.

Copying Persistents as Handles is equally bad, because you still don't know if
the underlying object will go away (e.g., if the original Persistent is made
weak or disposed). The object can even be garbage collected while you're calling
a function on it.

Copying Persistent as Persistent also effectively only copies the pointer,
there's no extra magic. So the only thing which is "better" when copying it as
Handle is that the receiver side cannot mess up the original Persistent by
calling Persistent::Dispose or Persistent::MakeWeak. The other way around is
still possible.

[marja, 2013-05-29 16:56:33]

thanks for having a look!

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/api_activity_logger.h:20:
APIActivityLogger(Dispatcher* dispatcher, ChromeV8Context* v8_context);
On 2013/05/29 15:55:13, kalman wrote:
> Yes - I would have much preferred to be calling these |context| not
> |v8_context|. The v8_context name was forced on me with a multiple inheritence
> thing going on somewhere that made it impossible to call it v8_context. But I
> think your patch will make that moot.
> 
> Anyway, I'll just point these out. v8_context -> context here.

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/app_bindings.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/app_bindings.cc:76: const Extension* extension =
ChromeV8ExtensionHandler::context_->extension();
On 2013/05/29 15:55:13, kalman wrote:
> see comment in ObjectBackedNativeHandler: could you call context() here (and
> everywhere else) rather than needing to qualify
> ChromeV8ExtensionHandler::context_?

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/app_window_custom_bindings.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/app_window_custom_bindings.h:17: ChromeV8Context*
context);
On 2013/05/29 15:55:13, kalman wrote:
> btw many of these can probably be moved onto a single line if you like.

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/content_watcher.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/content_watcher.cc:52: ChromeV8Context* v8_context) {
On 2013/05/29 15:55:13, kalman wrote:
> v8_context -> context

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/content_watcher.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/content_watcher.h:39: scoped_ptr<NativeHandler>
MakeNatives(ChromeV8Context* v8_context);
On 2013/05/29 15:55:13, kalman wrote:
> v8_context -> context

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/object_backed_native_handler.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/object_backed_native_handler.cc:94: context_ = NULL;
On 2013/05/29 15:55:13, kalman wrote:
> can this be moved back above the call to NativeHandler::Invalidate()?

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/object_backed_native_handler.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/object_backed_native_handler.h:42:
v8::Handle<v8::Context> v8_context();
On 2013/05/29 15:55:13, kalman wrote:
> can this be made to return a ChromeV8Context* (and call it context()) rather
> than the v8_context()? then the context_ below can be private.

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/object_backed_native_handler.h:47: ChromeV8Context*
context_;
On 2013/05/29 15:55:13, kalman wrote:
> then this can be private

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/page_actions_custom_bindings.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/page_actions_custom_bindings.cc:19: Dispatcher*
dispatcher, ChromeV8Context* v8_context)
On 2013/05/29 15:55:13, kalman wrote:
> v8_context -> context

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/page_actions_custom_bindings.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/page_actions_custom_bindings.h:17: ChromeV8Context*
v8_context);
On 2013/05/29 15:55:13, kalman wrote:
> v8_context -> context

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/sync_file_system_custom_bindings.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/sync_file_system_custom_bindings.h:17:
ChromeV8Context* v8_context);
On 2013/05/29 15:55:13, kalman wrote:
> v8_context -> context

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/tts_custom_bindings.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/tts_custom_bindings.h:15:
TTSCustomBindings(Dispatcher* dispatcher, ChromeV8Context* v8_context);
On 2013/05/29 15:55:13, kalman wrote:
> v8_context -> context

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/web_request_custom_bindings.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/web_request_custom_bindings.cc:17: Dispatcher*
dispatcher, ChromeV8Context* v8_context)
On 2013/05/29 15:55:13, kalman wrote:
> v8_context -> context

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/web_request_custom_bindings.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/web_request_custom_bindings.h:15:
WebRequestCustomBindings(Dispatcher* dispatcher, ChromeV8Context* v8_context);
On 2013/05/29 15:55:13, kalman wrote:
> v8_context -> context

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/webstore_bindings.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/webstore_bindings.cc:216:
ChromeV8ExtensionHandler::context_->v8_context());
On 2013/05/29 15:55:13, kalman wrote:
> likewise can this call context() instead of accessing context_.

Done.

https://codereview.chromium.org/16032015/diff/2001/chrome/test/base/module_sy...
File chrome/test/base/module_system_test.cc (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/test/base/module_sy...
chrome/test/base/module_system_test.cc:96:
extensions::Feature::BLESSED_EXTENSION_CONTEXT)),
On 2013/05/29 15:55:13, kalman wrote:
> let's make it an undefined context type, I want to maintain that there is
always
> an Extension present in a blessed context.

https://codereview.chromium.org/16032015/diff/2001/chrome/test/base/module_sy...
File chrome/test/base/module_system_test.h (right):

https://codereview.chromium.org/16032015/diff/2001/chrome/test/base/module_sy...
chrome/test/base/module_system_test.h:58:
scoped_ptr<extensions::ChromeV8Context> chrome_v8_context_;
On 2013/05/29 15:55:13, kalman wrote:
> It would be nicer if this were just scoped_ptr<e::ChromeV8Context> context_
> rather than having both a context_ and a chrome_v8_context_ field.

Done.

[not at google - send to devlin, 2013-05-29 17:41:55]

lgtm

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_extension.cc (right):

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_extension.cc:39: return
dispatcher_->v8_context_set().GetByV8Context(context()->v8_context());
this method is actually entirely unnecessary now. subclasses of
ChromeV8Extension can just call context().

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_extension.cc:43: ChromeV8Context* context =
GetContext();
also might as well inline this.

I will soon be able to delete this class entirely! yay.

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
File chrome/renderer/extensions/content_watcher.cc (right):

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:36:
WebKit::WebFrame::frameForContext(context()->v8_context()));
I am curious: how will this work in a world where this will need to return a
local handle? Will things need a handle scope everywhere?

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:264: ChromeV8Context* context =
GetContext();
could inline this as context() too, etc below.

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
File chrome/renderer/extensions/event_bindings.cc (right):

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/event_bindings.cc:100: ChromeV8Context* context =
GetContext();
ditto etc

https://codereview.chromium.org/16032015/diff/13001/chrome/test/base/module_s...
File chrome/test/base/module_system_test.cc (right):

https://codereview.chromium.org/16032015/diff/13001/chrome/test/base/module_s...
chrome/test/base/module_system_test.cc:110: context_->v8_context()->Exit();
idly wonder why this doesn't hold onto a context scope as a member. whatever,
nothing to do with this patch.

[marja, 2013-05-31 10:06:44]

Thanks for review!

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
File chrome/renderer/extensions/chrome_v8_extension.cc (right):

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_extension.cc:39: return
dispatcher_->v8_context_set().GetByV8Context(context()->v8_context());
On 2013/05/29 17:41:55, kalman wrote:
> this method is actually entirely unnecessary now. subclasses of
> ChromeV8Extension can just call context().

Done.

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/chrome_v8_extension.cc:43: ChromeV8Context* context =
GetContext();
On 2013/05/29 17:41:55, kalman wrote:
> also might as well inline this.
> 
> I will soon be able to delete this class entirely! yay.

Done.

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
File chrome/renderer/extensions/content_watcher.cc (right):

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:36:
WebKit::WebFrame::frameForContext(context()->v8_context()));
On 2013/05/29 17:41:55, kalman wrote:
> I am curious: how will this work in a world where this will need to return a
> local handle? Will things need a handle scope everywhere?

Yeah, we need a HandleScope whenever we're dealing with the v8::Context.

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:264: ChromeV8Context* context =
GetContext();
On 2013/05/29 17:41:55, kalman wrote:
> could inline this as context() too, etc below.

Done.

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
File chrome/renderer/extensions/event_bindings.cc (right):

https://codereview.chromium.org/16032015/diff/13001/chrome/renderer/extension...
chrome/renderer/extensions/event_bindings.cc:100: ChromeV8Context* context =
GetContext();
On 2013/05/29 17:41:55, kalman wrote:
> ditto etc

Done.

[commit-bot: I haz the power, 2013-05-31 10:07:02]

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

[commit-bot: I haz the power, 2013-05-31 10:18:54]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[marja, 2013-05-31 11:00:33]

sky, could you OWNERS-rubberstamp chrome/test/base/*? kalman@ did the actual
review.

[marja, 2013-06-03 07:37:19]

oops, forgot to add sky@ as a reviewer :/

sky, could you OWNERS-rubberstamp chrome/test/base/*? kalman@ did the actual
review.

[sky, 2013-06-03 15:58:48]

LGTM

[commit-bot: I haz the power, 2013-06-03 16:01:13]

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

[commit-bot: I haz the power, 2013-06-03 16:21:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-04 07:31:50]

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

[commit-bot: I haz the power, 2013-06-04 16:39:14]

Change committed as 203978