Hide/show widget on Windows screen lock/unlock

Hide/show widget on Windows screen lock/unlock to mimic behavior on other platforms (Mac,Android). 

Initial intention behind patch was to fix missing Page Visibility API
visibilitychange on Windows screen lock/unlock in example:
e.g. http://daniemon.com/tech/webapps/page-visibility/

Debugging showed that the problem is in NativeViewHostWrapper's HideWidget ->
NativeView::Hide() not getting called.
One of the related things was also missing call to
 Document::didChangeVisibilityState.

Some OSs (e.g. Windows and Linux) don't mark windows as hidden on screen
lock or when other opaque windows fully cover them.

Note that e.g. OSX and Android do this while Windows and Linux don't.

OnSoftVisibilityChanged enables that OS HWND window state is kept as is (on
Linux and Windows) and that e.g. widget is hidden and power saving PageVisibility API is still
properly triggered.

The (first) patch here targets Windows and screen lock - Linux support and
occlusion left for follow up patches.

BUG=532128

[aleksandar.stojiljkovic, 2016-02-29 13:51:13]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ sky@chromium.org

[tfarina, 2016-02-29 15:09:04]

Could you reword your subject line a bit? To say what is being changed here?
Just a short line, say, less than 70 characters. It should be a quick hint to
the reader about WHAT is being changed here.

I would also prefer if you prefix it (ok not to, if you prefer) with 'ui/views:'
rather than 'Page Visibility API'.
I don't think we have this concept ('Page Visibility API') explicitly anywhere
in views.

Thanks,

[aleksandar.stojiljkovic, 2016-02-29 16:55:50]

Description was changed from

==========
Page Visibility API: visibilitychange on Windows screen lock/unlock

Some OSs (e.g. Windows and Linux) don't mark windows as hidden on screen
lock or when other opaque windows fully cover them.

Note that e.g. OSX and Android do this while Windows and Linux don't.

OnSoftVisibilityChanged enables that OS window state is kept as is (on
Linux and Windows) and that e.g. power saving PageVisibility API is still
properly triggered.

The (first) patch here targets Windows and screen lock - Linux support and
occlusion left for follow up patches.
==========

to

==========
Hide/show widget on Windows screen lock/unlock to mimic behavior on other
platforms (Mac,Android). 

Initial intention behind patch was to fix missing Page Visibility API
visibilitychange on Windows screen lock/unlock in example:
e.g. http://daniemon.com/tech/webapps/page-visibility/

Debugging showed that the problem is in NativeViewHostWrapper's HideWidget ->
NativeView::Hide() not getting called.
One of the related things was also missing call to
 Document::didChangeVisibilityState.

Some OSs (e.g. Windows and Linux) don't mark windows as hidden on screen
lock or when other opaque windows fully cover them.

Note that e.g. OSX and Android do this while Windows and Linux don't.

OnSoftVisibilityChanged enables that OS HWND window state is kept as is (on
Linux and Windows) and that e.g. widget is hidden and power saving
PageVisibility API is still
properly triggered.

The (first) patch here targets Windows and screen lock - Linux support and
occlusion left for follow up patches.
==========

[aleksandar.stojiljkovic, 2016-02-29 16:56:24]

On 2016/02/29 15:09:04, tfarina wrote:
> Could you reword your subject line a bit? To say what is being changed here?
> Just a short line, say, less than 70 characters. It should be a quick hint to
> the reader about WHAT is being changed here.
> 
> I would also prefer if you prefix it (ok not to, if you prefer) with
'ui/views:'
> rather than 'Page Visibility API'.
> I don't think we have this concept ('Page Visibility API') explicitly anywhere
> in views.
> 
> Thanks,

Hope it is clearer now. Thanks.

[sky, 2016-02-29 16:58:19]

I think you're duplicating what we have in ui/base/idle. That interface is
currently polling based. But I think it's the right place to add the
functionality you're after.

[aleksandar.stojiljkovic, 2016-02-29 17:06:35]

On 2016/02/29 16:58:19, sky wrote:
> I think you're duplicating what we have in ui/base/idle. That interface is
> currently polling based. 

> But I think it's the right place to add the
> functionality you're after.
Do you mean that the right place to add this patch is to ui/base/idle?

