Browser Plugin: Reload and Stop operations

Browser Plugin: Reload and Stop operations

Fairly simple change. Just a whole lot of plumbing to call the following:

void BrowserPluginGuest::Stop() {
  web_contents()->Stop();
}

void BrowserPluginGuest::Reload() {
  web_contents()->GetController().Reload(false);
}

This includes the parsing the methods in browser_plugin_bindings.cc, the IPCs in browser_plugin.cc, the plumbing to the embedder and from the embedder to the appropriate guest.

This also includes tests to verify that these two operations do indeed get plumbed correctly.

BUG=148981
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157988

[Fady Samuel, 2012-09-12 21:38:07]

I plan to add another test, but I just implemented this so I thought I'd share.

[Fady Samuel, 2012-09-13 17:52:21]

I can't think of a good way to test stop() on the browser process side.

[lazyboy, 2012-09-13 19:14:15]

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

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:488:
ASSERT_TRUE(test_guest->WaitForUpdateRectMsg());
Add a comment saying that a successful reload would send new update rect
message.

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/browser/bro...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:489: 
nit: rm empty line at the end.

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

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:153: if (src_.empty())
if (!navigate_src_sent_) instead after my recent patch update, since we do want
to call Stop() even if we have tried to set empty source after a non-empty
navigation.

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:161: if (src_.empty())
Same here, if (!navigation_src_sent_).

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_bindings.cc:119: if
(IdentifierIsAddEventListener(name))
We probably want to put all the valid names into some sets of some sort at a
point since this is growing.

