Notifying the Out of Process Renderer about Visibility Change of a Remote Frame

Notifying the Out of Process Renderer about Visibility Change of a Remote Frame.

When a remote frame's visibility changes, the corresponding renderer (out of process) should be notified. This is achieved by plumbing a new IPC to the out of process renderer.

BUG=550994
Committed: https://crrev.com/babb9bf4ee6d35696aa61acc2eaefdb57fa21bde
Cr-Commit-Position: refs/heads/master@{#368875}

[EhsanK, 2015-11-03 18:54:55]

Description was changed from

==========
Added the Plumbing to Notify Browser/Guest Renderer about Visibility Changes of
<iframe>
Added the Plumbing to Notify Browser/Guest Renderer about Visibility Changes of
<iframe>. This should help the guest renderer process stop generating graphic
frames and painting when the <iframe> is made invisible.

BUG=550994
==========

to

==========
Added the Plumbing to Notify Browser/Guest Renderer about Visibility Changes of
<iframe>. This should help the guest renderer process stop generating graphic
frames and painting when the <iframe> is made invisible.

BUG=550994
==========

[EhsanK, 2015-11-03 20:38:23]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org, kenrb@chromium.org

[EhsanK, 2015-11-03 20:38:23]

PTAL.

[Charlie Reis, 2015-11-03 20:54:04]

[+site-isolation-reviews]

Thanks!

A few general notes:

Please CC site-isolation-dev@chromium.org on OOPIF-related CLs, to help our team
track the work being done and keep an eye out for potential issues.

Also, when listing multiple reviewers, it can help to mention what you'd like
each person to look at.  (Or you can start with one reviewer and then add owners
once you have the first approval.)  In this case, Ken knows this code better
than me, so I'll let him do the main review and I'll follow up for content/
OWNERS.

Also try to include tests if you can.  Seems like it should be possible in this
case.  Thanks!

[EhsanK, 2015-11-03 20:56:25]

On 2015/11/03 20:54:04, Charlie Reis (slow til 11-16) wrote:
> [+site-isolation-reviews]
> 
> Thanks!
> 
> A few general notes:
> 
> Please CC mailto:site-isolation-dev@chromium.org on OOPIF-related CLs, to help
our team
> track the work being done and keep an eye out for potential issues.
> 
> Also, when listing multiple reviewers, it can help to mention what you'd like
> each person to look at.  (Or you can start with one reviewer and then add
owners
> once you have the first approval.)  In this case, Ken knows this code better
> than me, so I'll let him do the main review and I'll follow up for content/
> OWNERS.
> 
> Also try to include tests if you can.  Seems like it should be possible in
this
> case.  Thanks!

Thanks for the advice. I will add tests. Any suggestions on who to add for the
blink side?

[Charlie Reis, 2015-11-03 20:57:51]

On 2015/11/03 20:56:25, ehsaaan wrote:
> On 2015/11/03 20:54:04, Charlie Reis (slow til 11-16) wrote:
> > [+site-isolation-reviews]
> > 
> > Thanks!
> > 
> > A few general notes:
> > 
> > Please CC mailto:site-isolation-dev@chromium.org on OOPIF-related CLs, to
help
> our team
> > track the work being done and keep an eye out for potential issues.
> > 
> > Also, when listing multiple reviewers, it can help to mention what you'd
like
> > each person to look at.  (Or you can start with one reviewer and then add
> owners
> > once you have the first approval.)  In this case, Ken knows this code better
> > than me, so I'll let him do the main review and I'll follow up for content/
> > OWNERS.
> > 
> > Also try to include tests if you can.  Seems like it should be possible in
> this
> > case.  Thanks!
> 
> Thanks for the advice. I will add tests. Any suggestions on who to add for the
> blink side?

dcheng@ is a Blink OWNER on the Site Isolation team.  It will probably make
sense to add him once Ken is happy with the CL.

[kenrb, 2015-11-04 21:57:42]

A test would be good. Also, I am concerned that this might have unexpected
consequences on account of this bug:
https://code.google.com/p/chromium/issues/detail?id=550574

An interesting test, with this patch in place, would be to create two
out-of-process iframes going to the same target page, and then have the parent
frame hide one of the iframes and see if the other one still works properly.

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

https://codereview.chromium.org/1414663011/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.cc:223:
view_->SetVisibility(visible);
Why not just call Show() or Hide() directly, rather than adding a new method to
RWHVChildFrame?

[EhsanK, 2015-11-05 15:57:21]

On 2015/11/04 21:57:42, kenrb wrote:
> A test would be good. Also, I am concerned that this might have unexpected
> consequences on account of this bug:
> https://code.google.com/p/chromium/issues/detail?id=550574
> 
> An interesting test, with this patch in place, would be to create two
> out-of-process iframes going to the same target page, and then have the parent
> frame hide one of the iframes and see if the other one still works properly.
> 
>
https://codereview.chromium.org/1414663011/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/cross_process_frame_connector.cc (right):
> 
>
https://codereview.chromium.org/1414663011/diff/1/content/browser/frame_host/...
> content/browser/frame_host/cross_process_frame_connector.cc:223:
> view_->SetVisibility(visible);
> Why not just call Show() or Hide() directly, rather than adding a new method
to
> RWHVChildFrame?

Thanks for the review. I am first trying to replicate the scenario locally and
observe what happens. I will add the test after.

[EhsanK, 2015-11-05 15:57:23]

On 2015/11/04 21:57:42, kenrb wrote:
> A test would be good. Also, I am concerned that this might have unexpected
> consequences on account of this bug:
> https://code.google.com/p/chromium/issues/detail?id=550574
> 
> An interesting test, with this patch in place, would be to create two
> out-of-process iframes going to the same target page, and then have the parent
> frame hide one of the iframes and see if the other one still works properly.
> 
>
https://codereview.chromium.org/1414663011/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/cross_process_frame_connector.cc (right):
> 
>
https://codereview.chromium.org/1414663011/diff/1/content/browser/frame_host/...
> content/browser/frame_host/cross_process_frame_connector.cc:223:
> view_->SetVisibility(visible);
> Why not just call Show() or Hide() directly, rather than adding a new method
to
> RWHVChildFrame?

Thanks for the review. I am first trying to replicate the scenario locally and
observe what happens. I will add the test after.

[EhsanK, 2015-11-05 15:57:24]

On 2015/11/04 21:57:42, kenrb wrote:
> A test would be good. Also, I am concerned that this might have unexpected
> consequences on account of this bug:
> https://code.google.com/p/chromium/issues/detail?id=550574
> 
> An interesting test, with this patch in place, would be to create two
> out-of-process iframes going to the same target page, and then have the parent
> frame hide one of the iframes and see if the other one still works properly.
> 
>
https://codereview.chromium.org/1414663011/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/cross_process_frame_connector.cc (right):
> 
>
https://codereview.chromium.org/1414663011/diff/1/content/browser/frame_host/...
> content/browser/frame_host/cross_process_frame_connector.cc:223:
> view_->SetVisibility(visible);
> Why not just call Show() or Hide() directly, rather than adding a new method
to
> RWHVChildFrame?

Thanks for the review. I am first trying to replicate the scenario locally and
observe what happens. I will add the test after.

[EhsanK, 2015-11-05 15:57:40]

PTAL.

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

https://codereview.chromium.org/1414663011/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.cc:223:
view_->SetVisibility(visible);
On 2015/11/04 21:57:42, kenrb wrote:
> Why not just call Show() or Hide() directly, rather than adding a new method
to
> RWHVChildFrame?

This is a provision for <webview>.
My understanding is that those methods will get called when we call WasHidden or
WasShown on the WebContentsImpl. That being said, in case of <webview>, we will
only need to forward the call to those of the WebContentsImpl and then the
RWHVCF ones will be called. If we call Show() and Hide() directly here, and then
forward those calls to WebContentsImpl, the Show() and Hide() will get called
again (twice). That is the only reason I though of having an extra function like
this so that I can add <webview> code later in it.

I can remove but maybe add it again later when fixing the <webview> issue?

[EhsanK, 2015-11-05 15:57:40]

PTAL.

[kenrb, 2015-11-05 18:41:50]

Okay, I am happy with this once a browser test is added.

[EhsanK, 2015-11-06 20:08:50]

ptal.

[EhsanK, 2015-11-07 02:12:42]

ekaramad@chromium.org changed reviewers:
+ dcheng@chromium.org

[EhsanK, 2015-11-07 02:12:43]

+dcheng@chromium.org: for the blink side. Thanks!

[dcheng, 2015-11-10 00:17:07]

(I haven't forgotten about this CL, but I'm traveling to BlinkOn so my reply may
be a bit delayed)

[kenrb, 2015-11-11 16:09:34]

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

https://codereview.chromium.org/1414663011/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.cc:223:
view_->SetVisibility(visible);
On 2015/11/05 15:57:39, ehsaaan wrote:
> On 2015/11/04 21:57:42, kenrb wrote:
> > Why not just call Show() or Hide() directly, rather than adding a new method
> to
> > RWHVChildFrame?
> 
> This is a provision for <webview>.
> My understanding is that those methods will get called when we call WasHidden
or
> WasShown on the WebContentsImpl. That being said, in case of <webview>, we
will
> only need to forward the call to those of the WebContentsImpl and then the
> RWHVCF ones will be called. If we call Show() and Hide() directly here, and
then
> forward those calls to WebContentsImpl, the Show() and Hide() will get called
> again (twice). That is the only reason I though of having an extra function
like
> this so that I can add <webview> code later in it.
> 
> I can remove but maybe add it again later when fixing the <webview> issue?

This sounds a bit hacky, but I don't know the <webview> code well enough to
suggest anything better.

Can we still remove RWHVCF::SetVisibility() by putting the check here instead?
You should be able to call
view_->GetRenderWidgetHost()->delegate()->DidForwardVisibilityChangeToGuest(visible),
and then call Hide() or Show() directly, correct?

https://codereview.chromium.org/1414663011/diff/20001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/1414663011/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:115: void
OnVisibilityChanged(bool visible);
At some point we might want to roll On*Changed messages into a single update,
but this is fine for now.

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

https://codereview.chromium.org/1414663011/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:109: // change in
visiblity is forwarded to all RenderWidgetHostView's.
nit: Spelling of 'visibility', and no apostrophe in RenderWidgetHostViews.

[EhsanK, 2015-11-11 19:13:17]

ekaramad@chromium.org changed reviewers:
+ lazyboy@chromium.org

[EhsanK, 2015-11-11 19:13:18]

PTAL.
+lazyboy@chromium.org to take a look at changes in
cross_process_frame_connector.cc.

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

https://codereview.chromium.org/1414663011/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.cc:223:
view_->SetVisibility(visible);
On 2015/11/11 16:09:33, kenrb wrote:
> On 2015/11/05 15:57:39, ehsaaan wrote:
> > On 2015/11/04 21:57:42, kenrb wrote:
> > > Why not just call Show() or Hide() directly, rather than adding a new
method
> > to
> > > RWHVChildFrame?
> > 
> > This is a provision for <webview>.
> > My understanding is that those methods will get called when we call
WasHidden
> or
> > WasShown on the WebContentsImpl. That being said, in case of <webview>, we
> will
> > only need to forward the call to those of the WebContentsImpl and then the
> > RWHVCF ones will be called. If we call Show() and Hide() directly here, and
> then
> > forward those calls to WebContentsImpl, the Show() and Hide() will get
called
> > again (twice). That is the only reason I though of having an extra function
> like
> > this so that I can add <webview> code later in it.
> > 
> > I can remove but maybe add it again later when fixing the <webview> issue?
> 
> This sounds a bit hacky, but I don't know the <webview> code well enough to
> suggest anything better.
> 
> Can we still remove RWHVCF::SetVisibility() by putting the check here instead?
> You should be able to call
>
view_->GetRenderWidgetHost()->delegate()->DidForwardVisibilityChangeToGuest(visible),
> and then call Hide() or Show() directly, correct?

Done. I will also add Istiaque to take a look at this then.

https://codereview.chromium.org/1414663011/diff/20001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/1414663011/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:115: void
OnVisibilityChanged(bool visible);
On 2015/11/11 16:09:33, kenrb wrote:
> At some point we might want to roll On*Changed messages into a single update,
> but this is fine for now.

Acknowledged.

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

https://codereview.chromium.org/1414663011/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:109: // change in
visiblity is forwarded to all RenderWidgetHostView's.
On 2015/11/11 16:09:34, kenrb wrote:
> nit: Spelling of 'visibility', and no apostrophe in RenderWidgetHostViews.

Acknowledged.

[lazyboy, 2015-11-11 22:04:27]

Just looked at CPFC and a bit of the test.

https://chromiumcodereview.appspot.com/1414663011/diff/40001/content/browser/...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://chromiumcodereview.appspot.com/1414663011/diff/40001/content/browser/...
content/browser/frame_host/cross_process_frame_connector.cc:224: if
(static_cast<RenderWidgetHostImpl*>(view_->GetRenderWidgetHost())
1) If you only want to check if this is for a guest or not,
use ForInnerDelegate(), see line 156 above for example.

2) variables/functions referring to "guests" should use
"Inner WebContents" inside content/, e.g.
DidForwardVisibilityChangeToInnerContents()