Screen lock is first part of patch, I plan to add also the same when other
opaque window cover the widget (this is functionality on e.g. OSX). I assumed it
is good to keep that code in one place but if preferred, could split idle and
occlusion computation to different places.

Thanks.

[aleksandar.stojiljkovic, 2016-02-29 18:55:53]

On 2016/02/29 16:58:19, sky wrote:
> I think you're duplicating what we have in ui/base/idle. That interface is
> currently polling based. But I think it's the right place to add the
> functionality you're after.

Thanks for pointer to ui/base/idle.
Checked the ui/base/idle code. It is used for asynchronous monitoring of idle
state during some period (and as you pointed it is pulling on every 30s), in
order to:
1) launch upgrade if user is idle,
2) if monitoring user activity times, to log.

ui/base/idle is implemented based on checks if there is screensaver process
running (Linux, MAC, Win) and if workstation is locked(Win, Mac, ChromeOS).

On the other hand, widget hiding needs to behave (I'm using Android and OSX as
role model) instantaneously, and immediately on Win+L or when opaque window
covering. Idle has semantics of user inactivity on the system, hiding widget on
the other hand needs to be propagated while user is changing z order (on windows
now only when tab is sent to background or when minimized).
 
Screensaver covering view would need to be handled as any other window covering
it, and there is difference to Idle semantics, too.

Screen lock related:
Adding notification API to pulling idle API (designed to monitor for some time,
calculate and callback on timeout) looks like changing initial ui/base/idle
design idea, e.g. for people compulsively locking unlocking tablets...

[aleksandar.stojiljkovic, 2016-02-29 20:00:51]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ ananta@chromium.org

[aleksandar.stojiljkovic, 2016-02-29 20:00:52]

Including ananta@ too.

[sky, 2016-02-29 21:04:20]

Have you run this by anyone on the web side of things to make sure web
visibility, as you wish to implement it, is desired? I want to make sure you
don't add all this logic without making sure we want to actually use it on the
web side.

[aleksandar.stojiljkovic, 2016-02-29 21:31:28]

Description was changed from

==========
Hide/show widget on Windows screen lock/unlock to mimic behavior on other
platforms (Mac,Android). 

Initial intention behind patch was to fix missing Page Visibility API
visibilitychange on Windows screen lock/unlock in example:
e.g. http://daniemon.com/tech/webapps/page-visibility/

Debugging showed that the problem is in NativeViewHostWrapper's HideWidget ->
NativeView::Hide() not getting called.
One of the related things was also missing call to
 Document::didChangeVisibilityState.

Some OSs (e.g. Windows and Linux) don't mark windows as hidden on screen
lock or when other opaque windows fully cover them.

Note that e.g. OSX and Android do this while Windows and Linux don't.

OnSoftVisibilityChanged enables that OS HWND window state is kept as is (on
Linux and Windows) and that e.g. widget is hidden and power saving
PageVisibility API is still
properly triggered.

The (first) patch here targets Windows and screen lock - Linux support and
occlusion left for follow up patches.
==========

to

==========
Hide/show widget on Windows screen lock/unlock to mimic behavior on other
platforms (Mac,Android). 

Initial intention behind patch was to fix missing Page Visibility API
visibilitychange on Windows screen lock/unlock in example:
e.g. http://daniemon.com/tech/webapps/page-visibility/

Debugging showed that the problem is in NativeViewHostWrapper's HideWidget ->
NativeView::Hide() not getting called.
One of the related things was also missing call to
 Document::didChangeVisibilityState.

Some OSs (e.g. Windows and Linux) don't mark windows as hidden on screen
lock or when other opaque windows fully cover them.

Note that e.g. OSX and Android do this while Windows and Linux don't.

OnSoftVisibilityChanged enables that OS HWND window state is kept as is (on
Linux and Windows) and that e.g. widget is hidden and power saving
PageVisibility API is still
properly triggered.

The (first) patch here targets Windows and screen lock - Linux support and
occlusion left for follow up patches.

BUG=532128
==========

[aleksandar.stojiljkovic, 2016-02-29 21:38:50]

On 2016/02/29 21:04:20, sky wrote:
> Have you run this by anyone on the web side of things to make sure web
> visibility, as you wish to implement it, is desired? I want to make sure you
> don't add all this logic without making sure we want to actually use it on the
> web side.

