Ignore show messages from RemoteFrames when the WebContents is hidden.

Ignore show messages from RemoteFrames when the WebContents is hidden.

BUG=628325
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9462cefe4c909d9be204df0fe005a5c2517741d7
Cr-Commit-Position: refs/heads/master@{#406153}

[lfg, 2016-07-15 17:39:38]

Description was changed from

==========
Ignore show messages from RemoteFrames when the WebContents is hidden.

BUG=628325
==========

to

==========
Ignore show messages from RemoteFrames when the WebContents is hidden.

BUG=628325
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[lfg, 2016-07-15 17:39:42]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 17:40:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-07-15 18:18:58]

lfg@chromium.org changed reviewers:
+ kenrb@chromium.org

[lfg, 2016-07-15 18:18:59]

Please take a look. This should fix the blocking issue, but there are still a
couple of issues I noticed when looking through this code, and I filled bugs
628700 and 628704 for follow-up work.

[lfg, 2016-07-15 18:20:22]

Description was changed from

==========
Ignore show messages from RemoteFrames when the WebContents is hidden.

BUG=628325
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ignore show messages from RemoteFrames when the WebContents is hidden.

BUG=628325
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-07-15 19:15:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-15 19:15:40]

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-07-18 17:11:45]

https://codereview.chromium.org/2154833002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/2154833002/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.cc:275:
->OnRenderFrameProxyVisibilityChanged(visible);
Why is the check done in the WebContents rather than here?

https://codereview.chromium.org/2154833002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2154833002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:197: // Returns true
if this RenderWidgetHost should be hidden.
I don't think this comment is very clear. My interpretation of this method would
be that it asks the delegate if the RenderWidgetHost can be shown, in the event
that something other than the WebContents is attempting to enable visibility.
It's a bit subtle and should be made more clear.

[lfg, 2016-07-18 17:44:01]

https://codereview.chromium.org/2154833002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/2154833002/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.cc:275:
->OnRenderFrameProxyVisibilityChanged(visible);
On 2016/07/18 17:11:45, kenrb wrote:
> Why is the check done in the WebContents rather than here?

No particular reason, I just thought the check in WebContents is simpler. Why do
you think it should be here?

Since we are crossing WebContents boundaries here, we would have to check the
delegate of the parent view instead of the delegate of the view which could be
more confusing than doing the check in the WebContents.

https://codereview.chromium.org/2154833002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2154833002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:197: // Returns true
if this RenderWidgetHost should be hidden.
On 2016/07/18 17:11:45, kenrb wrote:
> I don't think this comment is very clear. My interpretation of this method
would
> be that it asks the delegate if the RenderWidgetHost can be shown, in the
event
> that something other than the WebContents is attempting to enable visibility.
> It's a bit subtle and should be made more clear.

Yes, that interpretation is correct. I tried to clarify the comments based on
your suggestion, let me know what you think.

[lfg, 2016-07-18 17:44:06]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 17:44:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kenrb, 2016-07-18 18:05:28]

lgtm

https://codereview.chromium.org/2154833002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/2154833002/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.cc:275:
->OnRenderFrameProxyVisibilityChanged(visible);
On 2016/07/18 17:44:00, lfg wrote:
> On 2016/07/18 17:11:45, kenrb wrote:
> > Why is the check done in the WebContents rather than here?
> 
> No particular reason, I just thought the check in WebContents is simpler. Why
do
> you think it should be here?
> 
> Since we are crossing WebContents boundaries here, we would have to check the
> delegate of the parent view instead of the delegate of the view which could be
> more confusing than doing the check in the WebContents.

Okay, that's fine. The only advantage of having it here would be to have the
check in the same place for all RWHVCFs.

https://codereview.chromium.org/2154833002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2154833002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:197: // Returns true
if this RenderWidgetHost should be hidden. This is used by the
This is better. Maybe in the first sentence replace "should be hidden" with
"should remain hidden".

[lfg, 2016-07-18 18:07:28]

https://codereview.chromium.org/2154833002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2154833002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:197: // Returns true
if this RenderWidgetHost should be hidden. This is used by the
On 2016/07/18 18:05:28, kenrb wrote:
> This is better. Maybe in the first sentence replace "should be hidden" with
> "should remain hidden".

Done.

[lfg, 2016-07-18 18:32:10]

lfg@chromium.org changed reviewers:
+ alexmos@chromium.org

[lfg, 2016-07-18 18:32:10]

+alexmos, please take a look.

[alexmos, 2016-07-18 20:30:21]

LGTM, thanks for fixing!

[lfg, 2016-07-18 20:41:48]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[lfg, 2016-07-18 20:42:07]

lfg@chromium.org changed reviewers:
+ nasko@chromium.org

[lfg, 2016-07-18 20:42:08]

+nasko for owners review.

[commit-bot: I haz the power, 2016-07-18 20:42:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2016-07-18 21:33:22]

LGTM

[commit-bot: I haz the power, 2016-07-18 22:23:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 22:23:24]

Dry run: This issue passed the CQ dry run.

[lfg, 2016-07-18 23:43:04]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-07-18 23:43:04]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/2154833002/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2016-07-18 23:44:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 23:51:21]

Description was changed from

==========
Ignore show messages from RemoteFrames when the WebContents is hidden.

BUG=628325
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ignore show messages from RemoteFrames when the WebContents is hidden.

BUG=628325
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-07-18 23:51:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-18 23:51:36]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 23:54:27]

Description was changed from

==========
Ignore show messages from RemoteFrames when the WebContents is hidden.

BUG=628325
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ignore show messages from RemoteFrames when the WebContents is hidden.

BUG=628325
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9462cefe4c909d9be204df0fe005a5c2517741d7
Cr-Commit-Position: refs/heads/master@{#406153}
==========

[commit-bot: I haz the power, 2016-07-18 23:54:28]

Patchset 3 (id:??) landed as
https://crrev.com/9462cefe4c909d9be204df0fe005a5c2517741d7
Cr-Commit-Position: refs/heads/master@{#406153}