https://chromiumcodereview.appspot.com/1414663011/diff/40001/content/browser/...
File content/browser/site_per_process_browsertest.cc (right):

https://chromiumcodereview.appspot.com/1414663011/diff/40001/content/browser/...
content/browser/site_per_process_browsertest.cc:321: message_loop_runner_ = new
MessageLoopRunner;
This tight polling should be avoided.

Use NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED.
However that notification thing is also supposed to die, but should
be better than the tight loop here.

[EhsanK, 2015-11-12 19:54:30]

PTAL.

https://codereview.chromium.org/1414663011/diff/40001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1414663011/diff/40001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:224: if
(static_cast<RenderWidgetHostImpl*>(view_->GetRenderWidgetHost())
On 2015/11/11 22:04:27, lazyboy wrote:
> 1) If you only want to check if this is for a guest or not,
> use ForInnerDelegate(), see line 156 above for example.
> 
> 2) variables/functions referring to "guests" should use
> "Inner WebContents" inside content/, e.g.
> DidForwardVisibilityChangeToInnerContents()

Done.

https://codereview.chromium.org/1414663011/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1414663011/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:321: message_loop_runner_ = new
MessageLoopRunner;
On 2015/11/11 22:04:27, lazyboy wrote:
> This tight polling should be avoided.
> 
> Use NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED.
> However that notification thing is also supposed to die, but should
> be better than the tight loop here.

