Move session length timer into tray bubble.

Move session length timer into tray bubble.

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

[Thiemo Nagel, 2014-05-02 19:15:40]

Hi Steven,

could you please be so kind to take a look a this?

Thank you!
Thiemo

[Thiemo Nagel, 2014-05-02 19:35:22]

Hi Bartosz, Mitsuru, Steven,

(as a more precise update to my previous email) I'd like to kindly ask

stevenjb for review of ash/system/*

oshima for review of ash/resources/*

and bartfab for general comments because he wrote the previous version of the
code.

Thank you!
Thiemo

[oshima, 2014-05-02 19:39:10]

On 2014/05/02 19:35:22, Thiemo Nagel wrote:
> Hi Bartosz, Mitsuru, Steven,
> 
> (as a more precise update to my previous email) I'd like to kindly ask
> 
> stevenjb for review of ash/system/*
> 
> oshima for review of ash/resources/*
> 
> and bartfab for general comments because he wrote the previous version of the
> code.
> 
> Thank you!
> Thiemo

ash/resources lgtm

[stevenjb, 2014-05-02 19:40:01]

https://codereview.chromium.org/262973004/diff/20001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit.cc (right):

https://codereview.chromium.org/262973004/diff/20001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:46:
last_tray_bubble_message_(base::ASCIIToUTF16("")),
No need to initialize strings

https://codereview.chromium.org/262973004/diff/20001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:121:
notification_message_ = base::ASCIIToUTF16("");
.clear();

https://codereview.chromium.org/262973004/diff/20001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:142:
base::TimeDelta::FromMilliseconds(500),
Define and use a well named constant for 500

https://codereview.chromium.org/262973004/diff/20001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:196: kNotificationId /*
id */,
nit: not necessary to comment every param when the type of parameter is clear
(like this one), only when it is not clear (like title, display_source, and
delegate).

https://codereview.chromium.org/262973004/diff/20001/ash/system/tray/system_t...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/262973004/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray.cc:174: AddTrayItem(new
TraySessionLengthLimit(this));
I assume that placing the session length limit tray item below the user item is
an intentional change?

[Thiemo Nagel, 2014-05-02 20:05:19]

Hi Mitsuru and Steven,

great many thanks for your lightening-fast replies!  I've updated the CL to
reflect Steven's suggestions.

Thanks again!
Thiemo

https://codereview.chromium.org/262973004/diff/20001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit.cc (right):

https://codereview.chromium.org/262973004/diff/20001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:46:
last_tray_bubble_message_(base::ASCIIToUTF16("")),
On 2014/05/02 19:40:02, stevenjb wrote:
> No need to initialize strings

Done.

https://codereview.chromium.org/262973004/diff/20001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:121:
notification_message_ = base::ASCIIToUTF16("");
On 2014/05/02 19:40:02, stevenjb wrote:
> .clear();

Done.

https://codereview.chromium.org/262973004/diff/20001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:142:
base::TimeDelta::FromMilliseconds(500),
On 2014/05/02 19:40:02, stevenjb wrote:
> Define and use a well named constant for 500

Done.

https://codereview.chromium.org/262973004/diff/20001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:196: kNotificationId /*
id */,
On 2014/05/02 19:40:02, stevenjb wrote:
> nit: not necessary to comment every param when the type of parameter is clear
> (like this one), only when it is not clear (like title, display_source, and
> delegate).

Done.

https://codereview.chromium.org/262973004/diff/20001/ash/system/tray/system_t...
File ash/system/tray/system_tray.cc (right):

https://codereview.chromium.org/262973004/diff/20001/ash/system/tray/system_t...
ash/system/tray/system_tray.cc:174: AddTrayItem(new
TraySessionLengthLimit(this));
On 2014/05/02 19:40:02, stevenjb wrote:
> I assume that placing the session length limit tray item below the user item
is
> an intentional change?

Yes, it is.

[stevenjb, 2014-05-02 20:43:29]

lgtm

[bartfab (slow), 2014-05-05 13:45:04]

