Browser Plugin: Fix Events

content/renderer/browser_plugin/browser_plugin.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/browser_plugin/browser_plugin.h"
 
 #include "base/message_loop.h"
 #include "base/string_util.h"
 #if defined (OS_WIN)
 #include "base/sys_info.h"
@@ skipping 306 matching lines @@
       gfx::Size()));
 }
 
 void BrowserPlugin::GuestCrashed() {
   guest_crashed_ = true;
   container_->invalidate();
 
   if (!HasListeners(kCrashEventName))
     return;
 
-  EventListeners& listeners = event_listener_map_[kCrashEventName];
+  WebKit::WebFrame* frame = container()->element().document().frame();
+  v8::HandleScope handle_scope;
+  v8::Context::Scope context_scope(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);
+    frame->callFunctionEvenIfScriptDisabled(*it,

[abarth-chromium, 2012/10/09 18:38:18]

How do you know that this call doesn't destroy |frame| ?

[Fady Samuel, 2012/10/09 20:25:11]

Done.

+                                            v8::Object::New(),
+                                            0,
+                                            NULL);
   }
 }
 
 void BrowserPlugin::DidNavigate(const GURL& url, int process_id) {
   src_ = url.spec();
   process_id_ = process_id;
   if (!HasListeners(kNavigationEventName))
     return;
 
-  EventListeners& listeners = event_listener_map_[kNavigationEventName];
+  WebKit::WebFrame* frame = container()->element().document().frame();
+  v8::HandleScope handle_scope;
+  v8::Context::Scope context_scope(frame->mainWorldScriptContext());
+
+  v8::Local<v8::Value> param =
+      v8::Local<v8::Value>::New(v8::String::New(src_.c_str()));

[abarth-chromium, 2012/10/09 18:38:18]

It's more efficient to the two-argument form of v8::String::New.

Also, there's no reason to create a new local handle.  v8::String::New already
returns a local handle.

[Fady Samuel, 2012/10/09 20:25:11]

Done.

+
+  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);
+    frame->callFunctionEvenIfScriptDisabled(*it,
+                                            v8::Object::New(),
+                                            1,
+                                            &param);
   }
 }
 
 void BrowserPlugin::LoadStart(const GURL& url, bool is_top_level) {
   if (!HasListeners(kLoadStartEventName))
     return;
 
-  EventListeners& listeners = event_listener_map_[kLoadStartEventName];
+  WebKit::WebFrame* frame = container()->element().document().frame();
+  v8::HandleScope handle_scope;
+  v8::Context::Scope context_scope(frame->mainWorldScriptContext());
+
+  // Construct the loadStart event object.
+  v8::Local<v8::Object> event =
+      v8::Local<v8::Object>::New(v8::Object::New());

[abarth-chromium, 2012/10/09 18:38:18]

v8::Object::New already returns a local handle.  There's no reason to create
another one.

[Fady Samuel, 2012/10/09 20:25:11]

Done.

+  event->Set(v8::String::New(kURL, sizeof(kURL) - 1),
+             v8::String::New(url.spec().c_str(), url.spec().size()));

[abarth-chromium, 2012/10/09 18:38:18]

There's no reason to call c_str here because you don't need the string to be
null terminated.  Instead, you can call data(), which might avoid an allocation.

[Fady Samuel, 2012/10/09 20:25:11]

Done.

+  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();
-
-  v8::Context::Scope context_scope(v8::Context::New());
-  v8::HandleScope handle_scope;
-  // 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));
   for (; it != listeners.end(); ++it) {
     // Fire the event listener.
-    container()->element().document().frame()->
-        callFunctionEvenIfScriptDisabled(*it,
-                                         v8::Object::New(),
-                                         1,
-                                         &event);
+    frame->callFunctionEvenIfScriptDisabled(*it,
+                                            v8::Object::New(),
+                                            1,
+                                            &val);
   }
 }
 
 void BrowserPlugin::LoadAbort(const GURL& url,
                               bool is_top_level,
                               const std::string& type) {
   if (!HasListeners(kLoadAbortEventName))
     return;
 
-  EventListeners& listeners = event_listener_map_[kLoadAbortEventName];
+  WebKit::WebFrame* frame = container()->element().document().frame();
+  v8::HandleScope handle_scope;
+  v8::Context::Scope context_scope(frame->mainWorldScriptContext());
+
+  // Construct the loadAbort event object.
+  v8::Local<v8::Object> event =
+      v8::Local<v8::Object>::New(v8::Object::New());
+  event->Set(v8::String::New(kURL, sizeof(kURL) - 1),
+             v8::String::New(url.spec().c_str(), 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.c_str(), type.size()));
+  v8::Local<v8::Value> val = event;
+
+  EventListeners listeners(event_listener_map_[kLoadAbortEventName]);
   EventListeners::iterator it = listeners.begin();
-
-  v8::Context::Scope context_scope(v8::Context::New());
-  v8::HandleScope handle_scope;
-  // 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()));
   for (; it != listeners.end(); ++it) {
     // Fire the event listener.
-    container()->element().document().frame()->
-        callFunctionEvenIfScriptDisabled(*it,
-                                         v8::Object::New(),
-                                         1,
-                                         &event);
+    frame->callFunctionEvenIfScriptDisabled(*it,
+                                            v8::Object::New(),
+                                            1,
+                                            &val);
   }
 }
 
 void BrowserPlugin::LoadRedirect(const GURL& old_url,
                                  const GURL& new_url,
                                  bool is_top_level) {
   if (!HasListeners(kLoadRedirectEventName))
     return;
 
-  EventListeners& listeners = event_listener_map_[kLoadRedirectEventName];
-  EventListeners::iterator it = listeners.begin();
-
-  v8::Context::Scope context_scope(v8::Context::New());
+  WebKit::WebFrame* frame = container()->element().document().frame();
   v8::HandleScope handle_scope;
+  v8::Context::Scope context_scope(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());
-  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()));
+  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);
+    frame->callFunctionEvenIfScriptDisabled(*it,
+                                            v8::Object::New(),
+                                            1,
+                                            &val);
   }
 }
 
 void BrowserPlugin::AdvanceFocus(bool reverse) {
   // We do not have a RenderView when we are testing.
   if (render_view_)
     render_view_->GetWebView()->advanceFocus(reverse);
 }
 
 void BrowserPlugin::SetAcceptTouchEvents(bool accept) {
@@ skipping 277 matching lines @@
                                                  void* notify_data) {
 }
 
 void BrowserPlugin::didFailLoadingFrameRequest(
     const WebKit::WebURL& url,
     void* notify_data,
     const WebKit::WebURLError& error) {
 }
 
 }  // namespace content