Done.

[Charlie Reis, 2015-11-20 23:35:29]

I've mainly been waiting for Ken to approve before giving the owner's stamp. 
Ken, any more thoughts?

One nit below in the meantime.

https://codereview.chromium.org/1414663011/diff/80001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1414663011/diff/80001/content/common/frame_me...
content/common/frame_messages.h:1083: // be then forwarded to the guest
WebContents.
nit: guest -> inner.

It's also not clear to me from the comment whether this IPC is specific to
GuestView cases or applies to out-of-process iframes as well.  Is the second
sentence only true in the case of an inner WebContents, but the IPC is sent for
any RenderFrameProxy?  (Please clarify.)

[Fady Samuel, 2015-11-23 16:29:45]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-11-23 16:29:45]

https://codereview.chromium.org/1414663011/diff/80001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1414663011/diff/80001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:227:
static_cast<RenderWidgetHostImpl*>(view_->GetRenderWidgetHost())
nit: I think RenderWidgetHostImpl::From(...) is preferred?

[kenrb, 2015-11-26 16:27:53]

lgtm from my side

[dcheng, 2015-12-02 00:15:05]

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

https://codereview.chromium.org/1414663011/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:110: virtual void
ForwardVisibilityChangeToInnerContents(bool visible) {}
What will this method be used for? It seems a bit weird to plumb it through
RWHD, and it doesn't seem like it's actually used.

