VR: Factor transient timing out of UiSceneManager.

VR: Factor transient timing out of UiSceneManager.

- Add a class to manage transience. It can later be extended to handle
  more interesting transience, like URL bars, which appear when
  loading.
- Move presentation toast to use this new manager.

BUG=735163
Review-Url: https://codereview.chromium.org/2955023002
Cr-Commit-Position: refs/heads/master@{#483515}
Committed: https://chromium.googlesource.com/chromium/src/+/27ed3b381ba509ef709ebc97860d006611bab291

[cjgrant, 2017-06-26 20:34:19]

cjgrant@chromium.org changed reviewers:
+ bshe@chromium.org

[cjgrant, 2017-06-26 20:34:19]

Biao, here's an actual finished version of what I described on the whiteboard. 
If you like this, I would also use it to remove timers from the transient URL
bar and security warnings before submitting.

[cjgrant, 2017-06-26 20:39:35]

https://codereview.chromium.org/2955023002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2955023002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:443:
presentation_toast_->transience()->EndVisibility();
Biao, please note the logic here.  Basically, when transitioning from webvr back
to full-screen, the toast stays enabled, but we want to hide it (at least, I
think we do).  From the element's perspective, it is staying enabled, so it
would see no reason to hide itself.

There are other ways we could handle this.  One of them is, before calling
ConfigureScene(), explicitly disable the toast.  ConfigureScene() would then
re-enable it for fullscreen, but not kick the visibility timer.

[bshe, 2017-06-27 14:57:10]

https://codereview.chromium.org/2955023002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2955023002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_scene_manager.cc:443:
presentation_toast_->transience()->EndVisibility();
On 2017/06/26 20:39:35, cjgrant wrote:
> Biao, please note the logic here.  Basically, when transitioning from webvr
back
> to full-screen, the toast stays enabled, but we want to hide it (at least, I
> think we do).  From the element's perspective, it is staying enabled, so it
> would see no reason to hide itself.
> 
> There are other ways we could handle this.  One of them is, before calling
> ConfigureScene(), explicitly disable the toast.  ConfigureScene() would then
> re-enable it for fullscreen, but not kick the visibility timer.

It was previously hided I believe and I agree with you. Leave webvr presentation
and return to fullscreen shouldn't have the fullscreen toast visible.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/presentation_toast.cc (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/presentation_toast.cc:16:
transience_(this, base::TimeDelta::FromSeconds(10)) {}
Use |timeout| instead?

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/transience_manager.cc (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/transience_manager.cc:16: return;
no need to return

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/transience_manager.h (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/transience_manager.h:19: :
element_(element), timeout_(timeout) {
nit: move implementation to .cc file

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/transience_manager.h:26: void
KickVisibility();
Perhaps name the function to showIfEnabled? It is not very obvious from the
function name that if it is not enabled, it wont be visible.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/transience_manager.h:27: void
EndVisibility();
Remove this too?

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/transience_manager_unittest.cc
(right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/transience_manager_unittest.cc:5:
#include "chrome/browser/android/vr_shell/ui_elements/transience_manager.h"
nit: new line here

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:437:
presentation_toast_->SetEnabled(false);
Do you need this? Isn't ConfigureScene set enable to correct state?

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:446: if (web_vr)
Should this be if (web_vr && show_toast)? It still works as we only enable the
toast when the above is true. But it is less readable compare to (web_vr &&
show_toast)

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:591:
security_warning_timer_.Stop();
Stop shouldn't be necessary I believe. Start would reset previous timer.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:634: // TODO: Also do this
one!
Add ldap or create a crbug?

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:168:
base::ScopedMockTimeMessageLoopTaskRunner task_runner_;
Do you need this? Or do you want to add time sensitive test to this unit test
too?

[cjgrant, 2017-06-27 15:15:55]

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:634: // TODO: Also do this
one!
On 2017/06/27 14:57:09, bshe wrote:
> Add ldap or create a crbug?

I'm planning to do this before submitting.  I posted the CL to make sure you
approve of the method before changing all the other elements to use it.  There's
a follow-up patch coming.  And sorry, this wasn't as polished as it should have
been.  :)

[cjgrant, 2017-06-28 19:00:55]

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/presentation_toast.cc (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/presentation_toast.cc:16:
transience_(this, base::TimeDelta::FromSeconds(10)) {}
On 2017/06/27 14:57:09, bshe wrote:
> Use |timeout| instead?

Done.  Oops!

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/transience_manager.cc (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/transience_manager.cc:16: return;
On 2017/06/27 14:57:09, bshe wrote:
> no need to return 

Done.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/transience_manager.h (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/transience_manager.h:19: :
element_(element), timeout_(timeout) {
On 2017/06/27 14:57:09, bshe wrote:
> nit: move implementation to .cc file

Done.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/transience_manager.h:26: void
KickVisibility();
On 2017/06/27 14:57:09, bshe wrote:
> Perhaps name the function to showIfEnabled? It is not very obvious from the
> function name that if it is not enabled, it wont be visible.

Done.  I use Kick instead of Show to keep the timer-specific part evident.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/transience_manager.h:27: void
EndVisibility();
On 2017/06/27 14:57:09, bshe wrote:
> Remove this too?

In this reworked version, we still need it.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements/transience_manager_unittest.cc
(right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements/transience_manager_unittest.cc:5:
#include "chrome/browser/android/vr_shell/ui_elements/transience_manager.h"
On 2017/06/27 14:57:09, bshe wrote:
> nit: new line here

Done.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:437:
presentation_toast_->SetEnabled(false);
On 2017/06/27 14:57:09, bshe wrote:
> Do you need this? Isn't ConfigureScene set enable to correct state?

See the reworked version, where we need the few lines below.  Let me know if the
comment is not helpful.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:446: if (web_vr)
On 2017/06/27 14:57:09, bshe wrote:
> Should this be if (web_vr && show_toast)? It still works as we only enable the
> toast when the above is true. But it is less readable compare to (web_vr &&
> show_toast)

I think, as it is, this is better.  It's more clear with the IfEnabled() suffix
you suggested.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:591:
security_warning_timer_.Stop();
On 2017/06/27 14:57:09, bshe wrote:
> Stop shouldn't be necessary I believe. Start would reset previous timer.

Done.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager.cc:634: // TODO: Also do this
one!
On 2017/06/27 15:15:55, cjgrant wrote:
> On 2017/06/27 14:57:09, bshe wrote:
> > Add ldap or create a crbug?
> 
> I'm planning to do this before submitting.  I posted the CL to make sure you
> approve of the method before changing all the other elements to use it. 
There's
> a follow-up patch coming.  And sorry, this wasn't as polished as it should
have
> been.  :)

Done.

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):

https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:168:
base::ScopedMockTimeMessageLoopTaskRunner task_runner_;
On 2017/06/27 14:57:09, bshe wrote:
> Do you need this? Or do you want to add time sensitive test to this unit test
> too?

Done.  I made a separate test to verify transience.

[bshe, 2017-06-28 19:41:27]

On 2017/06/28 19:00:55, cjgrant wrote:
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/ui_elements/presentation_toast.cc
(right):
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_elements/presentation_toast.cc:16:
> transience_(this, base::TimeDelta::FromSeconds(10)) {}
> On 2017/06/27 14:57:09, bshe wrote:
> > Use |timeout| instead?
> 
> Done.  Oops!
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/ui_elements/transience_manager.cc
(right):
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_elements/transience_manager.cc:16: return;
> On 2017/06/27 14:57:09, bshe wrote:
> > no need to return 
> 
> Done.
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/ui_elements/transience_manager.h (right):
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_elements/transience_manager.h:19: :
> element_(element), timeout_(timeout) {
> On 2017/06/27 14:57:09, bshe wrote:
> > nit: move implementation to .cc file
> 
> Done.
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_elements/transience_manager.h:26: void
> KickVisibility();
> On 2017/06/27 14:57:09, bshe wrote:
> > Perhaps name the function to showIfEnabled? It is not very obvious from the
> > function name that if it is not enabled, it wont be visible.
> 
> Done.  I use Kick instead of Show to keep the timer-specific part evident.
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_elements/transience_manager.h:27: void
> EndVisibility();
> On 2017/06/27 14:57:09, bshe wrote:
> > Remove this too?
> 
> In this reworked version, we still need it.
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> File
chrome/browser/android/vr_shell/ui_elements/transience_manager_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_elements/transience_manager_unittest.cc:5:
> #include "chrome/browser/android/vr_shell/ui_elements/transience_manager.h"
> On 2017/06/27 14:57:09, bshe wrote:
> > nit: new line here
> 
> Done.
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/ui_scene_manager.cc (right):
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene_manager.cc:437:
> presentation_toast_->SetEnabled(false);
> On 2017/06/27 14:57:09, bshe wrote:
> > Do you need this? Isn't ConfigureScene set enable to correct state?
> 
> See the reworked version, where we need the few lines below.  Let me know if
the
> comment is not helpful.
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene_manager.cc:446: if (web_vr)
> On 2017/06/27 14:57:09, bshe wrote:
> > Should this be if (web_vr && show_toast)? It still works as we only enable
the
> > toast when the above is true. But it is less readable compare to (web_vr &&
> > show_toast)
> 
> I think, as it is, this is better.  It's more clear with the IfEnabled()
suffix
> you suggested.
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene_manager.cc:591:
> security_warning_timer_.Stop();
> On 2017/06/27 14:57:09, bshe wrote:
> > Stop shouldn't be necessary I believe. Start would reset previous timer.
> 
> Done.
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene_manager.cc:634: // TODO: Also do this
> one!
> On 2017/06/27 15:15:55, cjgrant wrote:
> > On 2017/06/27 14:57:09, bshe wrote:
> > > Add ldap or create a crbug?
> > 
> > I'm planning to do this before submitting.  I posted the CL to make sure you
> > approve of the method before changing all the other elements to use it. 
> There's
> > a follow-up patch coming.  And sorry, this wasn't as polished as it should
> have
> > been.  :)
> 
> Done.
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/2955023002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/ui_scene_manager_unittest.cc:168:
> base::ScopedMockTimeMessageLoopTaskRunner task_runner_;
> On 2017/06/27 14:57:09, bshe wrote:
> > Do you need this? Or do you want to add time sensitive test to this unit
test
> > too?
> 
> Done.  I made a separate test to verify transience.

lgtm

[cjgrant, 2017-06-29 18:51:54]

cjgrant@chromium.org changed reviewers:
+ ymalik@chromium.org

[cjgrant, 2017-06-29 18:51:55]

Biao, PTAL after the rebase onto your "exclusive" file/class rename.

Yash, please check the impact on auto-presentation, and feel free to weigh in on
the transience refactoring in general, of course.

[ymalik, 2017-06-29 19:13:32]

auto-presentation changes lgtm

[cjgrant, 2017-06-29 20:53:54]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-06-29 20:53:55]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2955023002/#ps80001
(title: "Rebase again onto ExclusivePresentationToast class/file rename.")

[commit-bot: I haz the power, 2017-06-29 20:54:28]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-29 22:24:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498769634564510,
"parent_rev": "9c98ed29555b35e3d3fcaa0e6fdeecbb87533dbc", "commit_rev":
"27ed3b381ba509ef709ebc97860d006611bab291"}

[commit-bot: I haz the power, 2017-06-29 22:24:24]

Description was changed from

==========
VR: Factor transient timing out of UiSceneManager.

- Add a class to manage transience. It can later be extended to handle
  more interesting transience, like URL bars, which appear when
  loading.
- Move presentation toast to use this new manager.

BUG=735163
==========

to

==========
VR: Factor transient timing out of UiSceneManager.

- Add a class to manage transience. It can later be extended to handle
  more interesting transience, like URL bars, which appear when
  loading.
- Move presentation toast to use this new manager.

BUG=735163

Review-Url: https://codereview.chromium.org/2955023002
Cr-Commit-Position: refs/heads/master@{#483515}
Committed:
https://chromium.googlesource.com/chromium/src/+/27ed3b381ba509ef709ebc97860d...
==========

[commit-bot: I haz the power, 2017-06-29 22:24:25]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/27ed3b381ba509ef709ebc97860d...