Browser Plugin: Implement Back, Forward, and Go.

Browser Plugin: Implement Back, Forward, and Go.


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

[Fady Samuel, 2012-09-20 18:39:11]

Hi Istiaque,

Could you please take a first pass at this patch before I ask Charlie for an
OWNERS review?

Thanks,

Fady

[lazyboy, 2012-09-20 20:05:30]

On 2012/09/20 18:39:11, Fady Samuel wrote:
> Hi Istiaque,
> 
> Could you please take a first pass at this patch before I ask Charlie for an
> OWNERS review?
> 
> Thanks,
> 
> Fady

[lazyboy, 2012-09-20 20:05:42]

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_embedder.cc:219: if (!guest)
Here and below:
if (guest)
  guest->Back();

pattern is probably better?

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/browser/bro...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:531:
test_guest->WaitForUpdateRectMsg();
See below: navigate back and forward here too, and verify that's no-op.

Also test cases for Go(0), Go(+ve_index)

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:579: string16
actual_title = title_watcher.WaitAndGetTitle();
same as below, try to go one step further forward.

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:589:
rvh->ExecuteJavascriptAndGetValue(string16(), ASCIIToUTF16("Go(-2);"));
A test to see if we try to go back further and stay at P1

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/common/brow...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/common/brow...
content/common/browser_plugin_messages.h:38:
IPC_MESSAGE_ROUTED2(BrowserPluginHostMsg_Go,
Add comment for this one too.

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:155:
BrowserPluginManager::Get()->Send(
Can we reduce this and next messages and use BPHostMsg_Go with +1/-1?

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/test/data/b...
File content/test/data/browser_plugin_embedder.html (right):

https://chromiumcodereview.appspot.com/10960003/diff/1001/content/test/data/b...
content/test/data/browser_plugin_embedder.html:12: plugin =
document.getElementById('plugin');
here and below:
var plugin = document.getElementById(....

[Fady Samuel, 2012-09-21 15:21:56]

Fixed nits. Added Charlie to review:

Charlie, is there a difference between Back and Go(-1) and Forward and Go(1)? If
not, we can probably make Back, Go, and Forward all use one IPC, right? Thanks,

Fady

https://codereview.chromium.org/10960003/diff/1001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/10960003/diff/1001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:219: if (!guest)
On 2012/09/20 20:05:42, lazyboy wrote:
> Here and below:
> if (guest)
>   guest->Back();
> 
> pattern is probably better?

Done.

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

https://codereview.chromium.org/10960003/diff/1001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:531:
test_guest->WaitForUpdateRectMsg();
On 2012/09/20 20:05:42, lazyboy wrote:
> See below: navigate back and forward here too, and verify that's no-op.
> 
> Also test cases for Go(0), Go(+ve_index)

While I'm not against more test coverage, I feel that these additional tests do
not increase test coverage of the code I just wrote. They do cover more
navigation code in general but this is not new code.

https://codereview.chromium.org/10960003/diff/1001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:579: string16
actual_title = title_watcher.WaitAndGetTitle();
On 2012/09/20 20:05:42, lazyboy wrote:
> same as below, try to go one step further forward.

ditto, I'm not convinced this actually improves test coverage.

https://codereview.chromium.org/10960003/diff/1001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:589:
rvh->ExecuteJavascriptAndGetValue(string16(), ASCIIToUTF16("Go(-2);"));
On 2012/09/20 20:05:42, lazyboy wrote:
> A test to see if we try to go back further and stay at P1

ditto.

https://codereview.chromium.org/10960003/diff/1001/content/common/browser_plu...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/10960003/diff/1001/content/common/browser_plu...
content/common/browser_plugin_messages.h:38:
IPC_MESSAGE_ROUTED2(BrowserPluginHostMsg_Go,
On 2012/09/20 20:05:42, lazyboy wrote:
> Add comment for this one too.

Done.

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

https://codereview.chromium.org/10960003/diff/1001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:155:
BrowserPluginManager::Get()->Send(
On 2012/09/20 20:05:42, lazyboy wrote:
> Can we reduce this and next messages and use BPHostMsg_Go with +1/-1?

We can, but I don't know if these are identical operations. I'll this question
for Charlie to answer.

https://codereview.chromium.org/10960003/diff/1001/content/test/data/browser_...
File content/test/data/browser_plugin_embedder.html (right):

https://codereview.chromium.org/10960003/diff/1001/content/test/data/browser_...
content/test/data/browser_plugin_embedder.html:12: plugin =
document.getElementById('plugin');
On 2012/09/20 20:05:42, lazyboy wrote:
> here and below:
> var plugin = document.getElementById(....

Done.

[Charlie Reis, 2012-09-21 18:38:35]

On 2012/09/21 15:21:56, Fady Samuel wrote:
> Fixed nits. Added Charlie to review:
> 
> Charlie, is there a difference between Back and Go(-1) and Forward and Go(1)?
If
> not, we can probably make Back, Go, and Forward all use one IPC, right?
Thanks,
> 
> Fady

Yes, those should be equivalent operations.

Is there something wrong with the upload?  I can't review Patch Set 4.  (It's
showing "error: old chunk mismatch" on the code review site.)

[Fady Samuel, 2012-09-26 16:14:27]

I eliminated BrowserPluginHostMsg_Back and BrowserPluginHostMsg_Forward.

All three operations now use BrowserPluginHostMsg_Go.

[Charlie Reis, 2012-09-28 17:41:20]

Sorry for the delay-- this looks pretty good.

https://codereview.chromium.org/10960003/diff/14001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/10960003/diff/14001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.h:89: friend class
BrowserPluginEmbedderHelper;
Looks like this CL needs to be rebased, since this friend class is no longer
there.

https://codereview.chromium.org/10960003/diff/14001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.h:126: void Go(int
instance_id, int relative_index);
This should be public and grouped with NavigateGuest, Stop, and Reload.

https://codereview.chromium.org/10960003/diff/14001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/10960003/diff/14001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.h:149: // navigation entry.
Maybe add "if possible," since this will do nothing if you give it an invalid
index.

https://codereview.chromium.org/10960003/diff/14001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/10960003/diff/14001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:159: -1));
For reference, it's ok to put some parameters on the same line when calling
functions (if it helps readability).  You just have to ensure they're each on
their own line for function declarations (assuming they don't all fit on the
same line).

No strong feelings here, but I'd probably put the -1 on the previous line.

https://codereview.chromium.org/10960003/diff/14001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/10960003/diff/14001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:96: return
NPVARIANT_TO_DOUBLE(variant);
Why DOUBLE?

[Fady Samuel, 2012-09-28 18:08:08]

Fixed nits. PTAL.

https://codereview.chromium.org/10960003/diff/14001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/10960003/diff/14001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.h:89: friend class
BrowserPluginEmbedderHelper;
On 2012/09/28 17:41:20, creis wrote:
> Looks like this CL needs to be rebased, since this friend class is no longer
> there.

Done.

https://codereview.chromium.org/10960003/diff/14001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.h:126: void Go(int
instance_id, int relative_index);
On 2012/09/28 17:41:20, creis wrote:
> This should be public and grouped with NavigateGuest, Stop, and Reload.

Done.

https://codereview.chromium.org/10960003/diff/14001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/10960003/diff/14001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.h:149: // navigation entry.
On 2012/09/28 17:41:20, creis wrote:
> Maybe add "if possible," since this will do nothing if you give it an invalid
> index.

Done.

https://codereview.chromium.org/10960003/diff/14001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/10960003/diff/14001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:159: -1));
On 2012/09/28 17:41:20, creis wrote:
> For reference, it's ok to put some parameters on the same line when calling
> functions (if it helps readability).  You just have to ensure they're each on
> their own line for function declarations (assuming they don't all fit on the
> same line).
> 
> No strong feelings here, but I'd probably put the -1 on the previous line.

Done.

https://codereview.chromium.org/10960003/diff/14001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/10960003/diff/14001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:96: return
NPVARIANT_TO_DOUBLE(variant);
On 2012/09/28 17:41:20, creis wrote:
> Why DOUBLE?

Javascript may interpret the literal as a double, in some cases. Even if
Javascript interprets it as a double, we still want to grab the value.

[Charlie Reis, 2012-09-28 18:40:46]

LGTM.

https://codereview.chromium.org/10960003/diff/14001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/10960003/diff/14001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:96: return
NPVARIANT_TO_DOUBLE(variant);
On 2012/09/28 18:08:09, Fady Samuel wrote:
> On 2012/09/28 17:41:20, creis wrote:
> > Why DOUBLE?
> 
> Javascript may interpret the literal as a double, in some cases. Even if
> Javascript interprets it as a double, we still want to grab the value.

Yes, but then we'll potentially lose data by returning it as an int.

I guess that would happen anyway, though, if we had to cast it to call
GoToOffset.  Plus we could add a separate DoubleFromeNPVariant if anyone needed
the extra precision.  Ok.

[commit-bot: I haz the power, 2012-09-28 18:41:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10960003/21001

[commit-bot: I haz the power, 2012-09-28 18:41:38]

Failed to apply patch for
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:
While running patch -p1 --forward --force;
patching file content/browser/browser_plugin/browser_plugin_host_browsertest.cc
Hunk #1 succeeded at 31 (offset 1 line).
Hunk #2 FAILED at 40.
Hunk #3 succeeded at 685 with fuzz 1 (offset 116 lines).
1 out of 3 hunks FAILED -- saving rejects to file
content/browser/browser_plugin/browser_plugin_host_browsertest.cc.rej

[commit-bot: I haz the power, 2012-09-28 19:00:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10960003/27001

[commit-bot: I haz the power, 2012-09-28 22:08:27]

Change committed as 159348