https://codereview.chromium.org/1414663011/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1414663011/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:334: failed_ =
expected_visibility_state_ ^
I think == would be clearer (and that's fine, since there are bools on both
sides)

https://codereview.chromium.org/1414663011/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrameView.cpp (right):

https://codereview.chromium.org/1414663011/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:87:
m_remoteFrame->visibilityChanged(isVisible());
I'm having trouble convincing myself this is correct.

Why is it OK to omit this callback if !isSelfVisible() == false?

https://codereview.chromium.org/1414663011/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right):

https://codereview.chromium.org/1414663011/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrameView.h:38: 
Nit: remove the newline here and on line 40. Also, we should group all the
Widget overrides together and put a comment at the top that says "// Widget
overrides:"

[EhsanK, 2015-12-07 16:10:15]

PTAL.

https://codereview.chromium.org/1414663011/diff/80001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1414663011/diff/80001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:227:
static_cast<RenderWidgetHostImpl*>(view_->GetRenderWidgetHost())
On 2015/11/23 16:29:45, Fady Samuel wrote:
> nit: I think RenderWidgetHostImpl::From(...) is preferred?

Done.

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

https://codereview.chromium.org/1414663011/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:110: virtual void
ForwardVisibilityChangeToInnerContents(bool visible) {}
On 2015/12/02 00:15:05, dcheng wrote:
> What will this method be used for? It seems a bit weird to plumb it through
> RWHD, and it doesn't seem like it's actually used.

