Browser plugin: Implement titleChanged event.

Browser plugin: Implement titleChanged event.

BUG=154166
TEST=browserTag.addEventListener('titleChanged', f);

[Charlie Reis, 2012-10-05 17:22:14]

Fady, can you take a look?

[Fady Samuel, 2012-10-05 17:45:52]

LGTM + 1 nit.

https://codereview.chromium.org/11027065/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11027065/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:467: // Construct the
loadRedirect event object.
Please update this comment :-)

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

https://codereview.chromium.org/11027065/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11027065/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.cc:467: // Construct the
loadRedirect event object.
On 2012/10/05 17:45:52, Fady Samuel wrote:
> Please update this comment :-)

Good copy/paste catch.  :)  Done.

https://codereview.chromium.org/11027065/diff/6002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11027065/diff/6002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:472:
v8::String::New(UTF16ToUTF8(title).c_str(), UTF16ToUTF8(title).size()));
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:29]

https://codereview.chromium.org/11027065/diff/6002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11027065/diff/6002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:472:
v8::String::New(UTF16ToUTF8(title).c_str(), UTF16ToUTF8(title).size()));
On 2012/10/05 19:20:48, creis wrote:
> Can you also give me a sanity check on this conversion?  My last version
didn't
> compile on Windows.

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]

https://codereview.chromium.org/11027065/diff/6002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11027065/diff/6002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:472:
v8::String::New(UTF16ToUTF8(title).c_str(), UTF16ToUTF8(title).size()));
On 2012/10/05 19:53:30, Fady Samuel wrote:
> On 2012/10/05 19:20:48, creis wrote:
> > Can you also give me a sanity check on this conversion?  My last version
> didn't
> > compile on Windows.
> 
> It looks like v8::String::New can work with UTF16. v8::String::New(const
> uint16_t* data, int length)

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.

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

https://codereview.chromium.org/11027065/diff/6002/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11027065/diff/6002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:464: v8::Context::Scope
context_scope(v8::Context::New());
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.

https://codereview.chromium.org/11027065/diff/6002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:468: v8::Local<v8::Value>
event =
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.

https://codereview.chromium.org/11027065/diff/6002/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:476:
callFunctionEvenIfScriptDisabled(*it,
How do you know that calling these functions won't mutate listeners?

[Fady Samuel, 2012-10-10 17:37:30]

https://codereview.chromium.org/11086025/ has landed. Please see this as an
example of how to fire off an event. More changes coming in the near future once
we figure out how we want to do events long-term.