Browser Plugin: Fix Events

content/renderer/browser_plugin/browser_plugin.cc

Index: content/renderer/browser_plugin/browser_plugin.cc
diff --git a/content/renderer/browser_plugin/browser_plugin.cc b/content/renderer/browser_plugin/browser_plugin.cc
index ed0f293678b2d13a4c5398ce6640fec90470115a..9d7fe83ab65ad87073177b102d3294d354741a39 100644
--- a/content/renderer/browser_plugin/browser_plugin.cc
+++ b/content/renderer/browser_plugin/browser_plugin.cc
@@ -324,16 +324,18 @@ void BrowserPlugin::GuestCrashed() {
   if (!HasListeners(kCrashEventName))
     return;
 
-  EventListeners& listeners = event_listener_map_[kCrashEventName];
+  WebKit::WebElement plugin = container()->element();
+  v8::HandleScope handle_scope;
+  v8::Context::Scope context_scope(
+      plugin.document().frame()->mainWorldScriptContext());
+
+  EventListeners listeners(event_listener_map_[kCrashEventName]);
   EventListeners::iterator it = listeners.begin();
   for (; it != listeners.end(); ++it) {
-    v8::Context::Scope context_scope(v8::Context::New());
-    v8::HandleScope handle_scope;
-    container()->element().document().frame()->
-        callFunctionEvenIfScriptDisabled(*it,
-                                         v8::Object::New(),
-                                         0,
-                                         NULL);
+    if (plugin.document().frame()) {

[abarth-chromium, 2012/10/09 20:30:00]

I would have put this in a local variable, but I'm a paranoid panda.  :)

WebFrame* frame = plugin.document().frame();
if (frame)
  frame->callFunctionEvenIfScriptDisabled(...)

[Fady Samuel, 2012/10/09 21:35:21]

Done.

+      plugin.document().frame()->callFunctionEvenIfScriptDisabled(
+          *it, v8::Object::New(), 0, NULL);
+    }
   }
 }
 
@@ -343,18 +345,20 @@ void BrowserPlugin::DidNavigate(const GURL& url, int process_id) {
   if (!HasListeners(kNavigationEventName))
     return;
 
-  EventListeners& listeners = event_listener_map_[kNavigationEventName];
+  WebKit::WebElement plugin = container()->element();
+  v8::HandleScope handle_scope;
+  v8::Context::Scope context_scope(
+      plugin.document().frame()->mainWorldScriptContext());
+
+  v8::Local<v8::Value> param = v8::String::New(src_.data(), src_.size());
+
+  EventListeners listeners(event_listener_map_[kNavigationEventName]);
   EventListeners::iterator it = listeners.begin();
   for (; it != listeners.end(); ++it) {
-    v8::Context::Scope context_scope(v8::Context::New());
-    v8::HandleScope handle_scope;
-    v8::Local<v8::Value> param =
-        v8::Local<v8::Value>::New(v8::String::New(src_.c_str()));
-    container()->element().document().frame()->
-        callFunctionEvenIfScriptDisabled(*it,
-                                         v8::Object::New(),
-                                         1,
-                                         &param);
+    if (plugin.document().frame()) {
+      plugin.document().frame()->callFunctionEvenIfScriptDisabled(
+          *it, v8::Object::New(), 1, &param);
+    }
   }
 }
 
@@ -362,27 +366,27 @@ void BrowserPlugin::LoadStart(const GURL& url, bool is_top_level) {
   if (!HasListeners(kLoadStartEventName))
     return;
 
-  EventListeners& listeners = event_listener_map_[kLoadStartEventName];
-  EventListeners::iterator it = listeners.begin();
-
-  v8::Context::Scope context_scope(v8::Context::New());
+  WebKit::WebElement plugin = container()->element();
   v8::HandleScope handle_scope;
+  v8::Context::Scope context_scope(
+      plugin.document().frame()->mainWorldScriptContext());
+
   // Construct the loadStart event object.
-  v8::Local<v8::Value> event =
-      v8::Local<v8::Object>::New(v8::Object::New());
-  v8::Local<v8::Object>::Cast(event)->Set(
-      v8::String::New(kURL, sizeof(kURL) - 1),
-      v8::String::New(url.spec().c_str(), url.spec().size()));
-  v8::Local<v8::Object>::Cast(event)->Set(
-      v8::String::New(kIsTopLevel, sizeof(kIsTopLevel) - 1),
-      v8::Boolean::New(is_top_level));
+  v8::Local<v8::Object> event = v8::Object::New();
+  event->Set(v8::String::New(kURL, sizeof(kURL) - 1),
+             v8::String::New(url.spec().data(), url.spec().size()));
+  event->Set(v8::String::New(kIsTopLevel, sizeof(kIsTopLevel) - 1),
+             v8::Boolean::New(is_top_level));
+  v8::Local<v8::Value> val = event;
+
+  EventListeners listeners(event_listener_map_[kLoadStartEventName]);
+  EventListeners::iterator it = listeners.begin();
   for (; it != listeners.end(); ++it) {
-    // Fire the event listener.
-    container()->element().document().frame()->
-        callFunctionEvenIfScriptDisabled(*it,
-                                         v8::Object::New(),
-                                         1,
-                                         &event);
+    if (plugin.document().frame()) {
+      // Fire the event listener.
+      plugin.document().frame()->callFunctionEvenIfScriptDisabled(
+          *it, v8::Object::New(), 1, &val);
+    }
   }
 }
 