This method is being called in cross_process_frame_connector.cc.

This will be implemented by WebContentsImpl. It will later be used for <webview>
to forward the change in visibility state to the Guest/Inner(?) WebContents.

https://codereview.chromium.org/1414663011/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1414663011/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:334: failed_ =
expected_visibility_state_ ^
On 2015/12/02 00:15:05, dcheng wrote:
> I think == would be clearer (and that's fine, since there are bools on both
> sides)

Done.

https://codereview.chromium.org/1414663011/diff/80001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1414663011/diff/80001/content/common/frame_me...
content/common/frame_messages.h:1083: // be then forwarded to the guest
WebContents.
On 2015/11/20 23:35:29, Charlie Reis wrote:
> nit: guest -> inner.
> 
> It's also not clear to me from the comment whether this IPC is specific to
> GuestView cases or applies to out-of-process iframes as well.  Is the second
> sentence only true in the case of an inner WebContents, but the IPC is sent
for
> any RenderFrameProxy?  (Please clarify.)

The second sentence was confusing so I removed it.
It applies to GuestView cases as well as oopif. "...message will be then
forwarded..." meant to say that if the frame corresponds to a <webview>, then
the corresponding WebContents for the guest (InnerWebContents) should be also
notified. But that is an issue to be dealt with later at
CrossProcessFrameConnector.

https://codereview.chromium.org/1414663011/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrameView.cpp (right):

https://codereview.chromium.org/1414663011/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:87:
m_remoteFrame->visibilityChanged(isVisible());
On 2015/12/02 00:15:05, dcheng wrote:
> I'm having trouble convincing myself this is correct.
> 
> Why is it OK to omit this callback if !isSelfVisible() == false?

I actually inferred the logic from here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

What I thought to be true was that if this Widget is set to invisible, then
setting the parent to visible/invisible should not change anything. Please help
me understand the error. Thanks!

https://codereview.chromium.org/1414663011/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrameView.h (right):

https://codereview.chromium.org/1414663011/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrameView.h:38: 
On 2015/12/02 00:15:05, dcheng wrote:
> Nit: remove the newline here and on line 40. Also, we should group all the
> Widget overrides together and put a comment at the top that says "// Widget
> overrides:"

