Initialize RenderWidget(Host)(View)s with correct visibility state

Initialize RenderWidget(Host)(View)s with correct visibility state

Currently, newly constructed RenderWidget(Host|View)*s assume they are
visible until they are notified otherwise. We've had a long list of bugs
where widgets created in the background would not receive any hidden
notification and would continue to operate as if they were visible. For
examples see bugs 118269, 142061, 155365 and 272962.

This makes the visibility of a widget a construction-time parameter and
sets the correct state at construction sites. Most widgets are created
to be visible. Notable exceptions are swapped-out widgets and widgets
created for background navigations. The initial widget visibility state is
passed down to the render process so the content::RenderWidget and Blink
have the correct state from the start.

RenderViewHostManager is a bit tricky since it can construct new
RenderViewHost instances on the fly due to navigation. This patch
adds a bool IsHidden function to RVHManager::Delegate so the manager
can ask the WebContentsImpl for the correct initial state.

BUG=20831, 155365, 272962
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218548

[jamesr, 2013-08-19 19:59:23]

Scott, could you review?  Thanks!

Charlie - please take a look in particular at the WebContentsImpl /
RenderViewHostManager changes and the content changes outside of
content/browser?

[sky, 2013-08-19 20:21:47]

https://codereview.chromium.org/23129015/diff/1/chrome/browser/ui/browser_nav...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/23129015/diff/1/chrome/browser/ui/browser_nav...
chrome/browser/ui/browser_navigator.cc:354: // TODO(sky): Figure out why this is
needed. Without it we seem to get
I wonder if this can be nuked now (don't feel you have to investigate this).

https://codereview.chromium.org/23129015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/test_render_view_host.cc (right):

https://codereview.chromium.org/23129015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/test_render_view_host.cc:251: true /* visible */),
Using enums also means you don't need comments like this.

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
File content/browser/web_contents/render_view_host_manager.h (right):

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager.h:295: const
NavigationEntryImpl& entry, bool hidden);
nit: when you wrap one param per line.

https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widge...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:242:
RenderProcessVisibilityManager::GetInstance()->WidgetVisibilityChanged(true);
How come we now longer need to invoke this?

https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widget.h
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widge...
content/renderer/render_widget.h:231: bool hidden);
From a readability perspective I prefer this to be an enum, so that call sites
look like:

new RenderWidget(...RenderWidget::VISIBILE), but that isn't in any style guide
so I leave it to you have if you want an enum here.

[jamesr, 2013-08-19 20:34:18]

On 2013/08/19 20:21:47, sky wrote:
>
https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widge...
> File content/renderer/render_widget.cc (left):
> 
>
https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widge...
> content/renderer/render_widget.cc:242:
> RenderProcessVisibilityManager::GetInstance()->WidgetVisibilityChanged(true);
> How come we now longer need to invoke this?

I removed the RenderProcessVisibilityManager.  All it did was keep track of the
# of visible widgets and invoked base::MemoryPressureListener when the visible
count hit zero.  RenderThreadImpl keeps track of the hidden widgets anyway so I
moved all of the logic into there.

> 
>
https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widget.h
> File content/renderer/render_widget.h (right):
> 
>
https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widge...
> content/renderer/render_widget.h:231: bool hidden);
> From a readability perspective I prefer this to be an enum, so that call sites
> look like:
> 
> new RenderWidget(...RenderWidget::VISIBILE), but that isn't in any style guide
> so I leave it to you have if you want an enum here.

That's what we prefer in Blink as well, but I've heard jam@ say he prefer bools
over enums.  If I went the enum route, I'd add it in content/common/ and use it
in both processes.  Sound good?

[sky, 2013-08-19 20:44:29]