@@ -392,30 +396,29 @@ void BrowserPlugin::LoadAbort(const GURL& url,
   if (!HasListeners(kLoadAbortEventName))
     return;
 
-  EventListeners& listeners = event_listener_map_[kLoadAbortEventName];
-  EventListeners::iterator it = listeners.begin();
-
-  v8::Context::Scope context_scope(v8::Context::New());
+  WebKit::WebElement plugin = container()->element();
   v8::HandleScope handle_scope;
+  v8::Context::Scope context_scope(
+      plugin.document().frame()->mainWorldScriptContext());
+
   // Construct the loadAbort event object.
-  v8::Local<v8::Value> event =
-      v8::Local<v8::Object>::New(v8::Object::New());
-  v8::Local<v8::Object>::Cast(event)->Set(
-      v8::String::New(kURL, sizeof(kURL) - 1),
-      v8::String::New(url.spec().c_str(), url.spec().size()));
-  v8::Local<v8::Object>::Cast(event)->Set(
-      v8::String::New(kIsTopLevel, sizeof(kIsTopLevel) - 1),
-      v8::Boolean::New(is_top_level));
-  v8::Local<v8::Object>::Cast(event)->Set(
-      v8::String::New(kType, sizeof(kType) - 1),
-      v8::String::New(type.c_str(), type.size()));
+  v8::Local<v8::Object> event = v8::Object::New();
+  event->Set(v8::String::New(kURL, sizeof(kURL) - 1),
+             v8::String::New(url.spec().data(), url.spec().size()));
+  event->Set(v8::String::New(kIsTopLevel, sizeof(kIsTopLevel) - 1),
+             v8::Boolean::New(is_top_level));
+  event->Set(v8::String::New(kType, sizeof(kType) - 1),
+             v8::String::New(type.data(), type.size()));
+  v8::Local<v8::Value> val = event;
+
+  EventListeners listeners(event_listener_map_[kLoadAbortEventName]);
+  EventListeners::iterator it = listeners.begin();
   for (; it != listeners.end(); ++it) {
-    // Fire the event listener.
-    container()->element().document().frame()->
-        callFunctionEvenIfScriptDisabled(*it,
-                                         v8::Object::New(),
-                                         1,
-                                         &event);
+    if (plugin.document().frame()) {
+      // Fire the event listener.
+      plugin.document().frame()->callFunctionEvenIfScriptDisabled(
+          *it, v8::Object::New(), 1, &val);
+    }
   }
 }
 
@@ -425,31 +428,30 @@ void BrowserPlugin::LoadRedirect(const GURL& old_url,
   if (!HasListeners(kLoadRedirectEventName))
     return;
 
-  EventListeners& listeners = event_listener_map_[kLoadRedirectEventName];
-  EventListeners::iterator it = listeners.begin();
-
-  v8::Context::Scope context_scope(v8::Context::New());
+  WebKit::WebElement plugin = container()->element();
   v8::HandleScope handle_scope;
+  v8::Context::Scope context_scope(
+      plugin.document().frame()->mainWorldScriptContext());
 
   // Construct the loadRedirect event object.
-  v8::Local<v8::Value> event =
+  v8::Local<v8::Object> event =
       v8::Local<v8::Object>::New(v8::Object::New());

[abarth-chromium, 2012/10/09 20:30:00]

No need to create a new local handle here either.  :)

[Fady Samuel, 2012/10/09 21:35:21]

Done.

-  v8::Local<v8::Object>::Cast(event)->Set(
-      v8::String::New(kOldURL, sizeof(kOldURL) - 1),
-      v8::String::New(old_url.spec().c_str(), old_url.spec().size()));
-  v8::Local<v8::Object>::Cast(event)->Set(
-      v8::String::New(kNewURL, sizeof(kNewURL) - 1),
-      v8::String::New(new_url.spec().c_str(), new_url.spec().size()));
-  v8::Local<v8::Object>::Cast(event)->Set(
-      v8::String::New(kIsTopLevel, sizeof(kIsTopLevel) - 1),
-      v8::Boolean::New(is_top_level));
+  event->Set(v8::String::New(kOldURL, sizeof(kOldURL) - 1),
+             v8::String::New(old_url.spec().c_str(), old_url.spec().size()));

[abarth-chromium, 2012/10/09 20:30:00]

c_str -> data

[Fady Samuel, 2012/10/09 21:35:21]

Done.

+  event->Set(v8::String::New(kNewURL, sizeof(kNewURL) - 1),
+             v8::String::New(new_url.spec().c_str(), new_url.spec().size()));
+  event->Set(v8::String::New(kIsTopLevel, sizeof(kIsTopLevel) - 1),
+             v8::Boolean::New(is_top_level));
+  v8::Local<v8::Value> val = event;
+
+  EventListeners listeners(event_listener_map_[kLoadRedirectEventName]);
+  EventListeners::iterator it = listeners.begin();
   for (; it != listeners.end(); ++it) {
-    // Fire the event listener.
-    container()->element().document().frame()->
-        callFunctionEvenIfScriptDisabled(*it,
-                                         v8::Object::New(),
-                                         1,
-                                         &event);
+    if (plugin.document().frame()) {
+      // Fire the event listener.

[abarth-chromium, 2012/10/09 20:30:00]

I would remove this comment since it doesn't really add anything to what the
code already said, but that might be WebKit style leaking into my review.

[Fady Samuel, 2012/10/09 21:35:21]

Done.

+      plugin.document().frame()->callFunctionEvenIfScriptDisabled(
+          *it, v8::Object::New(), 1, &val);
+    }
   }
 }