Make Page Visibility API work when the browser window is visible or not

Make Page Visibility API work when the browser window is visible or not

Currently, the Page Visibility API only works while switching between
tabs, but we need to make it work when the browser window is minimized
or hidden by switching between workspaces.

This patch only works for Ubuntu Unity and does not work in the lock
screen, which will be fixed in a separated patch.

BUG=293128

[joone, 2015-02-20 03:00:38]

joone.hur@intel.com changed reviewers:
+ jochen@chromium.org

[joone, 2015-02-20 03:00:38]

Hi Jochen,

Could you review my patch?
I made a little bit of change to the path of delivering the wasHidden message to
the render process in order to show content in a workspace view.

Thanks,
Joone

[joone, 2015-02-20 19:26:28]

joone.hur@intel.com changed reviewers:
+ erg@chromium.org

[jochen (gone - plz use gerrit), 2015-02-23 12:11:25]

is it possible to add tests for this?

I don't know enough about unity to really review this patch

[Elliot Glaysher, 2015-02-23 18:30:34]

On 2015/02/23 12:11:25, jochen (slow) wrote:
> is it possible to add tests for this?
> 
> I don't know enough about unity to really review this patch

This patch needs comments. I don't feel like I know what this |content_visible|
thing is that your passing through the various layers, how it differs from
|visible| when there is a |visible|, and why it's being passed during Hide
notifications.

[sadrul, 2015-02-23 21:43:33]

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

[sadrul, 2015-02-23 21:43:33]

https://codereview.chromium.org/944763002/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/944763002/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1253: bool
is_minimized = IsMinimized();
Can this be fixed/made cleverer to work correctly for
minimized/different-workspace cases?

[joone, 2015-02-23 21:53:45]

On 2015/02/23 18:30:34, Elliot Glaysher wrote:
> On 2015/02/23 12:11:25, jochen (slow) wrote:
> > is it possible to add tests for this?
> > 
> > I don't know enough about unity to really review this patch
> 
> This patch needs comments. I don't feel like I know what this
|content_visible|
> thing is that your passing through the various layers, how it differs from
> |visible| when there is a |visible|, and why it's being passed during Hide
> notifications.

We need to show content in Unity workspace view even if the browser window is
hidden because
we should see all windows in the workspace view. So, my patch allows to pass
|content_visible| to
RenderWidgetHostAura::Hide() in order not to hide content. 

Here is the example: while playing a video, we can stop the play using
VisibilityChange event with my patch
when moving to a different workspace. Then, when we switch to the workspace
view,
we can see the browser window, but it would not show any content if we didn't
pass |content_visible|.

[joone, 2015-02-23 22:06:45]

On 2015/02/23 12:11:25, jochen (slow) wrote:
> is it possible to add tests for this?

Is there any test case for reference? I need to control window manager
behaviors.
 
> I don't know enough about unity to really review this patch

My patch is not directly related to Unity. Under Unity, we can get
ConfigureNotify events when switching between workspaces.

[joone, 2015-02-23 23:26:18]

https://codereview.chromium.org/944763002/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/944763002/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1253: bool
is_minimized = IsMinimized();
On 2015/02/23 21:43:33, sadrul wrote:
> Can this be fixed/made cleverer to work correctly for
> minimized/different-workspace cases?

The Page Visibility API only works for minimizing/restoring a window without any
change so we don't need to handle VisibilityNotify events that are duplicated
events for minimize/restore case. I will update my patch.

[joone, 2015-03-02 19:51:23]

Could you review the updated patch?

[Elliot Glaysher, 2015-03-03 00:33:52]

Even after your comment in window.cc, I don't feel like I understand what this
patch is trying to do, or why Hide(true) makes sense as a concept.

https://codereview.chromium.org/944763002/diff/20001/ui/wm/core/focus_control...
File ui/wm/core/focus_controller.h (right):

https://codereview.chromium.org/944763002/diff/20001/ui/wm/core/focus_control...
ui/wm/core/focus_controller.h:74: bool contentVisible) override;
nit: camel case doesn't match the cc file, use unix_hacker for variable names.

