Browser plugin: Implement loadRedirect event.

Browser plugin: Implement loadRedirect event.

BUG=153536
TEST=browserTag.addEventListener('loadRedirect', f);


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160400

[Charlie Reis, 2012-10-04 00:46:41]

Fady, can you take a look?  I've verified that this works in practice, but I
can't get the test to work.  It's hanging when I try to navigate the guest a
second time.  Any ideas?

I originally wanted to just pass the redirect URL into the
StartBrowserPluginTest function, but there doesn't seem to be a way to wait for
that navigation to finish.  (Should we always wait for that guest navigation to
commit?  There might be some flakiness if we don't.)

https://codereview.chromium.org/11028019/diff/2001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11028019/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:629: string16
actual_title = title_watcher.WaitAndGetTitle();
The test is hanging here, as if the navigation never takes place (and thus no
title change happens).  I've tried without the "client-redirect?" part and it
still hangs, so that's not to blame.

[Fady Samuel, 2012-10-04 03:07:06]

http://codereview.chromium.org/11028019/diff/2001/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11028019/diff/2001/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:629: string16
actual_title = title_watcher.WaitAndGetTitle();
On 2012/10/04 00:46:42, creis wrote:
> The test is hanging here, as if the navigation never takes place (and thus no
> title change happens).  I've tried without the "client-redirect?" part and it
> still hangs, so that's not to blame.

The issue is you can't pass this partial URL. You need to give it a full URL. 

GURL test_url(test_server()->GetURL(redirectURL));
    rvh->ExecuteJavascriptAndGetValue(
        string16(),
        ASCIIToUTF16(StringPrintf("RedirectTo('%s');",
test_url.spec().c_str())));

That will get you past here. The tests below fail though. I'm not sure why.
BrowserPluginGuest::LoadRedirect never seems to get called.

[Fady Samuel, 2012-10-04 15:15:30]

http://codereview.chromium.org/11028019/diff/2001/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/11028019/diff/2001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:52: const std::string kNewURL
= "newUrl";
Please make these const char[] to match my loadStart/loadAbort patch.

http://codereview.chromium.org/11028019/diff/2001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:365:
v8::String::New(kOldURL.c_str(), kOldURL.size()),
Please use char[] instead of string to match my patch. Also you need to use
sizeof(OldURL) - 1. The -1 is important because normally reports the total size
of the string including the null termination character which we don't count
here.

[Charlie Reis, 2012-10-04 19:05:58]

Thanks-- new patch uploaded, and the test is working now.  Can you take another
look?

(I'll plan to merge with your loadStart CL to resolve conflicts once it lands.)

https://codereview.chromium.org/11028019/diff/2001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11028019/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:629: string16
actual_title = title_watcher.WaitAndGetTitle();
On 2012/10/04 03:07:06, Fady Samuel wrote:
> On 2012/10/04 00:46:42, creis wrote:
> > The test is hanging here, as if the navigation never takes place (and thus
no
> > title change happens).  I've tried without the "client-redirect?" part and
it
> > still hangs, so that's not to blame.
> 
> The issue is you can't pass this partial URL. You need to give it a full URL. 
> 
> GURL test_url(test_server()->GetURL(redirectURL));
>     rvh->ExecuteJavascriptAndGetValue(
>         string16(),
>         ASCIIToUTF16(StringPrintf("RedirectTo('%s');",
> test_url.spec().c_str())));
> 
> That will get you past here. The tests below fail though. I'm not sure why.
> BrowserPluginGuest::LoadRedirect never seems to get called.

Thanks.  The rest of the test wasn't working because it wasn't waiting for the
redirect event handler to be called, just for the guest's title to change.  I
changed the test to update the embedder's title in the redirect event handler,
closer to yours.

We also apparently don't get this event on client redirects, which have separate
commits.  I changed it to a server-redirect.

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

https://codereview.chromium.org/11028019/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:365:
v8::String::New(kOldURL.c_str(), kOldURL.size()),
On 2012/10/04 15:15:30, Fady Samuel wrote:
> Please use char[] instead of string to match my patch. Also you need to use
> sizeof(OldURL) - 1. The -1 is important because normally reports the total
size
> of the string including the null termination character which we don't count
> here.

Done.

[Fady Samuel, 2012-10-04 19:48:20]

LGTM! Thanks!

[commit-bot: I haz the power, 2012-10-04 22:37:56]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[sadrul, 2012-10-04 22:48:05]

stamp LGTM

[commit-bot: I haz the power, 2012-10-04 22:52:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/11028019/15013

[commit-bot: I haz the power, 2012-10-05 01:04:59]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-05 15:25:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/11028019/15013

[commit-bot: I haz the power, 2012-10-05 17:01:58]

Change committed as 160400