Lazy load chrome.* APIs

chrome/renderer/extensions/module_system.cc

Index: chrome/renderer/extensions/module_system.cc
diff --git a/chrome/renderer/extensions/module_system.cc b/chrome/renderer/extensions/module_system.cc
index 26590475d0bd1ac967a6575f209eab5e1b4e7367..486c5eea94995a65ed96846ddc23a55e0124903d 100644
--- a/chrome/renderer/extensions/module_system.cc
+++ b/chrome/renderer/extensions/module_system.cc
@@ -4,10 +4,18 @@
 
 #include "chrome/renderer/extensions/module_system.h"
 
+#include <sstream>
+
 #include "base/bind.h"
 #include "base/stl_util.h"
+#include "chrome/common/extensions/extension_messages.h"
+#include "chrome/renderer/extensions/chrome_v8_context.h"
+#include "chrome/renderer/extensions/extension_helper.h"
+#include "content/public/renderer/render_view.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebScopedMicrotaskSuppression.h"
 
+using content::ConsoleMessageLevel;

[not at google - send to devlin, 2013/02/13 01:45:49]

not used

[cduvall, 2013/02/15 00:40:28]

Done.

+
 namespace {
 
 const char* kModuleSystem = "module_system";
@@ -19,11 +27,10 @@ const char* kModulesField = "modules";
 
 namespace extensions {
 
-ModuleSystem::ModuleSystem(v8::Handle<v8::Context> context,
+ModuleSystem::ModuleSystem(ChromeV8Context* context,
                            SourceMap* source_map)
-    : NativeHandler(context->GetIsolate()),
-      context_(v8::Persistent<v8::Context>::New(context->GetIsolate(),
-                                                context)),
+    : ObjectBackedNativeHandler(context->v8_context()->GetIsolate()),
+      context_(context),
       source_map_(source_map),
       natives_enabled_(0) {
   RouteFunction("require",
@@ -31,7 +38,7 @@ ModuleSystem::ModuleSystem(v8::Handle<v8::Context> context,
   RouteFunction("requireNative",
       base::Bind(&ModuleSystem::GetNative, base::Unretained(this)));
 
-  v8::Handle<v8::Object> global(context_->Global());
+  v8::Handle<v8::Object> global(context_->v8_context()->Global());
   global->SetHiddenValue(v8::String::New(kModulesField), v8::Object::New());
   global->SetHiddenValue(v8::String::New(kModuleSystem),
                          v8::External::New(this));
@@ -41,8 +48,8 @@ ModuleSystem::~ModuleSystem() {
   v8::HandleScope handle_scope;
   // Deleting this value here prevents future lazy field accesses from
   // referencing ModuleSystem after it has been freed.
-  context_->Global()->DeleteHiddenValue(v8::String::New(kModuleSystem));
-  context_.Dispose(context_->GetIsolate());
+  context_->v8_context()->Global()->DeleteHiddenValue(
+      v8::String::New(kModuleSystem));
 }
 
 ModuleSystem::NativesEnabledScope::NativesEnabledScope(
@@ -73,13 +80,10 @@ void ModuleSystem::HandleException(const v8::TryCatch& try_catch) {
 }
 
 // static
-void ModuleSystem::DumpException(const v8::TryCatch& try_catch) {
-  v8::HandleScope handle_scope;
-
+std::string ModuleSystem::CreateExceptionString(const v8::TryCatch& try_catch) {
   v8::Handle<v8::Message> message(try_catch.Message());
   if (message.IsEmpty()) {
-    LOG(ERROR) << "try_catch has no message";
-    return;
+    return "try_catch has no message";
   }
 
   std::string resource_name = "<unknown resource>";
@@ -92,6 +96,16 @@ void ModuleSystem::DumpException(const v8::TryCatch& try_catch) {
   if (!message->Get().IsEmpty())
     error_message = *v8::String::Utf8Value(message->Get());
 
+  std::stringstream ret;
+  ret << "[" << resource_name << "(" << message->GetLineNumber() << ")] "
+             << error_message;
+  return ret.str();
+}
+
+// static
+void ModuleSystem::DumpException(const v8::TryCatch& try_catch) {
+  v8::HandleScope handle_scope;
+
   std::string stack_trace = "<stack trace unavailable>";
   if (!try_catch.StackTrace().IsEmpty()) {
     v8::String::Utf8Value stack_value(try_catch.StackTrace());
@@ -101,14 +115,19 @@ void ModuleSystem::DumpException(const v8::TryCatch& try_catch) {
       stack_trace = "<could not convert stack trace to string>";
   }
 
-  LOG(ERROR) << "[" << resource_name << "(" << message->GetLineNumber() << ")] "
-             << error_message
-             << "{" << stack_trace << "}";
+  LOG(ERROR) << CreateExceptionString(try_catch) << "{" << stack_trace << "}";
 }
 
-void ModuleSystem::Require(const std::string& module_name) {
+void ModuleSystem::LogExceptionToConsole(const v8::TryCatch& try_catch) {
+  ExtensionHelper::Get(context_->GetRenderView())->AddMessageToRootConsole(
+          content::CONSOLE_MESSAGE_LEVEL_ERROR,
+          CreateExceptionString(try_catch));

[not at google - send to devlin, 2013/02/13 01:45:49]

Doesn't seem like this belongs in ModuleSystem (at least not as a public
method), just do this in BindingGeneratingNativeHandler directly?

Also can then go back to just injecting the v8::Context rather than the
ChromeV8Context.

[cduvall, 2013/02/15 00:40:28]

Done.

+}
+
+v8::Handle<v8::Value> ModuleSystem::Require(const std::string& module_name) {
   v8::HandleScope handle_scope;
-  RequireForJsInner(v8::String::New(module_name.c_str()));
+  return handle_scope.Close(
+      RequireForJsInner(v8::String::New(module_name.c_str())));
 }
 
 v8::Handle<v8::Value> ModuleSystem::RequireForJs(const v8::Arguments& args) {
@@ -148,11 +167,13 @@ v8::Handle<v8::Value> ModuleSystem::RequireForJsInner(
   };
   {
     WebKit::WebScopedMicrotaskSuppression suppression;
+    // TODO(cduvall): Figure out how to handle this and throw it for later.

[not at google - send to devlin, 2013/02/13 01:45:49]

I think rethrow is fine. We should always throw it though (i.e. don't cache the
undefined value) - but leaving it the way it is is fine.

[cduvall, 2013/02/15 00:40:28]

Reverted to how this used to be because logging is now handled in the JS.

     v8::TryCatch try_catch;
     try_catch.SetCaptureMessage(true);
     func->Call(global, 3, args);
     if (try_catch.HasCaught()) {
-      HandleException(try_catch);
+      // HandleException(try_catch);
+      try_catch.ReThrow();
       return v8::Undefined();
     }
   }
@@ -160,10 +181,11 @@ v8::Handle<v8::Value> ModuleSystem::RequireForJsInner(
   return handle_scope.Close(exports);
 }
 
-void ModuleSystem::CallModuleMethod(const std::string& module_name,
-                                    const std::string& method_name) {
+v8::Local<v8::Value> ModuleSystem::CallModuleMethod(
+    const std::string& module_name,
+    const std::string& method_name) {
   std::vector<v8::Handle<v8::Value> > args;
-  CallModuleMethod(module_name, method_name, &args);
+  return CallModuleMethod(module_name, method_name, &args);
 }
 
 v8::Local<v8::Value> ModuleSystem::CallModuleMethod(
@@ -197,6 +219,10 @@ v8::Local<v8::Value> ModuleSystem::CallModuleMethod(
   return handle_scope.Close(result);
 }
 
+bool ModuleSystem::HasNativeHandler(const std::string& name) {
+  return native_handler_map_.find(name) != native_handler_map_.end();
+}
+
 void ModuleSystem::RegisterNativeHandler(const std::string& name,
     scoped_ptr<NativeHandler> native_handler) {
   native_handler_map_[name] =
@@ -213,8 +239,24 @@ void ModuleSystem::RunString(const std::string& code, const std::string& name) {
 }
 
 // static
+v8::Handle<v8::Value> ModuleSystem::NativeLazyFieldGetter(
+    v8::Local<v8::String> property, const v8::AccessorInfo& info) {
+  return LazyFieldGetterInner(property,
+                              info,
+                              &ModuleSystem::GetNativeFromString);
+}
+
+// static
 v8::Handle<v8::Value> ModuleSystem::LazyFieldGetter(
     v8::Local<v8::String> property, const v8::AccessorInfo& info) {
+  return LazyFieldGetterInner(property, info, &ModuleSystem::Require);
+}
+
+// static
+v8::Handle<v8::Value> ModuleSystem::LazyFieldGetterInner(
+    v8::Local<v8::String> property,
+    const v8::AccessorInfo& info,
+    GetModuleFunc get_module) {
   CHECK(!info.Data().IsEmpty());
   CHECK(info.Data()->IsObject());
   v8::HandleScope handle_scope;
@@ -229,25 +271,46 @@ v8::Handle<v8::Value> ModuleSystem::LazyFieldGetter(
   ModuleSystem* module_system = static_cast<ModuleSystem*>(
       v8::Handle<v8::External>::Cast(module_system_value)->Value());
 
-  v8::Handle<v8::Object> module;
-  {
-    NativesEnabledScope scope(module_system);
-    module = v8::Handle<v8::Object>::Cast(module_system->RequireForJsInner(
-        parameters->Get(v8::String::New(kModuleName))->ToString()));
+  std::string name = *v8::String::AsciiValue(
+      parameters->Get(v8::String::New(kModuleName))->ToString());
+
+  NativesEnabledScope scope(module_system);
+  v8::TryCatch try_catch;
+  v8::Handle<v8::Object> module = v8::Handle<v8::Object>::Cast(
+      (module_system->*get_module)(name));
+  if (try_catch.HasCaught()) {
+    module_system->LogExceptionToConsole(try_catch);
+    return handle_scope.Close(v8::Handle<v8::Value>());

[not at google - send to devlin, 2013/02/13 01:45:49]

rethrow should be fine

[cduvall, 2013/02/15 00:40:28]

Changed it to HandleException like the rest of the file.

   }
+
   if (module.IsEmpty())
     return handle_scope.Close(v8::Handle<v8::Value>());
 
   v8::Handle<v8::String> field =
       parameters->Get(v8::String::New(kModuleField))->ToString();
 
-  return handle_scope.Close(module->Get(field));
+  v8::Local<v8::Value> new_field = module->Get(field);
+  v8::Handle<v8::Object> object = info.This();
+  if (!new_field->IsUndefined()) {
+    object->Delete(property);
+    object->Set(property, new_field);

[not at google - send to devlin, 2013/02/13 01:45:49]

This shouldn't be necessary since Require/RequireNative does this already?
Right?

[cduvall, 2013/02/15 00:40:28]

Done.

+  }
+  return handle_scope.Close(new_field);
 }
 
 void ModuleSystem::SetLazyField(v8::Handle<v8::Object> object,
                                 const std::string& field,
                                 const std::string& module_name,
                                 const std::string& module_field) {
+  SetLazyField(object, field, module_name, module_field,
+      &ModuleSystem::LazyFieldGetter);
+}
+
+void ModuleSystem::SetLazyField(v8::Handle<v8::Object> object,
+                                const std::string& field,
+                                const std::string& module_name,
+                                const std::string& module_field,
+                                v8::AccessorGetter getter) {
   v8::HandleScope handle_scope;
   v8::Handle<v8::Object> parameters = v8::Object::New();
   parameters->Set(v8::String::New(kModuleName),
@@ -256,11 +319,19 @@ void ModuleSystem::SetLazyField(v8::Handle<v8::Object> object,
                   v8::String::New(module_field.c_str()));
 
   object->SetAccessor(v8::String::New(field.c_str()),

[not at google - send to devlin, 2013/02/13 01:45:49]

As mentioned in IRC, if you use SetNamedPropertyAccessor then you can make the
existence checks also call Require() then devtools should hopefully work fine.
Also, it's just good behaviour - extensions should be able to check access to an
API via ('foo' in chrome) or chrome.hasOwnProperty('foo').

[cduvall, 2013/02/15 00:40:28]

As you said in your email, looks like SetNamedPropertyAccessor isn't the
solution. Still looking for something that will work with ('foo' in chrome) and
chrome.hasOwnProperty('foo')

-                      &ModuleSystem::LazyFieldGetter,
+                      getter,
                       NULL,
                       parameters);
 }
 
+void ModuleSystem::SetNativeLazyField(v8::Handle<v8::Object> object,
+                                      const std::string& field,
+                                      const std::string& module_name,
+                                      const std::string& module_field) {
+  SetLazyField(object, field, module_name, module_field,
+      &ModuleSystem::NativeLazyFieldGetter);
+}
+
 v8::Handle<v8::Value> ModuleSystem::RunString(v8::Handle<v8::String> code,
                                               v8::Handle<v8::String> name) {
   v8::HandleScope handle_scope;
@@ -292,11 +363,16 @@ v8::Handle<v8::Value> ModuleSystem::GetSource(
 
 v8::Handle<v8::Value> ModuleSystem::GetNative(const v8::Arguments& args) {
   CHECK_EQ(1, args.Length());
+  std::string native_name = *v8::String::AsciiValue(args[0]->ToString());
+  return GetNativeFromString(native_name);
+}
+
+v8::Handle<v8::Value> ModuleSystem::GetNativeFromString(

[not at google - send to devlin, 2013/02/13 01:45:49]

Yeah this is confusing, can you rename it to
RequireNative/RequireNativeFromString. Sorry...

[cduvall, 2013/02/15 00:40:28]

Done.

+    const std::string& native_name) {
   if (natives_enabled_ == 0)
     return ThrowException("Natives disabled");
-  std::string native_name = *v8::String::AsciiValue(args[0]->ToString());
   if (overridden_native_handlers_.count(native_name) > 0u)
-    return RequireForJs(args);
+    return RequireForJsInner(v8::String::New(native_name.c_str()));
   NativeHandlerMap::iterator i = native_handler_map_.find(native_name);
   if (i == native_handler_map_.end())
     return v8::Undefined();