[sadrul, 2015-03-03 00:41:46]

> This patch only works for Ubuntu Unity ...

Does that mean it doesn't work on other window-managers on linux? I think we
should find a solution that works for at least a majority of the window
managers.

https://codereview.chromium.org/944763002/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/944763002/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1262: }
If this block of code does the right thing (i.e. content_window_->Hide() is
called), that should be sufficient to trigger the visibility events (that's how
it works when switching between tabs, right?). Can you explain why this alone
wouldn't be sufficient?

[joone, 2015-03-03 03:11:18]

On 2015/03/03 00:33:52, Elliot Glaysher wrote:
> Even after your comment in window.cc, I don't feel like I understand what this
> patch is trying to do, or why Hide(true) makes sense as a concept.
> 

My patch aims to get signals when workspace is changed on Ubuntu Unity.
In addition, Window::SetPageVisibility method is used to show content in the
workspace view.
We can use Window::Hide() instead Window::SetPageVisibility, but in this case,
the browser window just becomes white in the workspace view.

>
https://codereview.chromium.org/944763002/diff/20001/ui/wm/core/focus_control...
> File ui/wm/core/focus_controller.h (right):
> 
>
https://codereview.chromium.org/944763002/diff/20001/ui/wm/core/focus_control...
> ui/wm/core/focus_controller.h:74: bool contentVisible) override;
> nit: camel case doesn't match the cc file, use unix_hacker for variable names.

I will fix it.

[joone, 2015-03-03 03:29:26]

On 2015/03/03 00:41:46, sadrul wrote:
> > This patch only works for Ubuntu Unity ...
> 
> Does that mean it doesn't work on other window-managers on linux? I think we
> should find a solution that works for at least a majority of the window
> managers.
> 

Yes, I checked Unity, GNOME3, and GNOME2 on Ubuntu.
There is no common solution so it needs separated patches.
The main reason is that each window manager sends x-signals to clients
differently.
It depends on whether window compositor is enabled or not.

Are there other window managers we need to care for?

[joone, 2015-03-03 03:30:22]

https://codereview.chromium.org/944763002/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/944763002/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1262: }
On 2015/03/03 00:41:46, sadrul wrote:
> If this block of code does the right thing (i.e. content_window_->Hide() is
> called), that should be sufficient to trigger the visibility events (that's
how
> it works when switching between tabs, right?). Can you explain why this alone
> wouldn't be sufficient?

Please see my previous comment.

[no sievers, 2015-03-03 21:12:53]

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

[no sievers, 2015-03-03 21:12:54]

https://codereview.chromium.org/944763002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/944763002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:616: if
(!content_visible)
Can you explain why it makes sense to have |window_| visible while hiding the
contents of the tab (i.e. potentially freeing the delegated layer below, since
we are calling delegated_frame_host_->WasHidden() below)? How does that
guarantee that it renders a valid layer?

Can you also update the comment in what the problem is specifically, and how
this patch fixes it?

We are having a similar problem on Android where there are cases that are
similar to the one described in the bug here, and we'd like the renderer to be
hidden while the view itself might not necessarily be hidden (or at least it's
still the foreground view in the browser, and it will be the first to show once
the app is restored). It's sort of a visible-but-frozen-state maybe, not sure.
Either way, it would be nice if we can potentially align the semantics a bit so
that we don't have to add multiple and confusing APIs and states.

[joone, 2015-03-04 05:22:45]

https://codereview.chromium.org/944763002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/944763002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:616: if
(!content_visible)
On 2015/03/03 21:12:53, sievers wrote:
> Can you explain why it makes sense to have |window_| visible while hiding the
> contents of the tab (i.e. potentially freeing the delegated layer below, since
> we are calling delegated_frame_host_->WasHidden() below)? How does that
> guarantee that it renders a valid layer?

I tried to call the Hide method when the browser window is out of view by
switching to another workspace, which allows to trigger visibilityChange events.
The problem is that content was not shown in the workspace view, so the
content_visible parameter was added not to call host_->wasHidden() in
RenderWidgetHostViewAura::Hide().