Thanks.
No,... based the need for this fix on discrepancy between OSx&Android vs
Windows/Linux and then found the arguments in specification:
http://www.w3.org/TR/page-visibility/
"
For example, in the following cases the visibilityState attribute would return
the DOMString hidden:
•The User Agent is minimized.
•The User Agent is not minimized, but the page is on a background tab.
•The Operating System lock screen is shown.
"

Found related bug 532128 and will find someone more to include.

[aleksandar.stojiljkovic, 2016-02-29 21:47:47]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ ccameron@chromium.org

[aleksandar.stojiljkovic, 2016-02-29 21:47:47]

ccameron@,
We are trying to evaluate a need for this patch and approach, hope you could
help. Thanks.

[aleksandar.stojiljkovic, 2016-03-01 17:49:07]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ abarth@chromium.org, davidben@chromium.org, shishir@chromium.org

[aleksandar.stojiljkovic, 2016-03-01 17:49:07]

On 2016/02/29 21:04:20, sky wrote:
> Have you run this by anyone on the web side of things to make sure web
> visibility, as you wish to implement it, is desired? I want to make sure you
> don't add all this logic without making sure we want to actually use it on the
> web side.

davidben@, abarth@, shishir@
You were involved in PageVisibility API implementation - could you please
comment if this is desired functionality on Windows (page visibility events on
screen lock/unlock)?
Thanks.

[davidben, 2016-03-01 18:20:24]

On 2016/03/01 17:49:07, aleksandar.stojiljkovic wrote:
> On 2016/02/29 21:04:20, sky wrote:
> > Have you run this by anyone on the web side of things to make sure web
> > visibility, as you wish to implement it, is desired? I want to make sure you
> > don't add all this logic without making sure we want to actually use it on
the
> > web side.
> 
> davidben@, abarth@, shishir@
> You were involved in PageVisibility API implementation - could you please
> comment if this is desired functionality on Windows (page visibility events on
> screen lock/unlock)?
> Thanks.

This sounds like a good idea to me, especially if we already do it on other
platforms. A web app which turns off some high-resource things when the tab is
invisible probably ought to do the same when the screen is locked.

But I'm also pretty far from web platform stuff normally. blink-dev@ might be a
better place if you want broader discussion.

[nduca, 2016-03-04 18:29:41]

I think this sounds "Correct" wrt web platform behavior. I'd suggest pinging a
few people on IM to confirm --- igrigorik and ojan, but you can probably do that
out of band with working through fixing this.

[igrigorik, 2016-03-04 21:45:12]

On 2016/03/04 18:29:41, nduca wrote:
> I think this sounds "Correct" wrt web platform behavior. 

Yep, PV spec covers this case explicitly [1]: hidden if the operating system
lock screen is shown.

[1] http://w3c.github.io/page-visibility/#visibilitystate-attribute

[aleksandar.stojiljkovic, 2016-03-07 17:40:50]

On 2016/02/29 21:04:20, sky wrote:
> Have you run this by anyone on the web side of things to make sure web
> visibility, as you wish to implement it, is desired? I want to make sure you
> don't add all this logic without making sure we want to actually use it on the
> web side.

#18, #19, #20 are covering this.
The patch for case when other processes are displayed in fullscreen mode is also
ready (triggering the same code path as lock/unlock here).
I'll wait for positive review on this to submit that for review - or if you'd
prefer, it could be added to this patch?

[sky, 2016-03-07 18:51:45]

On 2016/03/07 17:40:50, aleksandar.stojiljkovic wrote:
> On 2016/02/29 21:04:20, sky wrote:
> > Have you run this by anyone on the web side of things to make sure web
> > visibility, as you wish to implement it, is desired? I want to make sure you
> > don't add all this logic without making sure we want to actually use it on
the
> > web side.
> 
> #18, #19, #20 are covering this.
> The patch for case when other processes are displayed in fullscreen mode is
also
> ready (triggering the same code path as lock/unlock here).
> I'll wait for positive review on this to submit that for review - or if you'd
> prefer, it could be added to this patch?

It seems like we're saying we only care about this for lock screens. Whether a
lock screen is present is global, and not per widget setting. I don't think you
should change ui/views at all for this.

[aleksandar.stojiljkovic, 2016-03-07 22:46:03]

