Browser Plugin: Update Guest WebContents Visibility on BrowserPlugin Visiblity Change.

Browser Plugin: Update Guest WebContents Visibility on BrowserPlugin Visiblity Change.


BUG=140310
Test=BrowserPluginHostTest.{EmbedderVisiblityChanged, BrowserPluginVisibilityChanged}

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

[Charlie Reis, 2012-10-05 00:33:17]

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

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:272:
guest->SetVisibility(visible(), guest_visible);
Just use visible_.

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:110: bool visible()
const { return visible_; }
Looks like we don't need to expose this.

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

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:107:
web_contents()->WasHidden();
Just to be clear, does this notify the guest renderer process so that it
realizes it is hidden/shown?  Maybe you can mention that in the comment in
browser_plugin_guest.h.

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:137: void
SetVisibility(bool embedder_visible, bool visible);
nit: Add a comment.

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

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:420:
IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest, EmbedderVisibilityChanged) {
I think these names are backwards, right?

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

https://codereview.chromium.org/11066032/diff/2001/content/common/browser_plu...
content/common/browser_plugin_messages.h:120: // Tells the guest to show or hide
itself
nit: End with a period.

This comment is also a bit ambiguous.  Maybe "Tells the guest it has been shown
or hidden," to make it more clear that the embedder renderer has already taken
the action?

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

https://codereview.chromium.org/11066032/diff/2001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:652: if (visible_ == visible)
Just confirming: we don't need to care if the embedder's WebContents was hidden
or shown here, right?

I thought there might have been a bug if you had a weird sequence of setting
visibility from both above and below, but that doesn't seem to be the case.  It
looks like it's safe for this visible_ flag to only track whether the
BrowserPlugin thinks its visible or shown, since this IPC message will only
affect the guest's WebContents if both |embedder_visible| and |visible| are
true.

(Maybe it's worth mentioning in browser_plugin.h that visible_ tracks only
whether the browser plugin is visible, regardless of the whole embedder
RenderView's visibility.)

[Fady Samuel, 2012-10-05 15:22:06]

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

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:272:
guest->SetVisibility(visible(), guest_visible);
On 2012/10/05 00:33:18, creis wrote:
> Just use visible_.

Done.

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.h:110: bool visible()
const { return visible_; }
On 2012/10/05 00:33:18, creis wrote:
> Looks like we don't need to expose this.

Done.

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

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:107:
web_contents()->WasHidden();
On 2012/10/05 00:33:18, creis wrote:
> Just to be clear, does this notify the guest renderer process so that it
> realizes it is hidden/shown?  Maybe you can mention that in the comment in
> browser_plugin_guest.h.

Done.

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:137: void
SetVisibility(bool embedder_visible, bool visible);
On 2012/10/05 00:33:18, creis wrote:
> nit: Add a comment.

Done.

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

https://codereview.chromium.org/11066032/diff/2001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:420:
IN_PROC_BROWSER_TEST_F(BrowserPluginHostTest, EmbedderVisibilityChanged) {
On 2012/10/05 00:33:18, creis wrote:
> I think these names are backwards, right?

Wow, you're right, I don't know how that happened :S Done.

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

https://codereview.chromium.org/11066032/diff/2001/content/common/browser_plu...
content/common/browser_plugin_messages.h:120: // Tells the guest to show or hide
itself
On 2012/10/05 00:33:18, creis wrote:
> nit: End with a period.
> 
> This comment is also a bit ambiguous.  Maybe "Tells the guest it has been
shown
> or hidden," to make it more clear that the embedder renderer has already taken
> the action?

Done.

[Charlie Reis, 2012-10-05 16:23:58]

Thanks, the comments help.  LGTM with 2 nits.

https://codereview.chromium.org/11066032/diff/8001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/11066032/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:144: // process (see
RenderProcessHostImpl::WidgetHidden).
Technically, when every WebContents in a RenderProcessHost is hidden, right?

https://codereview.chromium.org/11066032/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:146: // It will also send
a message to the  guest renderer process to cleanup
nit: extra space

[Fady Samuel, 2012-10-08 14:24:55]

Fixed nits, committing now.

https://codereview.chromium.org/11066032/diff/8001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/11066032/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:144: // process (see
RenderProcessHostImpl::WidgetHidden).
On 2012/10/05 16:23:58, creis wrote:
> Technically, when every WebContents in a RenderProcessHost is hidden, right?

Done.

https://codereview.chromium.org/11066032/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:146: // It will also send
a message to the  guest renderer process to cleanup
On 2012/10/05 16:23:58, creis wrote:
> nit: extra space

Done.

[Fady Samuel, 2012-10-08 14:24:55]

Fixed nits, committing now.

https://codereview.chromium.org/11066032/diff/8001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/11066032/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:144: // process (see
RenderProcessHostImpl::WidgetHidden).
On 2012/10/05 16:23:58, creis wrote:
> Technically, when every WebContents in a RenderProcessHost is hidden, right?

Done.

https://codereview.chromium.org/11066032/diff/8001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.h:146: // It will also send
a message to the  guest renderer process to cleanup
On 2012/10/05 16:23:58, creis wrote:
> nit: extra space

Done.

[commit-bot: I haz the power, 2012-10-08 14:25:20]

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

[commit-bot: I haz the power, 2012-10-08 15:48:53]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-10-08 16:02:07]

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

[commit-bot: I haz the power, 2012-10-08 16:25:11]

Change committed as 160660