If this does not make sense, we could make a new path to deliver
visibilityChange events or out-of-view state to a render process.

> Can you also update the comment in what the problem is specifically, and how
> this patch fixes it?

Yes, sure.

[no sievers, 2015-03-04 19:21:25]

https://codereview.chromium.org/944763002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/944763002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:616: if
(!content_visible)
On 2015/03/04 05:22:45, joone wrote:
> On 2015/03/03 21:12:53, sievers wrote:
> > Can you explain why it makes sense to have |window_| visible while hiding
the
> > contents of the tab (i.e. potentially freeing the delegated layer below,
since
> > we are calling delegated_frame_host_->WasHidden() below)? How does that
> > guarantee that it renders a valid layer?
> 
> I tried to call the Hide method when the browser window is out of view by
> switching to another workspace, which allows to trigger visibilityChange
events.
> The problem is that content was not shown in the workspace view, so the
> content_visible parameter was added not to call host_->wasHidden() in
> RenderWidgetHostViewAura::Hide().
> 

This is the code right here. You mean the 'content_visible parameter was added
not to call *window->Hide()* ...' correct?

I think you didn't answer my question though.
I'm wondering how it's guaranteed that the following state is valid and can
render contents (and what contents if the delegated frame host drops the layer):

window is visible && RenderView is hidden

> If this does not make sense, we could make a new path to deliver
> visibilityChange events or out-of-view state to a render process.
> 

I think it might make sense to have this state where we sort of pause a view.

[joone, 2015-03-04 22:05:04]

https://codereview.chromium.org/944763002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/944763002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:616: if
(!content_visible)
On 2015/03/04 19:21:25, sievers wrote:
> On 2015/03/04 05:22:45, joone wrote:
> > On 2015/03/03 21:12:53, sievers wrote:
> > > Can you explain why it makes sense to have |window_| visible while hiding
> the
> > > contents of the tab (i.e. potentially freeing the delegated layer below,
> since
> > > we are calling delegated_frame_host_->WasHidden() below)? How does that
> > > guarantee that it renders a valid layer?
> > 
> > I tried to call the Hide method when the browser window is out of view by
> > switching to another workspace, which allows to trigger visibilityChange
> events.
> > The problem is that content was not shown in the workspace view, so the
> > content_visible parameter was added not to call host_->wasHidden() in
> > RenderWidgetHostViewAura::Hide().
> > 
> 
> This is the code right here. You mean the 'content_visible parameter was added
> not to call *window->Hide()* ...' correct?

Yes, because if we calll window->Hide(), we only see the browser window without
content.

> I think you didn't answer my question though.
> I'm wondering how it's guaranteed that the following state is valid and can
> render contents (and what contents if the delegated frame host drops the
layer):
> 
> window is visible && RenderView is hidden

You need to take a look at Window::SetPageVisibility() method I added and how it
differs from Window::SetVisible. Window::SetPageVisibility() method finally
calls  RenderWidgetHostViewAura::Hide(), which doesn't set any visibility to the
compositor and layer and just set the page visibility state(visibiltyState
evens) to a render process. The only problem is that the browser window didn't
show content so I added the content_visible parameter in order to show content.

> > If this does not make sense, we could make a new path to deliver
> > visibilityChange events or out-of-view state to a render process.
> > 
> 
> I think it might make sense to have this state where we sort of pause a view.

Ok, I will try to add out-of-view state message between the browser process and
render process.

[no sievers, 2015-03-04 22:48:44]