For now, can we change this to
return IdentifierIsAddEventListner(name) ||
    IdentifierIsRemoveEventListener(name) ...;

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_bindings.cc:173: if
(IdentifierIsStop(name) && arg_count == 0) {
nit here and below: !arg_count
Also why would we care if there was an arg sent? Is there a benefit of ignoring
Stop/Reload in that case?

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:349: {
Out of curiosity, what does this inner scopes do?

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:379: // Verify
that we've navigated back to the same page.
Also ASSERT_FALSE(browser_plugin_.guest_crashed_)

[Fady Samuel, 2012-09-19 14:38:09]

Hi Istiaque,

Could you please take another look? I'm going to try to add a Stop
browser_plugin_host_browsertest as you review the stuff that's currently in
place, thanks,

Fady

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

https://codereview.chromium.org/10917225/diff/3001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:488:
ASSERT_TRUE(test_guest->WaitForUpdateRectMsg());
On 2012/09/13 19:14:15, lazyboy wrote:
> Add a comment saying that a successful reload would send new update rect
> message.

Done.

https://codereview.chromium.org/10917225/diff/3001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:489: 
On 2012/09/13 19:14:15, lazyboy wrote:
> nit: rm empty line at the end.

Done.

https://codereview.chromium.org/10917225/diff/3001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:489: 
On 2012/09/13 19:14:15, lazyboy wrote:
> nit: rm empty line at the end.

Done.

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

https://codereview.chromium.org/10917225/diff/3001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:153: if (src_.empty())
On 2012/09/13 19:14:15, lazyboy wrote:
> if (!navigate_src_sent_) instead after my recent patch update, since we do
want
> to call Stop() even if we have tried to set empty source after a non-empty
> navigation.

Done.

https://codereview.chromium.org/10917225/diff/3001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:161: if (src_.empty())
On 2012/09/13 19:14:15, lazyboy wrote:
> Same here, if (!navigation_src_sent_).

Done.

https://codereview.chromium.org/10917225/diff/3001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/10917225/diff/3001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_bindings.cc:173: if
(IdentifierIsStop(name) && arg_count == 0) {
On 2012/09/13 19:14:15, lazyboy wrote:
> nit here and below: !arg_count
> Also why would we care if there was an arg sent? Is there a benefit of
ignoring
> Stop/Reload in that case?

Done.

In response to your question: Do we really want to allow malformed methods to
work?

https://codereview.chromium.org/10917225/diff/3001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right):

https://codereview.chromium.org/10917225/diff/3001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:349: {
On 2012/09/13 19:14:15, lazyboy wrote:
> Out of curiosity, what does this inner scopes do?

It limits the scope of the msg, frame_id, src, size local variables?

We verify that we've navigated a first time, we clear the MessageSink, we crash
the browser plugin, and then we reload, and confirm that we have a new
NavigateGuest request.

https://codereview.chromium.org/10917225/diff/3001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:379: // Verify
that we've navigated back to the same page.
On 2012/09/13 19:14:15, lazyboy wrote:
> Also ASSERT_FALSE(browser_plugin_.guest_crashed_)

Done.

[lazyboy, 2012-09-19 15:06:05]

Looks good to me, ping me when you have the host_browsertest, I can take another
look.
Thanks.

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_bindings.cc:173: if
(IdentifierIsStop(name) && arg_count == 0) {
On 2012/09/19 14:38:09, Fady Samuel wrote:
> On 2012/09/13 19:14:15, lazyboy wrote:
> > nit here and below: !arg_count
> > Also why would we care if there was an arg sent? Is there a benefit of
> ignoring
> > Stop/Reload in that case?
> 
> Done.
> 
> In response to your question: Do we really want to allow malformed methods to
> work?

I don't know what the regular take on this is. I have seen js function calls to
be forgiving to developers in terms of parameters.

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10917225/diff/3001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin_browsertest.cc:349: {
On 2012/09/19 14:38:09, Fady Samuel wrote:
> On 2012/09/13 19:14:15, lazyboy wrote:
> > Out of curiosity, what does this inner scopes do?
> 
> It limits the scope of the msg, frame_id, src, size local variables?
> 
> We verify that we've navigated a first time, we clear the MessageSink, we
crash
> the browser plugin, and then we reload, and confirm that we have a new
> NavigateGuest request.

OK thanks. (Making sure they are just local scopes).

https://chromiumcodereview.appspot.com/10917225/diff/10001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/10917225/diff/10001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:233: fprintf(stderr,
">>>%s\n", __PRETTY_FUNCTION__);
Remove.

https://chromiumcodereview.appspot.com/10917225/diff/10001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/10917225/diff/10001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:179:
bindings->instance()->Reload();
return true; here too?

[Fady Samuel, 2012-09-19 15:50:56]

https://codereview.chromium.org/10917225/diff/10001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/10917225/diff/10001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:233: fprintf(stderr,
">>>%s\n", __PRETTY_FUNCTION__);
On 2012/09/19 15:06:06, lazyboy wrote:
> Remove.

Done.

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

https://codereview.chromium.org/10917225/diff/10001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:179:
bindings->instance()->Reload();
On 2012/09/19 15:06:06, lazyboy wrote:
> return true; here too?

Done.

[Fady Samuel, 2012-09-19 15:51:25]

Hi Charlie,

Could you please review this completed reload() + stop() method implementation?

Thanks,

Fady

[Charlie Reis, 2012-09-19 21:53:50]

I'll get to the rest, but wanted to send this first.

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

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.h:87: friend class
BrowserPluginEmbedderHelper;
I missed this before.  We generally don't like friend classes, since they make
it harder to tell where private members are used (and thus what invariants
hold).  Can we make the necessary calls public?

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.h:125: void Reload(int
instance_id);
These seem like something that should be public, alongside NavigateGuest
(regardless of the friend comment I made).

[Charlie Reis, 2012-09-19 22:17:09]

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:233:
web_contents()->GetController().Reload(false);
The NavigationController documentation says most callers should pass in true for
|check_for_repost|.  Wouldn't we want that to happen here, or are you trying to
avoid Chrome's repost warning page from showing?  (I suppose we could offer a
separate API for registering a callback if a repost is about to happen?)

Either way, please add a comment saying which decision is made here and why.

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:505:
test_embedder->WaitForGuestAdded();
Does this work if the guest has already been added?  (Wouldn't want it to wait
forever if there's a race.)

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:523:
test_guest->WaitForUpdateRectMsg();
Couldn't other things cause this message (like an error message)?  Can we listen
for a successful navigation, or handle it with a WaitForReload() similar to
WaitForStop()?

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

https://codereview.chromium.org/10917225/diff/23002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:163: // Renavigate to the same
URL if the guest has crashed.
Is this necessary?  I think NavigationController::Reload should be able to
handle the case that the renderer has crashed.

https://codereview.chromium.org/10917225/diff/23002/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/10917225/diff/23002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:61: // A request from
javascript has been made to stop the loading of the page.
nit: Javascript (consistent with above)

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

https://codereview.chromium.org/10917225/diff/23002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:43: const char
kReloadMethod[] = "reload";
nit: Alphabetize

[Fady Samuel, 2012-09-19 23:29:20]

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

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.h:125: void Reload(int
instance_id);
On 2012/09/19 21:53:50, creis wrote:
> These seem like something that should be public, alongside NavigateGuest
> (regardless of the friend comment I made).

Done.

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:233:
web_contents()->GetController().Reload(false);
On 2012/09/19 22:17:09, creis wrote:
> The NavigationController documentation says most callers should pass in true
for
> |check_for_repost|.  Wouldn't we want that to happen here, or are you trying
to
> avoid Chrome's repost warning page from showing?  (I suppose we could offer a
> separate API for registering a callback if a repost is about to happen?)
> 
> Either way, please add a comment saying which decision is made here and why.

Done.

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:505:
test_embedder->WaitForGuestAdded();
On 2012/09/19 22:17:09, creis wrote:
> Does this work if the guest has already been added?  (Wouldn't want it to wait
> forever if there's a race.)

Yes, this returns immediately if a guest has already been added.

https://codereview.chromium.org/10917225/diff/23002/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:523:
test_guest->WaitForUpdateRectMsg();
On 2012/09/19 22:17:09, creis wrote:
> Couldn't other things cause this message (like an error message)?  Can we
listen
> for a successful navigation, or handle it with a WaitForReload() similar to
> WaitForStop()?

Done. Implemented WaitForReload()

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

https://codereview.chromium.org/10917225/diff/23002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:163: // Renavigate to the same
URL if the guest has crashed.
On 2012/09/19 22:17:09, creis wrote:
> Is this necessary?  I think NavigationController::Reload should be able to
> handle the case that the renderer has crashed.

Done. The guest_crashed_ flag needs to be reset. but otherwise, we can simply do
Reload. Removed the ReloadAfterCrash test in content/renderer/browser_plugin.

https://codereview.chromium.org/10917225/diff/23002/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/10917225/diff/23002/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:61: // A request from
javascript has been made to stop the loading of the page.
On 2012/09/19 22:17:09, creis wrote:
> nit: Javascript (consistent with above)

Done.

[Charlie Reis, 2012-09-20 00:07:19]

LGTM with one missed nit from before.

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

https://codereview.chromium.org/10917225/diff/25016/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:43: const char
kReloadMethod[] = "reload";
nit: Reload before Remove.  :)

[commit-bot: I haz the power, 2012-09-20 17:31:48]

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

[commit-bot: I haz the power, 2012-09-21 03:22:30]

Retried try job too often for step(s) browser_tests