Done.

[Charlie Reis, 2015-12-08 19:18:34]

One question about ForwardVisibilityChangeToInnerContents, and otherwise looks
good with nits.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/frame_...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1414663011/diff/140001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.cc:234: return;
nit: Add blank line after this, before the next block.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.cc:242: }
nit: Same here.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1414663011/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:128: virtual void
ForwardVisibilityChangeToInnerContents(bool visible) {}
This isn't implemented anywhere.  Is it stale, or do you plan to override it
somewhere?

(We should either implement it now or leave this part out of this CL and add it
when you're ready.)

https://codereview.chromium.org/1414663011/diff/140001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1414663011/diff/140001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:522: bool observed_;
nit: was_observed_

https://codereview.chromium.org/1414663011/diff/140001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:523: bool failed_;
nit: did_fail_

https://codereview.chromium.org/1414663011/diff/140001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4255: // Verify that hiding
leads to a notification from RenderWidgetHost
nit: End with period.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4260: // Verify showing leads to
a notification as well.
nit: Add blank line above.

[EhsanK, 2015-12-09 00:14:17]

PTAL.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/frame_...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1414663011/diff/140001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.cc:234: return;
On 2015/12/08 19:18:33, Charlie Reis wrote:
> nit: Add blank line after this, before the next block.

Done.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.cc:242: }
On 2015/12/08 19:18:33, Charlie Reis wrote:
> nit: Same here.