On 2016/03/07 18:51:45, sky wrote:
> On 2016/03/07 17:40:50, aleksandar.stojiljkovic wrote:
> > On 2016/02/29 21:04:20, sky wrote:
> > > Have you run this by anyone on the web side of things to make sure web
> > > visibility, as you wish to implement it, is desired? I want to make sure
you
> > > don't add all this logic without making sure we want to actually use it on
> the
> > > web side.
> > 
> > #18, #19, #20 are covering this.
> > The patch for case when other processes are displayed in fullscreen mode is
> also
> > ready (triggering the same code path as lock/unlock here).
> > I'll wait for positive review on this to submit that for review - or if
you'd
> > prefer, it could be added to this patch?
> 

> It seems like we're saying we only care about this for lock screens.
About second patch (when other processes are displayed in fullscreen mode) - on
Android and Mac, Visibility API covers also other windows covering document.
Spec states: "hidden - The Document is not visible at all on any screen. ". So,
when e.g. VLC goes fullscreen, chrome can save power.

I have a question to ask, before providing updated patch. Thanks.

> Whether a
> lock screen is present is global, and not per widget setting. I don't think
you
> should change ui/views at all for this.

On the end, the API is like that - "document is hidden" event when user session
gets locked. And, that API is different from Windows global events like session
change and "other process window went fullscreen".

However, there is need to translate this event to gfx::NativeView::Hide().

The question I need to ask before updating the patch:
Is my understanding correct,... you don't think that the code path from
HWNDMessageHandler::OnSessionChange to NativeViewHost through views is correct -
if code would go through dedicated observer notification path, then it would be
OK?