On Mon, Aug 19, 2013 at 1:34 PM,  <jamesr@chromium.org> wrote:
> On 2013/08/19 20:21:47, sky wrote:
>
>
https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widge...
>>
>> File content/renderer/render_widget.cc (left):
>
>
>
>
https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widge...
>>
>> content/renderer/render_widget.cc:242:
>>
>>
RenderProcessVisibilityManager::GetInstance()->WidgetVisibilityChanged(true);
>> How come we now longer need to invoke this?
>
>
> I removed the RenderProcessVisibilityManager.  All it did was keep track of
> the
> # of visible widgets and invoked base::MemoryPressureListener when the
> visible
> count hit zero.  RenderThreadImpl keeps track of the hidden widgets anyway
> so I
> moved all of the logic into there.
>
>
>
>
>
https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widget.h
>>
>> File content/renderer/render_widget.h (right):
>
>
>
>
https://codereview.chromium.org/23129015/diff/1/content/renderer/render_widge...
>>
>> content/renderer/render_widget.h:231: bool hidden);
>>  From a readability perspective I prefer this to be an enum, so that call
>> sites
>> look like:
>
>
>> new RenderWidget(...RenderWidget::VISIBILE), but that isn't in any style
>> guide
>> so I leave it to you have if you want an enum here.
>
>
> That's what we prefer in Blink as well, but I've heard jam@ say he prefer
> bools
> over enums.  If I went the enum route, I'd add it in content/common/ and use
> it
> in both processes.  Sound good?
>
> https://codereview.chromium.org/23129015/

I'm not an OWNER there, so you better make sure jam (or someone else)
is ok with that.

  -Scott

[jamesr, 2013-08-19 21:02:19]

On 2013/08/19 20:44:29, sky wrote:
> > That's what we prefer in Blink as well, but I've heard jam@ say he prefer
> > bools
> > over enums.  If I went the enum route, I'd add it in content/common/ and use
> > it
> > in both processes.  Sound good?
> >
> > https://codereview.chromium.org/23129015/
> 
> I'm not an OWNER there, so you better make sure jam (or someone else)
> is ok with that.
> 
>   -Scott

I'm not either, and I think jam@ prefers bools so I'll leave this as is.

[sky, 2013-08-19 21:08:14]

LGTM

[Charlie Reis, 2013-08-19 22:18:02]

Looks pretty reasonable to me.  A few comments below.

https://codereview.chromium.org/23129015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23129015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_view_host_impl.h:113: // frame being
rendered by another process).
Can you mention the meaning of |hidden| up here?

https://codereview.chromium.org/23129015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/23129015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:127: is_hidden_(false),
Ah, so here's the culprit.  :)

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager.cc:648: bool hidden) {
Can we add a DCHECK that swapped_out implies hidden?  I'd love to catch any bugs
where swapped out RenderViewHosts are being considered visible.

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager.cc:904: hidden);
Style nit: No need for one param-per-line on function calls, just on function
declarations.

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
File content/browser/web_contents/render_view_host_manager.h (right):

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager.h:154: RenderViewHostImpl*
Navigate(const NavigationEntryImpl& entry, bool hidden);
I'd really like to avoid passing more parameters here and on
UpdateRendererStateForNavigate, especially non-navigation related ones.  Could
we add something like a IsHidden() method to RVHM::Delegate instead and pass
that into CreateRenderView?

(We already do a fair amount of state checking on the delegate_ in those
methods, so that wouldn't really be introducing a big dependency.)

https://codereview.chromium.org/23129015/diff/15001/chrome/browser/ui/tabs/ta...
File chrome/browser/ui/tabs/tab_strip_model.cc (left):

https://codereview.chromium.org/23129015/diff/15001/chrome/browser/ui/tabs/ta...
chrome/browser/ui/tabs/tab_strip_model.cc:709: // backing store of the visible
tab causing flashing. See bug 20831.
Maybe add 20831 to the BUG list of the CL?  There was no test on that CL, so it
would be helpful for this commit to post a message there in case it ever
regresses.

[jamesr, 2013-08-19 23:50:39]

PTAL, creis@.  I think I've addressed all feedback here.

jschuh@ - could you review the view_messages.h change?

https://codereview.chromium.org/23129015/diff/1/chrome/browser/ui/browser_nav...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/23129015/diff/1/chrome/browser/ui/browser_nav...
chrome/browser/ui/browser_navigator.cc:354: // TODO(sky): Figure out why this is
needed. Without it we seem to get
On 2013/08/19 20:21:47, sky wrote:
> I wonder if this can be nuked now (don't feel you have to investigate this).

I believe it can - I traced this through and it doesn't actually do anything
since the RenderWidgetHostView isn't made yet.

https://codereview.chromium.org/23129015/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/23129015/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_view_host_impl.h:113: // frame being
rendered by another process).
On 2013/08/19 22:18:02, creis wrote:
> Can you mention the meaning of |hidden| up here?

Done.

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager.cc:648: bool hidden) {
On 2013/08/19 22:18:02, creis wrote:
> Can we add a DCHECK that swapped_out implies hidden?  I'd love to catch any
bugs
> where swapped out RenderViewHosts are being considered visible.

Done.

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager.cc:904: hidden);
On 2013/08/19 22:18:02, creis wrote:
> Style nit: No need for one param-per-line on function calls, just on function
> declarations.