Done.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1414663011/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:128: virtual void
ForwardVisibilityChangeToInnerContents(bool visible) {}
On 2015/12/08 19:18:33, Charlie Reis wrote:
> This isn't implemented anywhere.  Is it stale, or do you plan to override it
> somewhere?
> 
> (We should either implement it now or leave this part out of this CL and add
it
> when you're ready.)

I did override it in WebContentsImpl. Purely, this method's purpose is to inform
the out of process renderer of the visibility change and is only valid for
<guestview>'s. That is why I initially left out the implementation to make this
CL only related to oopif.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1414663011/diff/140001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:522: bool observed_;
On 2015/12/08 19:18:34, Charlie Reis wrote:
> nit: was_observed_

Done.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:523: bool failed_;
On 2015/12/08 19:18:34, Charlie Reis wrote:
> nit: did_fail_

Done.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4255: // Verify that hiding
leads to a notification from RenderWidgetHost
On 2015/12/08 19:18:33, Charlie Reis wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:4260: // Verify showing leads to
a notification as well.
On 2015/12/08 19:18:34, Charlie Reis wrote:
> nit: Add blank line above.

Done.

[Charlie Reis, 2015-12-10 07:45:20]

Ah, that explains it-- thanks.  A few related questions below.

https://codereview.chromium.org/1414663011/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1414663011/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:128: virtual void
ForwardVisibilityChangeToInnerContents(bool visible) {}
On 2015/12/09 00:14:16, ehsaaan wrote:
> On 2015/12/08 19:18:33, Charlie Reis wrote:
> > This isn't implemented anywhere.  Is it stale, or do you plan to override it
> > somewhere?
> > 
> > (We should either implement it now or leave this part out of this CL and add
> it
> > when you're ready.)
> 
> I did override it in WebContentsImpl. Purely, this method's purpose is to
inform
> the out of process renderer of the visibility change and is only valid for
> <guestview>'s. That is why I initially left out the implementation to make
this
> CL only related to oopif.

Leaving it out for now would be fine, but if so we should leave out this
declaration (and the call) as well as the implementation, adding them both
later.

(I'm fine either way at this point.)

https://codereview.chromium.org/1414663011/diff/160001/content/browser/frame_...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1414663011/diff/160001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.cc:236: if
(frame_proxy_in_parent_renderer_->frame_tree_node()
I feel like this is a bit hard to follow.  Can you add a comment explaining it?

https://codereview.chromium.org/1414663011/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1414663011/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:126: // If there is
a guest WebContents, it should be notified of the visibility
nit: s/guest/inner/

https://codereview.chromium.org/1414663011/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1414663011/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4849: void
WebContentsImpl::ForwardVisibilityChangeToInnerContents(bool visible) {
Please list this with the other RWHD overrides.

I have some concerns about the name, since it sounds like something you would
call on the outer WebContents so that it forwards something to its inner
WebContents (which doesn't make sense, since there could be multiple).  In
actuality, it looks like it's meant to be called on the inner WebContents
itself.

Can we just call it ForwardVisibilityChange and clarify in the declaration and
call site that it's being called on the inner WebContents?

[EhsanK, 2015-12-10 17:08:31]

PTAL.

https://codereview.chromium.org/1414663011/diff/160001/content/browser/frame_...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/1414663011/diff/160001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.cc:236: if
(frame_proxy_in_parent_renderer_->frame_tree_node()
On 2015/12/10 07:45:19, Charlie Reis wrote:
> I feel like this is a bit hard to follow.  Can you add a comment explaining
it?

This part is checking if the corresponding RWHVCF corresponds to the main frame
of <webview>. Before this I was simply checking if a BrowserPluginGuest exists
in the WebContents but lazyboy@ pointed me towards this line.

I have removed this logic from the CL. I will add it again (with comments) in
the next one.

https://codereview.chromium.org/1414663011/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1414663011/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:126: // If there is
a guest WebContents, it should be notified of the visibility
On 2015/12/10 07:45:19, Charlie Reis wrote:
> nit: s/guest/inner/


Acknowledged. This function is removed now.

https://codereview.chromium.org/1414663011/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1414663011/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4849: void
WebContentsImpl::ForwardVisibilityChangeToInnerContents(bool visible) {
On 2015/12/10 07:45:19, Charlie Reis wrote:
> Please list this with the other RWHD overrides.
> 
> I have some concerns about the name, since it sounds like something you would
> call on the outer WebContents so that it forwards something to its inner
> WebContents (which doesn't make sense, since there could be multiple).  In
> actuality, it looks like it's meant to be called on the inner WebContents
> itself.
> 
> Can we just call it ForwardVisibilityChange and clarify in the declaration and
> call site that it's being called on the inner WebContents?

For this CL I removed the function altogether. But I will call it that when I
submit the <webview> CL. Thanks!

[Charlie Reis, 2015-12-10 17:21:25]

Thanks, LGTM!  Please also update the CL description to not mention guests.

[dcheng, 2015-12-10 18:31:17]

Patch looks good. Can we add a unit test in Blink for this as well?

[EhsanK, 2015-12-10 18:45:32]

Description was changed from

==========
Added the Plumbing to Notify Browser/Guest Renderer about Visibility Changes of
<iframe>. This should help the guest renderer process stop generating graphic
frames and painting when the <iframe> is made invisible.

BUG=550994
==========

to

==========
Notifying the Out of Process Renderer about Visibility Change of a Remote Frame.

When a remote frame's visibility changes, the corresponding renderer (out of
process) should be notified. This is achieved by plumbing a new IPC to the out
of process renderer.

BUG=550994
==========

[EhsanK, 2015-12-22 20:42:57]

Patchset #11 (id:200001) has been deleted

[EhsanK, 2015-12-22 20:49:13]

@dcheng: I added two unit tests to blink.

PTAL.

[dcheng, 2016-01-06 18:43:58]

https://codereview.chromium.org/1414663011/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp (right):

https://codereview.chromium.org/1414663011/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp:204: }
Nit: newline

https://codereview.chromium.org/1414663011/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.h (right):

https://codereview.chromium.org/1414663011/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.h:166: void
visibilityChanged(bool visible) override { m_visible = visible; }
Move these to a subclass of TestWebRemoteFrameClient? I don't think this is a
method that's going to be used by other tests.

https://codereview.chromium.org/1414663011/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.h:168: bool isVisible() {
return m_visible; }
Nit: const

[EhsanK, 2016-01-08 22:05:37]

PTAL.

https://codereview.chromium.org/1414663011/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp (right):

https://codereview.chromium.org/1414663011/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp:204: }
On 2016/01/06 18:43:58, dcheng wrote:
> Nit: newline

Done.

https://codereview.chromium.org/1414663011/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.h (right):

https://codereview.chromium.org/1414663011/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.h:166: void
visibilityChanged(bool visible) override { m_visible = visible; }
On 2016/01/06 18:43:58, dcheng wrote:
> Move these to a subclass of TestWebRemoteFrameClient? I don't think this is a
> method that's going to be used by other tests.

Done.

https://codereview.chromium.org/1414663011/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.h:168: bool isVisible() {
return m_visible; }
On 2016/01/06 18:43:58, dcheng wrote:
> Nit: const

Done.

[dcheng, 2016-01-11 19:07:59]

lgtm with a nit

https://codereview.chromium.org/1414663011/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.h (right):

https://codereview.chromium.org/1414663011/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.h:171: class
TestWebRemoteFrameClientForVisibility : public TestWebRemoteFrameClient {
Put this directly in WebFrameTest.cpp: no need to declare it here.

[EhsanK, 2016-01-12 04:54:54]

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

[commit-bot: I haz the power, 2016-01-12 04:55:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414663011/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414663011/280001

[commit-bot: I haz the power, 2016-01-12 06:54:49]

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

[commit-bot: I haz the power, 2016-01-12 06:54:50]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-01-12 15:11:46]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-01-12 15:11:47]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1414663011/#ps280001
(title: "Removed the Call to WebRemoteFrameImpl::close() and
WebViewHelper::reset() in the Destructor of the …")

[commit-bot: I haz the power, 2016-01-12 15:12:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414663011/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414663011/280001

[commit-bot: I haz the power, 2016-01-12 15:17:16]

Description was changed from

==========
Notifying the Out of Process Renderer about Visibility Change of a Remote Frame.

When a remote frame's visibility changes, the corresponding renderer (out of
process) should be notified. This is achieved by plumbing a new IPC to the out
of process renderer.

BUG=550994
==========

to

==========
Notifying the Out of Process Renderer about Visibility Change of a Remote Frame.

When a remote frame's visibility changes, the corresponding renderer (out of
process) should be notified. This is achieved by plumbing a new IPC to the out
of process renderer.

BUG=550994
==========

[commit-bot: I haz the power, 2016-01-12 15:17:18]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2016-01-12 15:18:35]

Description was changed from

==========
Notifying the Out of Process Renderer about Visibility Change of a Remote Frame.

When a remote frame's visibility changes, the corresponding renderer (out of
process) should be notified. This is achieved by plumbing a new IPC to the out
of process renderer.

BUG=550994
==========

to

==========
Notifying the Out of Process Renderer about Visibility Change of a Remote Frame.

When a remote frame's visibility changes, the corresponding renderer (out of
process) should be notified. This is achieved by plumbing a new IPC to the out
of process renderer.

BUG=550994

Committed: https://crrev.com/babb9bf4ee6d35696aa61acc2eaefdb57fa21bde
Cr-Commit-Position: refs/heads/master@{#368875}
==========

[commit-bot: I haz the power, 2016-01-12 15:18:36]

Patchset 14 (id:??) landed as
https://crrev.com/babb9bf4ee6d35696aa61acc2eaefdb57fa21bde
Cr-Commit-Position: refs/heads/master@{#368875}

[EhsanK, 2016-06-09 17:42:53]

Publishing the long-left-unpublished comment.

https://codereview.chromium.org/1414663011/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.h (right):

https://codereview.chromium.org/1414663011/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/FrameTestHelpers.h:171: class
TestWebRemoteFrameClientForVisibility : public TestWebRemoteFrameClient {
On 2016/01/11 19:07:59, dcheng wrote:
> Put this directly in WebFrameTest.cpp: no need to declare it here.

Done.