On 2015/03/04 22:05:04, joone wrote:
>
https://codereview.chromium.org/944763002/diff/20001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/944763002/diff/20001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_aura.cc:616: if
> (!content_visible)
> On 2015/03/04 19:21:25, sievers wrote:
> > On 2015/03/04 05:22:45, joone wrote:
> > > On 2015/03/03 21:12:53, sievers wrote:
> > > > Can you explain why it makes sense to have |window_| visible while
hiding
> > the
> > > > contents of the tab (i.e. potentially freeing the delegated layer below,
> > since
> > > > we are calling delegated_frame_host_->WasHidden() below)? How does that
> > > > guarantee that it renders a valid layer?
> > > 
> > > I tried to call the Hide method when the browser window is out of view by
> > > switching to another workspace, which allows to trigger visibilityChange
> > events.
> > > The problem is that content was not shown in the workspace view, so the
> > > content_visible parameter was added not to call host_->wasHidden() in
> > > RenderWidgetHostViewAura::Hide().
> > > 
> > 
> > This is the code right here. You mean the 'content_visible parameter was
added
> > not to call *window->Hide()* ...' correct?
> 
> Yes, because if we calll window->Hide(), we only see the browser window
without
> content.
> 
> > I think you didn't answer my question though.
> > I'm wondering how it's guaranteed that the following state is valid and can
> > render contents (and what contents if the delegated frame host drops the
> layer):
> > 
> > window is visible && RenderView is hidden
> 
> You need to take a look at Window::SetPageVisibility() method I added and how
it
> differs from Window::SetVisible. Window::SetPageVisibility() method finally
> calls  RenderWidgetHostViewAura::Hide(), which doesn't set any visibility to
the
> compositor and layer and just set the page visibility state(visibiltyState
> evens) to a render process. The only problem is that the browser window didn't
> show content so I added the content_visible parameter in order to show
content.
> 
> > > If this does not make sense, we could make a new path to deliver
> > > visibilityChange events or out-of-view state to a render process.
> > > 
> > 
> > I think it might make sense to have this state where we sort of pause a
view.
> 
> Ok, I will try to add out-of-view state message between the browser process
and
> render process.

Why? I'm increasingly confused, and I think you still didn't answer my question
above.

There are multiple things such as ui::window visibility, the window being backed
by a valid cc layer with valid contents (delegated or snapshot), and the
visibility state in the renderer.
RWHV::WasHidden() mostly deals with the latter. Why do you want to add another
IPC, what does that achieve?

I'm also not sure how far up the fact that the view is now not live anymore
(renderer is hidden i.e. 'page visibility' api) should be exposed.

[joone, 2015-03-04 23:10:42]

On 2015/03/04 22:48:44, sievers wrote:

> > > I think it might make sense to have this state where we sort of pause a
> view.
> > 
> > Ok, I will try to add out-of-view state message between the browser process
> and
> > render process.
> 
> Why? I'm increasingly confused, and I think you still didn't answer my
question
> above.
> 
> There are multiple things such as ui::window visibility, the window being
backed
> by a valid cc layer with valid contents (delegated or snapshot), and the
> visibility state in the renderer.
> RWHV::WasHidden() mostly deals with the latter. Why do you want to add another
> IPC, what does that achieve?
> 
> I'm also not sure how far up the fact that the view is now not live anymore
> (renderer is hidden i.e. 'page visibility' api) should be exposed.

I misunderstood that you thought the out-of-view state message is needed because
you mentioned "this state".
Anyway, yes, the view is not live anymore. It just renders a final snapshot of
content that was rendered before moving to another workspace.

[joone, 2015-03-09 05:02:21]

sievers: I've updated the comment. Could you review this?

[sadrul, 2015-03-09 17:30:21]

On 2015/03/03 03:29:26, joone wrote:
> On 2015/03/03 00:41:46, sadrul wrote:
> > > This patch only works for Ubuntu Unity ...
> > 
> > Does that mean it doesn't work on other window-managers on linux? I think we
> > should find a solution that works for at least a majority of the window
> > managers.
> > 
> 
> Yes, I checked Unity, GNOME3, and GNOME2 on Ubuntu.
> There is no common solution so it needs separated patches.
> The main reason is that each window manager sends x-signals to clients
> differently.
> It depends on whether window compositor is enabled or not.
> 
> Are there other window managers we need to care for?

We do try to make things work in as many window managers as possible unless the
cost is prohibitive.

