Browser plugin: Implement titleChanged event.

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 acdea505531f3c687f84dc5f68770e90ef04c081..bd657533104695a48106df71336b43e62e644a16 100644
--- a/content/renderer/browser_plugin/browser_plugin.cc
+++ b/content/renderer/browser_plugin/browser_plugin.cc
@@ -53,6 +53,8 @@ const char kOldURL[] = "oldUrl";
 const char kPartitionAttribute[] = "partition";
 const char kPersistPrefix[] = "persist:";
 const char kSrcAttribute[] = "src";
+const char kTitle[] = "title";
+const char kTitleChangedEventName[] = "titleChanged";
 const char kType[] = "type";
 const char kURL[] = "url";
 }
@@ -452,6 +454,32 @@ void BrowserPlugin::LoadRedirect(const GURL& old_url,
   }
 }
 
+void BrowserPlugin::TitleChanged(const string16& title) {
+  if (!HasListeners(kTitleChangedEventName))
+    return;
+
+  EventListeners& listeners = event_listener_map_[kTitleChangedEventName];
+  EventListeners::iterator it = listeners.begin();
+
+  v8::Context::Scope context_scope(v8::Context::New());

[abarth-chromium, 2012/10/05 22:10:39]

It's very unlikely that you should create a new context.  That's a very
heavy-weight operation, and you're going to mess up the context stack.

+  v8::HandleScope handle_scope;
+
+  // Construct the titleChanged event object.
+  v8::Local<v8::Value> event =

[abarth-chromium, 2012/10/05 22:10:39]

Why are you using the type "v8::Local<v8::Value>" here?  That's just forcing you
to call Cast on the next line.  You probably shouldn't ever call Cast in this
code.  If you need to make a v8::Local<v8::Value> later (so you can take its
address), you can do that without violating the type system.

+      v8::Local<v8::Object>::New(v8::Object::New());
+  v8::Local<v8::Object>::Cast(event)->Set(
+      v8::String::New(kTitle, sizeof(kTitle) - 1),
+      v8::String::New(UTF16ToUTF8(title).c_str(), UTF16ToUTF8(title).size()));

[Charlie Reis, 2012/10/05 19:20:48]

Can you also give me a sanity check on this conversion?  My last version didn't
compile on Windows.

[Fady Samuel, 2012/10/05 19:53:30]

It looks like v8::String::New can work with UTF16. v8::String::New(const
uint16_t* data, int length)

[Charlie Reis, 2012/10/05 19:59:15]

Isn't uint16_t an unsigned int?  I wouldn't think that's for UTF16.

I was using v8::String::New(title.c_str(), title.size())); before, and I got
this error on Windows:
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/68126/...

error C2665: 'v8::String::New' : none of the 2 overloads could convert all the
argument types
        ..\v8/include/v8.h(1220): could be 'v8::Local<T> v8::String::New(const
char *,int)'
        with
        [
            T=v8::String
        ]
        ..\v8/include/v8.h(1223): or       'v8::Local<T> v8::String::New(const
uint16_t *,int)'
        with
        [
            T=v8::String
        ]
        while trying to match the argument list '(const wchar_t *, unsigned
int)'

I saw a similar pattern to this new one in
chrome/renderer/extensions/app_bindings.cc, though it didn't pass the size.

+  for (; it != listeners.end(); ++it) {
+    // Fire the event listener.
+    container()->element().document().frame()->
+        callFunctionEvenIfScriptDisabled(*it,

[abarth-chromium, 2012/10/05 22:10:39]

How do you know that calling these functions won't mutate listeners?

+                                         v8::Object::New(),
+                                         1,
+                                         &event);
+  }
+}
+
 void BrowserPlugin::AdvanceFocus(bool reverse) {
   // We do not have a RenderView when we are testing.
   if (render_view_)