As a reference (something that I'm after), on OSX this task is about performance
(and about API):
Issue 310374: Mavericks: Use occlusion APIs for power savings

[sky, 2016-03-07 23:54:16]

igrigorik@chromium.org earlier said:

 [1]: hidden if the operating system lock screen is shown.

Which is global, not per window. To truly track per window obscuring
would be a major pain. That's why I'm questioning the need for it.

  -Scott

On Mon, Mar 7, 2016 at 2:46 PM,  <aleksandar.stojiljkovic@intel.com> wrote:
> On 2016/03/07 18:51:45, sky wrote:
>> On 2016/03/07 17:40:50, aleksandar.stojiljkovic wrote:
>> > On 2016/02/29 21:04:20, sky wrote:
>> > > Have you run this by anyone on the web side of things to make sure web
>> > > visibility, as you wish to implement it, is desired? I want to make
>> > > sure
> you
>> > > don't add all this logic without making sure we want to actually use
>> > > it on
>> the
>> > > web side.
>> >
>> > #18, #19, #20 are covering this.
>> > The patch for case when other processes are displayed in fullscreen mode
>> > is
>> also
>> > ready (triggering the same code path as lock/unlock here).
>> > I'll wait for positive review on this to submit that for review - or if
> you'd
>> > prefer, it could be added to this patch?
>>
>
>> It seems like we're saying we only care about this for lock screens.
> About second patch (when other processes are displayed in fullscreen mode) -
> on
> Android and Mac, Visibility API covers also other windows covering document.
> Spec states: "hidden - The Document is not visible at all on any screen. ".
> So,
> when e.g. VLC goes fullscreen, chrome can save power.
>
> I have a question to ask, before providing updated patch. Thanks.
>
>> Whether a
>> lock screen is present is global, and not per widget setting. I don't
>> think
> you
>> should change ui/views at all for this.
>
> On the end, the API is like that - "document is hidden" event when user
> session
> gets locked. And, that API is different from Windows global events like
> session
> change and "other process window went fullscreen".
>
> However, there is need to translate this event to gfx::NativeView::Hide().
>
> The question I need to ask before updating the patch:
> Is my understanding correct,... you don't think that the code path from
> HWNDMessageHandler::OnSessionChange to NativeViewHost through views is
> correct -
> if code would go through dedicated observer notification path, then it would
> be
> OK?
>
> As a reference (something that I'm after), on OSX this task is about
> performance
> (and about API):
> Issue 310374: Mavericks: Use occlusion APIs for power savings
>
>
>
> https://codereview.chromium.org/1746013002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aleksandar.stojiljkovic, 2016-03-08 09:43:14]

On 2016/03/07 23:54:16, sky wrote:
> mailto:igrigorik@chromium.org earlier said:
> 
>  [1]: hidden if the operating system lock screen is shown.
> 
> Which is global, not per window. To truly track per window obscuring
> would be a major pain. That's why I'm questioning the need for it.
> 
>   -Scott
> 

I understand. Yes, for "per window obscuring" it's needed to use SetWindowHookEx
on messages which would have performance impact. I will work on Lunux, too for
all possible "global notifications" before revisiting per window obscuring cases
and benefit vs cost.

What about the patch here? Which parts you suggest to redesign?

Thanks.

PS.
Some more details about global vs per window handling:
Luckily there are a few things that can be done without that (and that have
global character as you mention)
- lock screen (the patch under review here)
- other (processes) windows going fullscreen (just for the reference patch is
like this [1], will post it in later reviews)
- monitor (in single monitor mode) turned off
- screensaver (they don't go through fullscreen notification, depending on
screensaver app).

[1]
https://github.com/crosswalk-project/chromium-crosswalk/pull/326/files

[sky, 2016-03-08 17:09:37]

It is still not clear to me what cases you are trying to handle.
Please clarify that. I propose we deal with lock screen, screen saver
and nothing else.

On Tue, Mar 8, 2016 at 1:43 AM,  <aleksandar.stojiljkovic@intel.com> wrote:
> On 2016/03/07 23:54:16, sky wrote:
>> mailto:igrigorik@chromium.org earlier said:
>>
>> [1]: hidden if the operating system lock screen is shown.
>>
>> Which is global, not per window. To truly track per window obscuring
>> would be a major pain. That's why I'm questioning the need for it.
>>
>> -Scott
>>
>
> I understand. Yes, for "per window obscuring" it's needed to use
> SetWindowHookEx
> on messages which would have performance impact. I will work on Lunux, too
> for
> all possible "global notifications" before revisiting per window obscuring
> cases
> and benefit vs cost.
>
> What about the patch here? Which parts you suggest to redesign?
>
> Thanks.
>
> PS.
> Some more details about global vs per window handling:
> Luckily there are a few things that can be done without that (and that have
> global character as you mention)
> - lock screen (the patch under review here)
> - other (processes) windows going fullscreen (just for the reference patch
> is
> like this [1], will post it in later reviews)
> - monitor (in single monitor mode) turned off
> - screensaver (they don't go through fullscreen notification, depending on
> screensaver app).
>
> [1]
> https://github.com/crosswalk-project/chromium-crosswalk/pull/326/files
>
> https://codereview.chromium.org/1746013002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aleksandar.stojiljkovic, 2016-03-08 20:53:28]

On 2016/03/08 17:09:37, sky wrote:
> It is still not clear to me what cases you are trying to handle.
> Please clarify that. I propose we deal with lock screen, screen saver
> and nothing else.

In order of priority:

1) lock screen
2) screen saver

Later and this is not trivial (multiple monitors, remote session...) and also
with questionable benefit:
3) other processes going fullscreen - questionable benefit as user are used to
shutting down browser on Windows before running games or playing movies.
4) monitors turn down (questionable if there is suc thing in API).

So, for 1 and 2 - same notification can be used to start pulling timers for idle
use cases (#8)?

[sky, 2016-03-08 22:54:45]

Doing 1 and 2 now is fine by me. I question whether we want 3 and 4 at all.

That means we're back to the global api, not a per widget api. By
which I mean a way to determine when the system is locked or screen
saver is up that does not entail polling.

  -Scott

On Tue, Mar 8, 2016 at 12:53 PM,  <aleksandar.stojiljkovic@intel.com> wrote:
> On 2016/03/08 17:09:37, sky wrote:
>> It is still not clear to me what cases you are trying to handle.
>> Please clarify that. I propose we deal with lock screen, screen saver
>> and nothing else.
>
> In order of priority:
>
> 1) lock screen
> 2) screen saver
>
> Later and this is not trivial (multiple monitors, remote session...) and
> also
> with questionable benefit:
> 3) other processes going fullscreen - questionable benefit as user are used
> to
> shutting down browser on Windows before running games or playing movies.
> 4) monitors turn down (questionable if there is suc thing in API).
>
> So, for 1 and 2 - same notification can be used to start pulling timers for
> idle
> use cases (#8)?
>
>
>
> https://codereview.chromium.org/1746013002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.