Prevents session_length_timeout reappearing when it's already dismissed.

Prevents session_length_timeout reappearing when it's already dismissed.

BUG=285941
R=stevenjb@chromium.org, bartfab@chromium.org
TEST=manually

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

[stevenjb, 2013-09-09 17:58:03]

https://codereview.chromium.org/23464044/diff/1/ash/system/session_length_lim...
File ash/system/session_length_limit/tray_session_length_limit.cc (right):

https://codereview.chromium.org/23464044/diff/1/ash/system/session_length_lim...
ash/system/session_length_limit/tray_session_length_limit.cc:84: // which should
not create a new one if it's already closed.
nit: s/one/notification

https://codereview.chromium.org/23464044/diff/1/ash/system/session_length_lim...
ash/system/session_length_limit/tray_session_length_limit.cc:86:
!message_center->HasNotification(kSessionLengthTimeoutNotificationId)) {
This seems awkward; we're effectively using 'enable_spoken_feedback' as
'create'. I think it would be more clear if this logic were moved to Update().

[Jun Mukai, 2013-09-09 19:37:25]

https://codereview.chromium.org/23464044/diff/1/ash/system/session_length_lim...
File ash/system/session_length_limit/tray_session_length_limit.cc (right):

https://codereview.chromium.org/23464044/diff/1/ash/system/session_length_lim...
ash/system/session_length_limit/tray_session_length_limit.cc:84: // which should
not create a new one if it's already closed.
On 2013/09/09 17:58:03, stevenjb (chromium) wrote:
> nit: s/one/notification

Done.

https://codereview.chromium.org/23464044/diff/1/ash/system/session_length_lim...
ash/system/session_length_limit/tray_session_length_limit.cc:86:
!message_center->HasNotification(kSessionLengthTimeoutNotificationId)) {
On 2013/09/09 17:58:03, stevenjb (chromium) wrote:
> This seems awkward; we're effectively using 'enable_spoken_feedback' as
> 'create'. I think it would be more clear if this logic were moved to Update().

I don't think I need to separate the function / logic here, instead rename the
|enable_spoken_feedback| flag to another name, |state_changed|.

[stevenjb, 2013-09-09 19:43:07]

lgtm

https://codereview.chromium.org/23464044/diff/1/ash/system/session_length_lim...
File ash/system/session_length_limit/tray_session_length_limit.cc (right):

https://codereview.chromium.org/23464044/diff/1/ash/system/session_length_lim...
ash/system/session_length_limit/tray_session_length_limit.cc:86:
!message_center->HasNotification(kSessionLengthTimeoutNotificationId)) {
On 2013/09/09 19:37:25, Jun Mukai wrote:
> On 2013/09/09 17:58:03, stevenjb (chromium) wrote:
> > This seems awkward; we're effectively using 'enable_spoken_feedback' as
> > 'create'. I think it would be more clear if this logic were moved to
Update().
> 
> I don't think I need to separate the function / logic here, instead rename the
> |enable_spoken_feedback| flag to another name, |state_changed|.

OK, that's more clear now.

[bartfab (slow), 2013-09-09 20:36:30]

lgtm

https://codereview.chromium.org/23464044/diff/6001/ash/system/session_length_...
File ash/system/session_length_limit/tray_session_length_limit.cc (right):

https://codereview.chromium.org/23464044/diff/6001/ash/system/session_length_...
ash/system/session_length_limit/tray_session_length_limit.cc:80: //
|remaining_time|.  |state_changed| is true when it's internal state has been
Nit: s/it's/its/

[Jun Mukai, 2013-09-09 20:44:25]

https://codereview.chromium.org/23464044/diff/6001/ash/system/session_length_...
File ash/system/session_length_limit/tray_session_length_limit.cc (right):

https://codereview.chromium.org/23464044/diff/6001/ash/system/session_length_...
ash/system/session_length_limit/tray_session_length_limit.cc:80: //
|remaining_time|.  |state_changed| is true when it's internal state has been
On 2013/09/09 20:36:30, bartfab wrote:
> Nit: s/it's/its/

Done.

[commit-bot: I haz the power, 2013-09-09 20:45:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/23464044/12001

[commit-bot: I haz the power, 2013-09-10 08:36:26]

Change committed as 222212