Hmm.  We're inconsistent on this, but sure.

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
File content/browser/web_contents/render_view_host_manager.h (right):

https://codereview.chromium.org/23129015/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager.h:154: RenderViewHostImpl*
Navigate(const NavigationEntryImpl& entry, bool hidden);
On 2013/08/19 22:18:02, creis wrote:
> I'd really like to avoid passing more parameters here and on
> UpdateRendererStateForNavigate, especially non-navigation related ones.  Could
> we add something like a IsHidden() method to RVHM::Delegate instead and pass
> that into CreateRenderView?
> 
> (We already do a fair amount of state checking on the delegate_ in those
> methods, so that wouldn't really be introducing a big dependency.)

OK, done.  I have to call it in two places - when calling CreateRenderView() and
when creating the initial view in ::Init().  I think it's still OK.

https://codereview.chromium.org/23129015/diff/15001/chrome/browser/ui/tabs/ta...
File chrome/browser/ui/tabs/tab_strip_model.cc (left):

https://codereview.chromium.org/23129015/diff/15001/chrome/browser/ui/tabs/ta...
chrome/browser/ui/tabs/tab_strip_model.cc:709: // backing store of the visible
tab causing flashing. See bug 20831.
On 2013/08/19 22:18:03, creis wrote:
> Maybe add 20831 to the BUG list of the CL?  There was no test on that CL, so
it
> would be helpful for this commit to post a message there in case it ever
> regresses.

Done.

[Charlie Reis, 2013-08-20 02:09:58]

Thanks, LGTM!  Don't forget to update the last bit of the CL description as
well.

https://codereview.chromium.org/23129015/diff/29001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.h (right):

https://codereview.chromium.org/23129015/diff/29001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.h:100: // Returns true if
views created for this delegate should be initially
nit: should currently be created in a hidden state.

(Or something similar, to make it clear that it depends on the current state of
the delegate, and may be different the next time it's called.)

https://codereview.chromium.org/23129015/diff/29001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/23129015/diff/29001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1551: bool hidden =
capturer_count_ == 0 && !should_normally_be_visible_;
We should probably call IsHidden instead, in case this logic evolves over time.

https://codereview.chromium.org/23129015/diff/29001/content/renderer/render_t...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/23129015/diff/29001/content/renderer/render_t...
content/renderer/render_thread_impl.cc:583: IPC::Listener* listener) {
Style nit: No need for this change, since both arguments can fit on the same
line as the function declaration.

(There's a clause on this in the "Code Formatting" bullets of
http://www.chromium.org/developers/coding-style.)

[commit-bot: I haz the power, 2013-08-20 20:42:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/23129015/34002

[commit-bot: I haz the power, 2013-08-20 20:47:10]

Change committed as 218548

[Cris Neckar, 2013-08-20 20:48:48]

IPC changes LGTM

[no sievers, 2013-09-17 18:42:08]

This causes a regression on Android (crbug.com/284427) - am not saying it's this
patches fault, but I'm trying to understand how this case is supposed to work:

1) navigation starts and a new RenderView is created with the current visibility
state, but it is not committed yet.
2) The app changes the visibility state with WasHidden/WasShown.
WebContentsImpl::WasHidden()/WasShown() forwards that to
render_manager_.render_view_host_->GetView() which is different from
pending_render_view_host_ created in 1).
3) the new RenderView gets committed, but it has the old visibility state from
1).

Looking at the Gtk code, there is also nothing obvious that syncs the visibility
state with the renderer.
During commit Show() is called on the new RWHV, but on Gtk that only seems to
show the window.

[jamesr, 2013-09-17 19:35:28]

The same problem exists on aura (and probably other platforms).  We're debugging
this in https://code.google.com/p/chromium/issues/detail?id=286259 and have a
patch in https://codereview.chromium.org/23866013/.  Can you check if that fixes
the android case?