I will note that this change fixes an issue for a fairly small number of users,
but adds a lot of cost in the code (confusing API regarding visibility in aura
and content). We should find a fix that works for most users consistently across
platforms (so at least the aura/views platforms (Windows, ChromeOS, Linux), and
preferably on MacOS too), and hopefully with a cleaner API.

[joone, 2015-03-09 22:54:45]

On 2015/03/09 17:30:21, sadrul wrote:
> On 2015/03/03 03:29:26, joone wrote:
> > On 2015/03/03 00:41:46, sadrul wrote:
> > > > This patch only works for Ubuntu Unity ...
> > > 
> > > Does that mean it doesn't work on other window-managers on linux? I think
we
> > > should find a solution that works for at least a majority of the window
> > > managers.
> > > 
> > 
> > Yes, I checked Unity, GNOME3, and GNOME2 on Ubuntu.
> > There is no common solution so it needs separated patches.
> > The main reason is that each window manager sends x-signals to clients
> > differently.
> > It depends on whether window compositor is enabled or not.
> > 
> > Are there other window managers we need to care for?
> 
> We do try to make things work in as many window managers as possible unless
the
> cost is prohibitive.
> 
> I will note that this change fixes an issue for a fairly small number of
users,
> but adds a lot of cost in the code (confusing API regarding visibility in aura
> and content). We should find a fix that works for most users consistently
across
> platforms (so at least the aura/views platforms (Windows, ChromeOS, Linux),
and
> preferably on MacOS too), and hopefully with a cleaner API.

Ok, I will try to make a cleaner API to fix this issue for the aura/views
platforms.

[no sievers, 2015-03-09 22:59:31]

On 2015/03/09 22:54:45, joone wrote:
> On 2015/03/09 17:30:21, sadrul wrote:
> > On 2015/03/03 03:29:26, joone wrote:
> > > On 2015/03/03 00:41:46, sadrul wrote:
> > > > > This patch only works for Ubuntu Unity ...
> > > > 
> > > > Does that mean it doesn't work on other window-managers on linux? I
think
> we
> > > > should find a solution that works for at least a majority of the window
> > > > managers.
> > > > 
> > > 
> > > Yes, I checked Unity, GNOME3, and GNOME2 on Ubuntu.
> > > There is no common solution so it needs separated patches.
> > > The main reason is that each window manager sends x-signals to clients
> > > differently.
> > > It depends on whether window compositor is enabled or not.
> > > 
> > > Are there other window managers we need to care for?
> > 
> > We do try to make things work in as many window managers as possible unless
> the
> > cost is prohibitive.
> > 
> > I will note that this change fixes an issue for a fairly small number of
> users,
> > but adds a lot of cost in the code (confusing API regarding visibility in
aura
> > and content). We should find a fix that works for most users consistently
> across
> > platforms (so at least the aura/views platforms (Windows, ChromeOS, Linux),
> and
> > preferably on MacOS too), and hopefully with a cleaner API.
> 
> Ok, I will try to make a cleaner API to fix this issue for the aura/views
> platforms.

A few thoughts - apart from the compatibility issues:
- maybe this doesn't have to be a content-API, but can rather be handled by the
aura windows internally somehow?
- you might want to double-check wrt my earlier question wrt delegated frame
eviction, and make sure that frames also cannot get evicted while you are still
in this 'content visible' state

[joone, 2015-04-07 21:38:56]

On 2015/03/09 22:59:31, sievers wrote:
> 
> A few thoughts - apart from the compatibility issues:
> - maybe this doesn't have to be a content-API, but can rather be handled by
the
> aura windows internally somehow?
> - you might want to double-check wrt my earlier question wrt delegated frame
> eviction, and make sure that frames also cannot get evicted while you are
still
> in this 'content visible' state

The updated patch used the ViewMsg_SetWindowVisibility message in order to pass
visibility state of the host window to the render process,
which allows the code to be changed minimally.
Currently, I'm working on ChromiumOS for fixing the same problem. It can also
use the ViewMsg_SetWindowVisibility message.