Add countdown when session time is limited

Add countdown when session time is limited

This CL adds a countdown to the ash tray when the session time is limited.
The UI can/will be improved further in the future. This is a first basic
functional implementation.

BUG=chromium-os:26957
TEST=Manual
TBR=sky@chromium.org
(for ash.gyp and ash_strings.grd changes)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173293

[bartfab (slow), 2012-12-14 17:06:25]

Hi Steven,

Please take a look.

[stevenjb, 2012-12-14 18:02:21]

We really need to pass the expired time and update the seconds in the tray
itself. It shouldn't be a big change and the code is pretty fragile otherwise.

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
File ash/system/session_length_limit/session_length_limit_observer.h (right):

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
ash/system/session_length_limit/session_length_limit_observer.h:22: const
base::TimeDelta& remaining) = 0;
This really needs to be OnSessionExpiredTimeChanged(), otherwise the countdown
could be uneven which would look really strange.

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
File ash/system/session_length_limit/tray_session_length_limit.cc (right):

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
ash/system/session_length_limit/tray_session_length_limit.cc:51:
label->set_owned_by_client();
We really shouldn't be using set_owned_by_client under normal circumstances, see
comment below.

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
ash/system/session_length_limit/tray_session_length_limit.cc:76: void
OnRemainingSessionTimeChanged(const base::TimeDelta& remaining);
The names of these imply that they are callbacks, when really these are setters
called by the observer. We should rename them to Set* instead of On*

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
ash/system/session_length_limit/tray_session_length_limit.cc:90:
scoped_ptr<views::Label> label_seconds_right_;
I see that this pattern is copied from TimeView, but I'm not a fan. I think this
would be much simpler if the views were just owned by the view hierarchy and the
entire class rebuilt when the structure changes. Unowned pointers can be used
instead for the views that need to be modified. That said, I won't push back on
this since we already use the pattern. It might be worthwhile in the future to
create a time view as a helper class that both of this and TimeView could use.

Also, label_ should have a better name (maybe label_full_ or label_all_) and a
comment above it and above the others explaining that one is for horizontal
layout and the others for vertical.

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
ash/system/session_length_limit/tray_session_length_limit.cc:98: bool
horizontal_layout) {
Use ShelfAlignment instead of a bool for layout and in UpdateClockLayout() and
SetBorder(); enums are generally better than bools, especially as function
parameters.

[bartfab (slow), 2012-12-14 20:11:09]

Hi Steven, please take another look.

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
File ash/system/session_length_limit/session_length_limit_observer.h (right):

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
ash/system/session_length_limit/session_length_limit_observer.h:22: const
base::TimeDelta& remaining) = 0;
On 2012/12/14 18:02:21, stevenjb (chromium) wrote:
> This really needs to be OnSessionExpiredTimeChanged(), otherwise the countdown
> could be uneven which would look really strange.

Changed to use an internal periodic timer.

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
File ash/system/session_length_limit/tray_session_length_limit.cc (right):

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
ash/system/session_length_limit/tray_session_length_limit.cc:51:
label->set_owned_by_client();
On 2012/12/14 18:02:21, stevenjb (chromium) wrote:
> We really shouldn't be using set_owned_by_client under normal circumstances,
see
> comment below.

Done.

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
ash/system/session_length_limit/tray_session_length_limit.cc:76: void
OnRemainingSessionTimeChanged(const base::TimeDelta& remaining);
On 2012/12/14 18:02:21, stevenjb (chromium) wrote:
> The names of these imply that they are callbacks, when really these are
setters
> called by the observer. We should rename them to Set* instead of On*

Done.

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
ash/system/session_length_limit/tray_session_length_limit.cc:90:
scoped_ptr<views::Label> label_seconds_right_;
On 2012/12/14 18:02:21, stevenjb (chromium) wrote:
> I see that this pattern is copied from TimeView, but I'm not a fan. I think
this
> would be much simpler if the views were just owned by the view hierarchy and
the
> entire class rebuilt when the structure changes. Unowned pointers can be used
> instead for the views that need to be modified. That said, I won't push back
on
> this since we already use the pattern. It might be worthwhile in the future to
> create a time view as a helper class that both of this and TimeView could use.
> 
> Also, label_ should have a better name (maybe label_full_ or label_all_) and a
> comment above it and above the others explaining that one is for horizontal
> layout and the others for vertical.

Done.

https://chromiumcodereview.appspot.com/11568036/diff/1/ash/system/session_len...
ash/system/session_length_limit/tray_session_length_limit.cc:98: bool
horizontal_layout) {
On 2012/12/14 18:02:21, stevenjb (chromium) wrote:
> Use ShelfAlignment instead of a bool for layout and in UpdateClockLayout() and
> SetBorder(); enums are generally better than bools, especially as function
> parameters.

Done.

[stevenjb, 2012-12-14 20:18:58]

This looks much better, thanks.
LGTM

[commit-bot: I haz the power, 2012-12-15 08:42:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11568036/5001

[commit-bot: I haz the power, 2012-12-15 08:42:19]

Presubmit check for 11568036-5001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ash

Presubmit checks took 1.2s to calculate.

[commit-bot: I haz the power, 2012-12-15 09:00:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11568036/5001

[bartfab (slow), 2012-12-15 12:04:52]

Committed as http://crrev.com/173293