https://codereview.chromium.org/262973004/diff/40001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/262973004/diff/40001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:519: This session will end in <ph
name="session_time_remaining">$1<ex>4 minutes 23 seconds</ex></ph>. You will be
automatically signed out!
Please request a string review by the UI folks. No need to hold the CL but we
should make sure that the strings are reviewed and, if necessary, adjusted
before branch. I do not believe that we tend to end notifications with an
exclamation mark where a full stop would do,

https://codereview.chromium.org/262973004/diff/40001/ash/resources/ash_resour...
File ash/resources/ash_resources.grd (right):

https://codereview.chromium.org/262973004/diff/40001/ash/resources/ash_resour...
ash/resources/ash_resources.grd:156: <structure type="chrome_scaled_image"
name="IDR_AURA_UBER_TRAY_BUBBLE_SESSION_LENGTH_LIMIT"
file="cros/status/status_session_length_timer.png" />
Please remember to pngcrunch the new resources.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit.cc (right):

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:62:
CHECK(tray_bubble_view_ == NULL);
Nit: CHECK(!tray_bubble_view_);

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:119: switch
(limit_state_) {
Nit: There is no need for this switch statement anymore. There are only two
cases now, which correspond to the if statement above. You can fold all this
code into the if statement.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:122:
notification_message_.clear();
Clear the last_ messages as well.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:127:
tray_bubble_message_ = l10n_util::GetStringFUTF16(
Rather than passing information between methods via member variables, which is
akin to globals, why not have this method call two methods such as
UpdateNotification() and UpdateBubbleView(), passing the updated strings to
these?

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:223:
tray_bubble_view_->SetMessage(tray_bubble_message_);
Why is |last_tray_bubble_message_| necessary? Can you not simply always set the
message and call Layout()? Or does this cause flicker?

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit.h (right):

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.h:20: class
TraySessionLengthLimitTest;
Nit: This should not be needed anymore.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.h:54: friend class
test::TraySessionLengthLimitTest;
Nit: This should not be needed anymore.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit_unittest.cc (left):

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit_unittest.cc:82:
EXPECT_FALSE(IsTrayViewVisible());
Is it not possible whether the new bubble view is visible or not? Otherwise, we
are losing test coverage.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit_unittest.cc:170:
UpdateSessionLengthLimitInMin(14);
Does the notifcation disappear now if the limit is shortened? Or why did you
remove this part of the test?

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit_unittest.cc (right):

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit_unittest.cc:110: //
Session length limit is updated to longer. This should not read the
Nit: Update to reflect the updated comment below.

[Thiemo Nagel, 2014-05-05 14:48:40]

Hi Bartosz,

thanks a lot for your thorough review!  I think I've addressed most of your
comments and I'd suggest to move the remainder to a 2nd CL in order to avoid
delay.

Best,
Thiemo

https://codereview.chromium.org/262973004/diff/40001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/262973004/diff/40001/ash/ash_strings.grd#newc...
ash/ash_strings.grd:519: This session will end in <ph
name="session_time_remaining">$1<ex>4 minutes 23 seconds</ex></ph>. You will be
automatically signed out!
On 2014/05/05 13:45:04, bartfab wrote:
> Please request a string review by the UI folks. No need to hold the CL but we
> should make sure that the strings are reviewed and, if necessary, adjusted
> before branch. I do not believe that we tend to end notifications with an
> exclamation mark where a full stop would do,

It's in progress, and yes, that was the first comment I got... :)

https://codereview.chromium.org/262973004/diff/40001/ash/resources/ash_resour...
File ash/resources/ash_resources.grd (right):

https://codereview.chromium.org/262973004/diff/40001/ash/resources/ash_resour...
ash/resources/ash_resources.grd:156: <structure type="chrome_scaled_image"
name="IDR_AURA_UBER_TRAY_BUBBLE_SESSION_LENGTH_LIMIT"
file="cros/status/status_session_length_timer.png" />
On 2014/05/05 13:45:04, bartfab wrote:
> Please remember to pngcrunch the new resources.

It's pngcrush (took me a minute to figure that out), but the latest cry seems to
be tools/resources/optimize-png-files.sh -- that shaved more than 2KB off the
combined initial size of the icons.  I'm impressed.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit.cc (right):

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:62:
CHECK(tray_bubble_view_ == NULL);
On 2014/05/05 13:45:04, bartfab wrote:
> Nit: CHECK(!tray_bubble_view_);

Done.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:119: switch
(limit_state_) {
On 2014/05/05 13:45:04, bartfab wrote:
> Nit: There is no need for this switch statement anymore. There are only two
> cases now, which correspond to the if statement above. You can fold all this
> code into the if statement.

At least for the moment, I'd like to keep the switch to make it easy to have
different wordings for LIMIT_SET vs. LIMIT_EXPIRING_SOON.  But I moved the timer
to a different place to make the switch more succinct.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:122:
notification_message_.clear();
On 2014/05/05 13:45:04, bartfab wrote:
> Clear the last_ messages as well.

Good catch!  Done.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:127:
tray_bubble_message_ = l10n_util::GetStringFUTF16(
On 2014/05/05 13:45:04, bartfab wrote:
> Rather than passing information between methods via member variables, which is
> akin to globals, why not have this method call two methods such as
> UpdateNotification() and UpdateBubbleView(), passing the updated strings to
> these?

I'd like to keep UpdateState() separate because CreateDefaultView() calls it
before deciding whether a tray bubble view needs to be created.  I wouldn't like
that call to update the UI because that creates the impression of recursion
(even if that would not be the case), making the code harder to read.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:223:
tray_bubble_view_->SetMessage(tray_bubble_message_);
On 2014/05/05 13:45:04, bartfab wrote:
> Why is |last_tray_bubble_message_| necessary? Can you not simply always set
the
> message and call Layout()? Or does this cause flicker?

I haven't tested it, but I figure that Layout() is rather expensive and thus
only call it when necessary.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit.h (right):

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.h:20: class
TraySessionLengthLimitTest;
On 2014/05/05 13:45:04, bartfab wrote:
> Nit: This should not be needed anymore.

It's required for kNotificationId.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.h:54: friend class
test::TraySessionLengthLimitTest;
On 2014/05/05 13:45:04, bartfab wrote:
> Nit: This should not be needed anymore.

See above.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit_unittest.cc (left):

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit_unittest.cc:82:
EXPECT_FALSE(IsTrayViewVisible());
On 2014/05/05 13:45:04, bartfab wrote:
> Is it not possible whether the new bubble view is visible or not? Otherwise,
we
> are losing test coverage.

I plan to look at that once the current CL has landed and submit it as a 2nd CL,
possibly together with string updates from the UX team.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit_unittest.cc:170:
UpdateSessionLengthLimitInMin(14);
On 2014/05/05 13:45:04, bartfab wrote:
> Does the notifcation disappear now if the limit is shortened? Or why did you
> remove this part of the test?

No, it doesn't.  It's just that I consider this test obsolete as
TraySessionLengthLimitTest.Notification already includes a change 15min -->
10min.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit_unittest.cc (right):

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit_unittest.cc:110: //
Session length limit is updated to longer. This should not read the
On 2014/05/05 13:45:04, bartfab wrote:
> Nit: Update to reflect the updated comment below.

Done.

[bartfab (slow), 2014-05-05 17:49:57]

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit.cc (right):

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:119: switch
(limit_state_) {
On 2014/05/05 14:48:41, Thiemo Nagel wrote:
> On 2014/05/05 13:45:04, bartfab wrote:
> > Nit: There is no need for this switch statement anymore. There are only two
> > cases now, which correspond to the if statement above. You can fold all this
> > code into the if statement.
> 
> At least for the moment, I'd like to keep the switch to make it easy to have
> different wordings for LIMIT_SET vs. LIMIT_EXPIRING_SOON.  But I moved the
timer
> to a different place to make the switch more succinct.

Do you have any indication that that will be required? It seems strange to keep
extra code and complexity around just in case a string change may be required at
some point.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:127:
tray_bubble_message_ = l10n_util::GetStringFUTF16(
On 2014/05/05 14:48:41, Thiemo Nagel wrote:
> On 2014/05/05 13:45:04, bartfab wrote:
> > Rather than passing information between methods via member variables, which
is
> > akin to globals, why not have this method call two methods such as
> > UpdateNotification() and UpdateBubbleView(), passing the updated strings to
> > these?
> 
> I'd like to keep UpdateState() separate because CreateDefaultView() calls it
> before deciding whether a tray bubble view needs to be created.  I wouldn't
like
> that call to update the UI because that creates the impression of recursion
> (even if that would not be the case), making the code harder to read.

What you would gain are considerable memory savings: Right now, this class keeps
up to 6 strings in memory:

(1) notification_message_;
(2) last_notification_message_;
(3) tray_bubble_message_;
(4) last_tray_bubble_message_;
(5) the notification message held in memory by the notification
(6) the bubble message held in memory by the bubble view

There are two redundant pairs in here: (1) == (5) and (3) == (6). If you called
the UI update methods from UpdateState(), you could avoid this redundant memory
use.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:223:
tray_bubble_view_->SetMessage(tray_bubble_message_);
On 2014/05/05 14:48:41, Thiemo Nagel wrote:
> On 2014/05/05 13:45:04, bartfab wrote:
> > Why is |last_tray_bubble_message_| necessary? Can you not simply always set
> the
> > message and call Layout()? Or does this cause flicker?
> 
> I haven't tested it, but I figure that Layout() is rather expensive and thus
> only call it when necessary.

Looking at the code, Layout() here should be very cheap: LabelTrayView uses a
FillLayout, whose Layout() method is trivial. The Layout() call gets propagated
to the children but nothing much happens there if the bounds have not changed.

The advantage of removing the if statement here is that you could further reduce
memory footprint by not keeping the previous string around.

https://codereview.chromium.org/262973004/diff/90001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit.h (right):

https://codereview.chromium.org/262973004/diff/90001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.h:55: // Update state,
notifications and tray bubble view.  Called by the Timer in
Nit: s/notifications/notification/

[Thiemo Nagel, 2014-05-06 14:27:59]

Hi Bartosz,

thanks for the enlightening discussion!  I hope I've addressed everything now. 
Could you please take another look?

Best,
Thiemo

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit.cc (right):

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:119: switch
(limit_state_) {
> Do you have any indication that that will be required? It seems strange to
keep
> extra code and complexity around just in case a string change may be required
at
> some point.

OK, it's removed.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:127:
tray_bubble_message_ = l10n_util::GetStringFUTF16(
All redundant strings have been removed.

https://codereview.chromium.org/262973004/diff/40001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.cc:223:
tray_bubble_view_->SetMessage(tray_bubble_message_);
Ok, fine, after our offline discussion I'm removing all strings to favour data
size reduction over conserving CPU.

https://codereview.chromium.org/262973004/diff/90001/ash/system/chromeos/sess...
File ash/system/chromeos/session/tray_session_length_limit.h (right):

https://codereview.chromium.org/262973004/diff/90001/ash/system/chromeos/sess...
ash/system/chromeos/session/tray_session_length_limit.h:55: // Update state,
notifications and tray bubble view.  Called by the Timer in
On 2014/05/05 17:49:58, bartfab wrote:
> Nit: s/notifications/notification/

Done.

[bartfab (slow), 2014-05-06 17:57:00]

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
File ash/system/chromeos/session/tray_session_length_limit.cc (right):

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.cc:59: base::string16
TraySessionLengthLimit::ComposeNotificationMessage() const {
Nit 1: Re-arrange methods so that their implementation order matches the
declaration order.

Nit 2: Instead of these two methods being members that implicitly get the
remaining time from a member variable, you could just make them helpers in an
anonymous namespace, e.g.:

namespace {

base::string16 ComposeNotificationMessage(
    base::TimeDelta remaining_session_time) {
  [...]
}

}  // namespace

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.cc:103: void
TraySessionLengthLimit::OnViewClicked(views::View* sender) {
What is the point of being a ViewClickListener if you do nothing on click?

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.cc:134: limit_state_ =
LIMIT_NONE;
Nit: Reset last_limit_state_.

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
File ash/system/chromeos/session/tray_session_length_limit.h (right):

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.h:55: // Update state,
notification and tray bubble view.  Called by the Timer in
Nit: Why is Timer capitalized? It sounds like a reference to a class name but
you are not using any class of that name.

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.h:73: LimitState
limit_state_;       // Current state.
Nit: Style guide says: Two spaces before //, do not try to align comments on
different lines.

[Thiemo Nagel, 2014-05-06 18:49:06]

Hi Bartosz,

thanks a lot for your comments!  I do only partially agree -- but for that part
enthusiastically.

Best,
Thiemo

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
File ash/system/chromeos/session/tray_session_length_limit.cc (right):

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.cc:59: base::string16
TraySessionLengthLimit::ComposeNotificationMessage() const {
On 2014/05/06 17:57:00, bartfab wrote:
> Nit 1: Re-arrange methods so that their implementation order matches the
> declaration order.

Done.

> Nit 2: Instead of these two methods being members that implicitly get the
> remaining time from a member variable, you could just make them helpers in an
> anonymous namespace, e.g.:

I don't see a point in this.  In my perspective, the purpose of a class is to
tie together data and the related functionality that accesses this data.  The
two Compose* methods adhere to this paradigm, they are private so that they
cannot be accessed from the outside world and they are const to indicate that
they don't have side-effects.  What would be the advantage of moving them
outside the class?

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.cc:103: void
TraySessionLengthLimit::OnViewClicked(views::View* sender) {
On 2014/05/06 17:57:00, bartfab wrote:
> What is the point of being a ViewClickListener if you do nothing on click?

Mindless cut&paste.  Fixed.

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.cc:134: limit_state_ =
LIMIT_NONE;
On 2014/05/06 17:57:00, bartfab wrote:
> Nit: Reset last_limit_state_.

This must happen in UpdateNotification(), otherwise a potentially existing
notification is not deleted when transitioning to LIMIT_NONE.

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
File ash/system/chromeos/session/tray_session_length_limit.h (right):

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.h:55: // Update state,
notification and tray bubble view.  Called by the Timer in
On 2014/05/06 17:57:00, bartfab wrote:
> Nit: Why is Timer capitalized? It sounds like a reference to a class name but
> you are not using any class of that name.

Thanks, you're right.  I'm using a RepeatingTimer.  Changed that.

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.h:73: LimitState
limit_state_;       // Current state.
On 2014/05/06 17:57:00, bartfab wrote:
> Nit: Style guide says: Two spaces before //, do not try to align comments on
> different lines.

Where does it say that?

There is ample evidence of aligned comments in the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...

[bartfab (slow), 2014-05-07 12:30:52]

lgtm

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
File ash/system/chromeos/session/tray_session_length_limit.h (right):

https://codereview.chromium.org/262973004/diff/130001/ash/system/chromeos/ses...
ash/system/chromeos/session/tray_session_length_limit.h:73: LimitState
limit_state_;       // Current state.
On 2014/05/06 18:49:06, Thiemo Nagel wrote:
> On 2014/05/06 17:57:00, bartfab wrote:
> > Nit: Style guide says: Two spaces before //, do not try to align comments on
> > different lines.
> 
> Where does it say that?
> 
> There is ample evidence of aligned comments in the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...

I think you are right. I read the JavaScript style guide recently and I think I
remembered it from there. Sorry for the confusion.

[Thiemo Nagel, 2014-05-07 15:22:19]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-05-07 15:26:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tnagel@chromium.org/262973004/200013

[commit-bot: I haz the power, 2014-05-07 19:52:35]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 22:33:18]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 23:04:08]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium

[commit-bot: I haz the power, 2014-05-08 00:34:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-08 00:34:02]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium

[Thiemo Nagel, 2014-05-08 04:25:29]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-05-08 04:28:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tnagel@chromium.org/262973004/200013

[commit-bot: I haz the power, 2014-05-08 05:25:57]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium

[Thiemo Nagel, 2014-05-08 21:51:39]

The CQ bit was unchecked by tnagel@chromium.org

[Thiemo Nagel, 2014-05-12 14:33:51]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-05-12 14:34:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tnagel@chromium.org/262973004/220001

[commit-bot: I haz the power, 2014-05-12 15:18:18]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-12 18:09:10]

Change committed as 269813