Added quick lock mechanism while in Touchview

- Moved screenshot shortcut handling from MaximizeModeController to PowerButtonController
- Added a command line switch to enable quick locking mode.
- Refactored SessionStateAnimator into an interface, implementation, and a test double so that all the LockStateControllerTests could be re-enabled.
- Re-enabled LockStateControllerTests.

BUG=371608, 167048, 162645
Committed: https://crrev.com/ca2e1496db635fb826949d54c72798fd56f7fcfd
Cr-Commit-Position: refs/heads/master@{#294696}

[flackr, 2014-06-10 17:34:42]

The general concept seems fine to me. We might need to make this only when
maximize mode is engaged though and somehow specific to extra power buttons,
rather than the standard ones on the ChromeOS keyboards.

https://codereview.chromium.org/326813004/diff/1/ash/wm/power_button_controll...
File ash/wm/power_button_controller.cc (right):

https://codereview.chromium.org/326813004/diff/1/ash/wm/power_button_controll...
ash/wm/power_button_controller.cc:91:
controller_->StartLockAnimationAndLockImmediately();
Won't this affect the regular power button on a chromebook keyboard?

[bruthig, 2014-06-17 14:00:11]

I've reverted the immediate lock on power button press to maintain the
lock/shutdown behaviour.  flackr@ can you please take another look so I can
submit this patch as a small step towards implementing an easy/quick way to lock
the screen while in touchview mode.

Added derat@ as an fyi.

[flackr, 2014-06-17 14:09:09]

I'm not sure we want this change if it doesn't get us the easy lock behavior. On
the bug, you weren't sure that we could ever tell the power button apart which
suggests we need some board specific change. It may be that powerd should not
send the power button event on that board when the volume key is held at the
same time?

The take screenshot on volume down + power was a hack to allow taking a
screenshot in touchview and doesn't really make sense on devices with functional
keyboards. Also, we probably would only want to do it for side mounted volume
and power buttons.

https://codereview.chromium.org/326813004/diff/20001/ash/wm/power_button_cont...
File ash/wm/power_button_controller.h (right):

https://codereview.chromium.org/326813004/diff/20001/ash/wm/power_button_cont...
ash/wm/power_button_controller.h:43: , public ui::DisplayConfigurator::Observer
nit: indent 2 more

[Daniel Erat, 2014-06-17 14:14:29]

huh, i didn't even know that this shortcut existed. and powerd doesn't have
anything to do with X, so i'm not sure about the feasibility of making it ignore
the power button if volume-down is held (unless chrome notified it about each
volume-down press and release, which seems a bit overly-complicated for
this...).

[flackr, 2014-06-17 14:19:37]

On 2014/06/17 14:14:29, Daniel Erat wrote:
> huh, i didn't even know that this shortcut existed.

Only while touchview / maximized mode is engaged:
https://codereview.chromium.org/263813002/

> and powerd doesn't have
> anything to do with X, so i'm not sure about the feasibility of making it
ignore
> the power button if volume-down is held (unless chrome notified it about each
> volume-down press and release, which seems a bit overly-complicated for
> this...).

The problem in the bug is that we wanted to lock the screen immediately when the
side power button was pressed to make this action easier to use, but other than
knowing that on certain devices the power button is side mounted there's no
difference in the keycode from the power button on the main keyboard of most
devices..

[flackr, 2014-06-17 14:19:38]

On 2014/06/17 14:14:29, Daniel Erat wrote:
> huh, i didn't even know that this shortcut existed.

Only while touchview / maximized mode is engaged:
https://codereview.chromium.org/263813002/

> and powerd doesn't have
> anything to do with X, so i'm not sure about the feasibility of making it
ignore
> the power button if volume-down is held (unless chrome notified it about each
> volume-down press and release, which seems a bit overly-complicated for
> this...).

The problem in the bug is that we wanted to lock the screen immediately when the
side power button was pressed to make this action easier to use, but other than
knowing that on certain devices the power button is side mounted there's no
difference in the keycode from the power button on the main keyboard of most
devices..

[Daniel Erat, 2014-06-17 14:30:05]

On 2014/06/17 14:19:38, flackr wrote:
> On 2014/06/17 14:14:29, Daniel Erat wrote:
> > huh, i didn't even know that this shortcut existed.
> 
> Only while touchview / maximized mode is engaged:
> https://codereview.chromium.org/263813002/
> 
> > and powerd doesn't have
> > anything to do with X, so i'm not sure about the feasibility of making it
> ignore
> > the power button if volume-down is held (unless chrome notified it about
each
> > volume-down press and release, which seems a bit overly-complicated for
> > this...).
> 
> The problem in the bug is that we wanted to lock the screen immediately when
the
> side power button was pressed to make this action easier to use, but other
than
> knowing that on certain devices the power button is side mounted there's no
> difference in the keycode from the power button on the main keyboard of most
> devices..

the keycode doesn't matter here, does it? i believe that chrome still uses
powerd's reports of power button events (sent over d-bus) rather than watching
for X events. powerd uses /dev/input to listen for power button and lid-switch
events; it seems possible that different power buttons could be distinguished
there (e.g. powerd already ignores ACPI power buttons if there's a
chrome-os-style power key instead). the powerd code is at
src/platform/power_manager/powerd/system/input.cc in a chrome os checkout.

[bruthig, 2014-08-29 14:44:33]

flackr@, derat@, I've updated the implementation and I've written some potential
tests, however there are issues with them currently. Please see my comments in
lock_state_controller_unittest.cc to discuss.

https://codereview.chromium.org/326813004/diff/20001/ash/wm/power_button_cont...
File ash/wm/power_button_controller.h (right):

https://codereview.chromium.org/326813004/diff/20001/ash/wm/power_button_cont...
ash/wm/power_button_controller.h:43: , public ui::DisplayConfigurator::Observer
On 2014/06/17 14:09:09, flackr wrote:
> nit: indent 2 more

Done.

https://codereview.chromium.org/326813004/diff/80001/ash/wm/lock_state_contro...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/80001/ash/wm/lock_state_contro...
ash/wm/lock_state_controller_unittest.cc:89: // no leaks happen during tests.
In order for the new QuickLock tests below to work in this test fixture they
require there to be an animation duration, however according to this comment it
appears that memory leaks occur if they do.

What I would like to propose is that change I PowerButtonController to accept a
pure virtual LockStateController 'interface' and rename the existing
LockStateController implementation to LockStateControllerImpl (or something like
that).  This would allow me to test the PowerButtonController logic in isolation
by passing in a simple LockStateController test stub.

Thoughts?

[Daniel Erat, 2014-08-29 14:58:26]

https://codereview.chromium.org/326813004/diff/80001/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/326813004/diff/80001/ash/ash_switches.h#newco...
ash/ash_switches.h:33: ASH_EXPORT extern const char kAshEnablePowerQuickLock[];
maybe either kAshEnablePowerButtonQuickLock or just kAshEnableQuickLock? having
"Power" in there by itself seems a bit strange

https://codereview.chromium.org/326813004/diff/80001/ash/wm/lock_state_contro...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/80001/ash/wm/lock_state_contro...
ash/wm/lock_state_controller_unittest.cc:89: // no leaks happen during tests.
On 2014/08/29 14:44:33, bruthig wrote:
> In order for the new QuickLock tests below to work in this test fixture they
> require there to be an animation duration, however according to this comment
it
> appears that memory leaks occur if they do.
> 
> What I would like to propose is that change I PowerButtonController to accept
a
> pure virtual LockStateController 'interface' and rename the existing
> LockStateController implementation to LockStateControllerImpl (or something
like
> that).  This would allow me to test the PowerButtonController logic in
isolation
> by passing in a simple LockStateController test stub.
> 
> Thoughts?

i am frustrated by how long all of the tests in this file have been disabled
(and by all of the commented-out code) and am highly supportive of any changes
that would result in them being enabled. (or am i misunderstanding your
suggestion? could the existing tests also be turned back on?)

https://codereview.chromium.org/326813004/diff/80001/ash/wm/power_button_cont...
File ash/wm/power_button_controller.cc (right):

https://codereview.chromium.org/326813004/diff/80001/ash/wm/power_button_cont...
ash/wm/power_button_controller.cc:33: switches::kAshEnablePowerQuickLock)),
nit: just indent this four spaces beyond the previous line

[bruthig, 2014-08-29 15:41:47]

Continued discussion on LockStateControllerTests...

https://codereview.chromium.org/326813004/diff/80001/ash/wm/lock_state_contro...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/80001/ash/wm/lock_state_contro...
ash/wm/lock_state_controller_unittest.cc:89: // no leaks happen during tests.
On 2014/08/29 14:58:26, Daniel Erat wrote:
> On 2014/08/29 14:44:33, bruthig wrote:
> > In order for the new QuickLock tests below to work in this test fixture they
> > require there to be an animation duration, however according to this comment
> it
> > appears that memory leaks occur if they do.
> > 
> > What I would like to propose is that change I PowerButtonController to
accept
> a
> > pure virtual LockStateController 'interface' and rename the existing
> > LockStateController implementation to LockStateControllerImpl (or something
> like
> > that).  This would allow me to test the PowerButtonController logic in
> isolation
> > by passing in a simple LockStateController test stub.
> > 
> > Thoughts?
> 
> i am frustrated by how long all of the tests in this file have been disabled
> (and by all of the commented-out code) and am highly supportive of any changes
> that would result in them being enabled. (or am i misunderstanding your
> suggestion? could the existing tests also be turned back on?)

I actually spent quite a bit of time looking in to why these tests were disabled
and what it would take to re-enable them.  The problem is that these
LockStateControllerTests are actually exercising and verifying animation layers
and other stuff in the ui/compositor space, which in my opinion is much to large
a scope to verify alongside the triggers of when to perform such animations. 
And the best that I can tell, there is not a good way to to control the timing
of the animations from the test and they can only be set to run at some multiple
of real clock time, possibly even a zero multiple.  At the bottom of it all the
ui/compositor layers are dependent on a static Time/TimeTicks Now() function
which currently cannot be well controlled from within a test space.  The work to
be able to inject a mock time object is tracked here: www.crbug.com/166469 .

Now even if it were possible to to control the clock time from a
LockStateController test I am of the opinion that the test would still be
exercising way to much code outside of the ash space and we should somehow
create a better test seam somewhere between ash and the ui/compositor.  Perhaps
a SessionStateAnimator or ui::LayerAnimator test double.

So to answer your question, no, my suggestion would not allow me to easily
re-enable all the other tests in this file.  But it would allow me to actually
write tests to somewhat verify my current change.  My suggestion would also have
me move the QuickLock tests to a power_button_controller_test.cc file.

[bruthig, 2014-09-02 13:53:48]

Just minor fixes

https://codereview.chromium.org/326813004/diff/80001/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/326813004/diff/80001/ash/ash_switches.h#newco...
ash/ash_switches.h:33: ASH_EXPORT extern const char kAshEnablePowerQuickLock[];
On 2014/08/29 14:58:25, Daniel Erat wrote:
> maybe either kAshEnablePowerButtonQuickLock or just kAshEnableQuickLock?
having
> "Power" in there by itself seems a bit strange

Done.

https://codereview.chromium.org/326813004/diff/80001/ash/wm/power_button_cont...
File ash/wm/power_button_controller.cc (right):

https://codereview.chromium.org/326813004/diff/80001/ash/wm/power_button_cont...
ash/wm/power_button_controller.cc:33: switches::kAshEnablePowerQuickLock)),
On 2014/08/29 14:58:26, Daniel Erat wrote:
> nit: just indent this four spaces beyond the previous line

Done.

[Daniel Erat, 2014-09-02 14:09:48]

looks fine to me. were you also planning to include the proposed refactoring
that'll let you add tests for this?

[flackr, 2014-09-03 12:48:29]

Looks good. Just a few comments.

https://codereview.chromium.org/326813004/diff/100001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/100001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:91: //    
ui::ScopedAnimationDurationScaleMode::ZERO_DURATION));
nit: Delete code if we no longer want it, though I'm guessing this is because
you're working on the tests?

https://codereview.chromium.org/326813004/diff/100001/ash/wm/power_button_con...
File ash/wm/power_button_controller.cc (right):

https://codereview.chromium.org/326813004/diff/100001/ash/wm/power_button_con...
ash/wm/power_button_controller.cc:66:
Shell::GetInstance()->accelerator_controller()->PerformAction(
Can you limit this to be only when in maximize mode?

https://codereview.chromium.org/326813004/diff/100001/ash/wm/power_button_con...
File ash/wm/power_button_controller.h (right):

https://codereview.chromium.org/326813004/diff/100001/ash/wm/power_button_con...
ash/wm/power_button_controller.h:97: bool enable_quick_lock_;
I think we should just query the commandline when we need to know this.

For tests you can append switches in the SetUp like so:
https://code.google.com/p/chromium/codesearch#chromium/src/ash/root_window_co...

[bruthig, 2014-09-08 15:33:20]

I've written a SessionStateAnimator test double and re-enabled most
LockStateControllerTests.  I still have two concerns and I'm looking for input.

derat@ can you please take a look?

https://codereview.chromium.org/326813004/diff/100001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/100001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:91: //    
ui::ScopedAnimationDurationScaleMode::ZERO_DURATION));
On 2014/09/03 12:48:28, flackr wrote:
> nit: Delete code if we no longer want it, though I'm guessing this is because
> you're working on the tests?

Done.

https://codereview.chromium.org/326813004/diff/100001/ash/wm/power_button_con...
File ash/wm/power_button_controller.cc (right):

https://codereview.chromium.org/326813004/diff/100001/ash/wm/power_button_con...
ash/wm/power_button_controller.cc:66:
Shell::GetInstance()->accelerator_controller()->PerformAction(
On 2014/09/03 12:48:29, flackr wrote:
> Can you limit this to be only when in maximize mode?

Done.

https://codereview.chromium.org/326813004/diff/100001/ash/wm/power_button_con...
File ash/wm/power_button_controller.h (right):

https://codereview.chromium.org/326813004/diff/100001/ash/wm/power_button_con...
ash/wm/power_button_controller.h:97: bool enable_quick_lock_;
On 2014/09/03 12:48:29, flackr wrote:
> I think we should just query the commandline when we need to know this.
> 
> For tests you can append switches in the SetUp like so:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/root_window_co...

Rob and I discussed offline and decided to leave this as is because this is not
a temporary command line switch that is going to go away.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:626: // TODO(bruthig): Before check-in.
This causes the following warning
This test expects a crash due to exercising a CHECK(false), I've only found the
EXPECT_DEATH macro to be able to handle this but I'm getting the warning in the
code comment.  Is this warning ok, or what should I remove the test, or ... ?

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
File ash/wm/session_state_animator_impl_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl_unittest.cc:1: // TODO(bruthig): Add some
tests for the SessionStateAnimatorImpl.
Tests for the integration of the SessionStateAnimator & ui/compositor should be
written at some point.  I'm not sure if this needs to be tracked or if this is
the best way to track that... Thoughts?

[Daniel Erat, 2014-09-08 16:16:03]

awesome, thanks for doing this! my comments are mostly just nits.

you can also update the BUG= line to include 167048, since i think this will
(finally) fix that issue.

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
File ash/test/test_session_state_animator.cc (right):

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:10: #include "base/time/time.h"
nit: move this to the header since you use time structs there

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:18: void DummyCallback() {}
i could swear that something like this already declared somewhere, but i'm
entirely unable to find it. sorry, this is a useless comment :-/

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:118: for (ActiveAnimationsMap::iterator
container_itr = active_animations_.begin();
nit: i've seen both "it" or "iter" used commonly in chromium for iterators, but
not "itr"

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:261: ActiveAnimation active_animation =
{
nit: use the c'tor here (since you defined it)?

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
File ash/test/test_session_state_animator.h (right):

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.h:38: // animations that will have
completed after |duration|  will have it's
nit: remove extra space before "will"; s/it's/its/

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.h:43: // contained AnimationSequence will
have OnAnimationCompleted if
s/have/run/?

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.h:122: virtual void
StartAnimationInSequence(int container_mask,
nit: move "int container_mask" to the next line, indented four spaces (all
arguments should be horizontally aligned in function signatures if the signature
doesn't fit on a single line)

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.h:148: // A time counter that tracks the
last shecudled animation or animation
nit: s/shecudled/scheduled/

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller.h (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller.h:149: void
SetDelegate(LockStateControllerDelegate* delegate);
nit: mind making the argument be a scoped_ptr so the ownership transfer is
self-documenting?

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller_unittest.cc (left):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:140: //TODO (antrim) : restore
EXPECT_TRUE(animator_helper_->IsAnimating());
nit: should this keep a EXPECT_EQ(0u, test_animator_->GetAnimationCount())?

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:240: //TODO (antrim) : restore
EXPECT_TRUE(animator_helper_->IsAnimating());
nit: should this keep a EXPECT_EQ(0u, test_animator_->GetAnimationCount())?

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:815:
AdvancePartially(SessionStateAnimator::ANIMATION_SPEED_UNDOABLE, 0.5f);
did these constants just rot due to the test being disabled for so long?

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:626: // TODO(bruthig): Before check-in.
This causes the following warning
On 2014/09/08 15:33:20, bruthig wrote:
> This test expects a crash due to exercising a CHECK(false), I've only found
the
> EXPECT_DEATH macro to be able to handle this but I'm getting the warning in
the
> code comment.  Is this warning ok, or what should I remove the test, or ... ?

i think the warning is okay, but the above "We should restore the windows"
comment needs to be updated

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:916: // HideBackground();
what's the story with these comments? we shouldn't have commented-out code
checked in, and this test looks like it's still disabled.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:1003: EXPECT_EQ(0,
delegate->handle_take_screenshot_count());
this doesn't seem to match the above comment. why is the count still 0 here?

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
File ash/wm/session_state_animator.h (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator.h:91: ROOT_CONTAINER = 1 << 6,
nit: add a comment describing this mask

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator.h:111: //     
session_state_animator->BegineAnimationSequence(some_callback);
nit: s/Begine/Begin/

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
File ash/wm/session_state_animator_impl.cc (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:430: SessionStateAnimatorImpl* animator_;
nit: document that the animator isn't owned by this object

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:441:
SessionStateAnimatorImpl::AnimationSequence::AnimationSequence(
nit: these are all short; consider inlining them in the class definition above
to make the code shorter and easier to read (inlining is fine in .cc files)

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:560: void
SessionStateAnimatorImpl::StartAnimation(int container_mask,
nit: move "int container_mask" to the next line and indent four spaces, maybe
(see next comment)

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:561: SessionStateAnimator::AnimationType
type,
hmm, is the SessionStateAnimator:: needed here even though that's a base class?
if so, these should be indented four spaces instead of two; if not, you can
probably leave them aligned with the opening paren

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:587:
SessionStateAnimatorImpl::BeginAnimationSequence(base::Closure callback) {
nit: indent four spaces instead of two

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
File ash/wm/session_state_animator_impl_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl_unittest.cc:1: // TODO(bruthig): Add some
tests for the SessionStateAnimatorImpl.
On 2014/09/08 15:33:20, bruthig wrote:
> Tests for the integration of the SessionStateAnimator & ui/compositor should
be
> written at some point.  I'm not sure if this needs to be tracked or if this is
> the best way to track that... Thoughts?

it'd be better to file a bug instead of checking in an empty file

[bruthig, 2014-09-09 17:32:35]

derat@, I think I addressed all comments, please have another look.

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
File ash/test/test_session_state_animator.cc (right):

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:10: #include "base/time/time.h"
On 2014/09/08 16:16:01, Daniel Erat wrote:
> nit: move this to the header since you use time structs there

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:118: for (ActiveAnimationsMap::iterator
container_itr = active_animations_.begin();
On 2014/09/08 16:16:01, Daniel Erat wrote:
> nit: i've seen both "it" or "iter" used commonly in chromium for iterators,
but
> not "itr"

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:261: ActiveAnimation active_animation =
{
On 2014/09/08 16:16:01, Daniel Erat wrote:
> nit: use the c'tor here (since you defined it)?

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
File ash/test/test_session_state_animator.h (right):

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.h:38: // animations that will have
completed after |duration|  will have it's
On 2014/09/08 16:16:02, Daniel Erat wrote:
> nit: remove extra space before "will"; s/it's/its/

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.h:43: // contained AnimationSequence will
have OnAnimationCompleted if
On 2014/09/08 16:16:01, Daniel Erat wrote:
> s/have/run/?

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.h:122: virtual void
StartAnimationInSequence(int container_mask,
On 2014/09/08 16:16:02, Daniel Erat wrote:
> nit: move "int container_mask" to the next line, indented four spaces (all
> arguments should be horizontally aligned in function signatures if the
signature
> doesn't fit on a single line)

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/test/test_session_s...
ash/test/test_session_state_animator.h:148: // A time counter that tracks the
last shecudled animation or animation
On 2014/09/08 16:16:02, Daniel Erat wrote:
> nit: s/shecudled/scheduled/

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller.h (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller.h:149: void
SetDelegate(LockStateControllerDelegate* delegate);
On 2014/09/08 16:16:02, Daniel Erat wrote:
> nit: mind making the argument be a scoped_ptr so the ownership transfer is
> self-documenting?

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller_unittest.cc (left):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:140: //TODO (antrim) : restore
EXPECT_TRUE(animator_helper_->IsAnimating());
On 2014/09/08 16:16:02, Daniel Erat wrote:
> nit: should this keep a EXPECT_EQ(0u, test_animator_->GetAnimationCount())?

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:240: //TODO (antrim) : restore
EXPECT_TRUE(animator_helper_->IsAnimating());
On 2014/09/08 16:16:02, Daniel Erat wrote:
> nit: should this keep a EXPECT_EQ(0u, test_animator_->GetAnimationCount())?

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:815:
AdvancePartially(SessionStateAnimator::ANIMATION_SPEED_UNDOABLE, 0.5f);
On 2014/09/08 16:16:02, Daniel Erat wrote:
> did these constants just rot due to the test being disabled for so long?

0.5f might be fine, but the reason I changed it is because AdvancePartially(...,
0.5f) was called multiple times and I just wanted to make sure that the sum of
all calls in this tests method were less than 1.0f just in case something weird
is happening in the background and animations weren't being removed properly by
the sut.
I can change it back if you'd like.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:626: // TODO(bruthig): Before check-in.
This causes the following warning
On 2014/09/08 16:16:02, Daniel Erat wrote:
> On 2014/09/08 15:33:20, bruthig wrote:
> > This test expects a crash due to exercising a CHECK(false), I've only found
> the
> > EXPECT_DEATH macro to be able to handle this but I'm getting the warning in
> the
> > code comment.  Is this warning ok, or what should I remove the test, or ...
?
> 
> i think the warning is okay, but the above "We should restore the windows"
> comment needs to be updated

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:916: // HideBackground();
On 2014/09/08 16:16:02, Daniel Erat wrote:
> what's the story with these comments? we shouldn't have commented-out code
> checked in, and this test looks like it's still disabled.

Originally I was thinking these tests would need support from the ui/compositor
but I just removed any ui/compositor stuff from the LockStateController and put
it in to the SessionStateAnimator.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:1003: EXPECT_EQ(0,
delegate->handle_take_screenshot_count());
On 2014/09/08 16:16:02, Daniel Erat wrote:
> this doesn't seem to match the above comment. why is the count still 0 here?

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
File ash/wm/session_state_animator.h (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator.h:91: ROOT_CONTAINER = 1 << 6,
On 2014/09/08 16:16:02, Daniel Erat wrote:
> nit: add a comment describing this mask

Added, if you can think of a better description let me know.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator.h:111: //     
session_state_animator->BegineAnimationSequence(some_callback);
On 2014/09/08 16:16:02, Daniel Erat wrote:
> nit: s/Begine/Begin/

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
File ash/wm/session_state_animator_impl.cc (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:430: SessionStateAnimatorImpl* animator_;
On 2014/09/08 16:16:03, Daniel Erat wrote:
> nit: document that the animator isn't owned by this object

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:441:
SessionStateAnimatorImpl::AnimationSequence::AnimationSequence(
On 2014/09/08 16:16:03, Daniel Erat wrote:
> nit: these are all short; consider inlining them in the class definition above
> to make the code shorter and easier to read (inlining is fine in .cc files)

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:560: void
SessionStateAnimatorImpl::StartAnimation(int container_mask,
On 2014/09/08 16:16:03, Daniel Erat wrote:
> nit: move "int container_mask" to the next line and indent four spaces, maybe
> (see next comment)

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:561: SessionStateAnimator::AnimationType
type,
On 2014/09/08 16:16:03, Daniel Erat wrote:
> hmm, is the SessionStateAnimator:: needed here even though that's a base
class?
> if so, these should be indented four spaces instead of two; if not, you can
> probably leave them aligned with the opening paren

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:587:
SessionStateAnimatorImpl::BeginAnimationSequence(base::Closure callback) {
On 2014/09/08 16:16:03, Daniel Erat wrote:
> nit: indent four spaces instead of two

Done.

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
File ash/wm/session_state_animator_impl_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/140001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl_unittest.cc:1: // TODO(bruthig): Add some
tests for the SessionStateAnimatorImpl.
On 2014/09/08 16:16:03, Daniel Erat wrote:
> On 2014/09/08 15:33:20, bruthig wrote:
> > Tests for the integration of the SessionStateAnimator & ui/compositor should
> be
> > written at some point.  I'm not sure if this needs to be tracked or if this
is
> > the best way to track that... Thoughts?
> 
> it'd be better to file a bug instead of checking in an empty file

Ok, I will also add BUG=162645, it seems to already be tracking these missing
tests.  I will also add a comment after check-in to have record of the current
state.

[Daniel Erat, 2014-09-09 17:42:09]

lgtm

https://codereview.chromium.org/326813004/diff/160001/ash/test/test_session_s...
File ash/test/test_session_state_animator.h (right):

https://codereview.chromium.org/326813004/diff/160001/ash/test/test_session_s...
ash/test/test_session_state_animator.h:39: // animations that will have
completed after |duration| will have it's
nit: s/it's/its/ (former is contraction of "it" and "is"; latter is possessive)

https://codereview.chromium.org/326813004/diff/160001/ash/wm/session_state_an...
File ash/wm/session_state_animator_impl.cc (right):

https://codereview.chromium.org/326813004/diff/160001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:435: if (sequences_completed_ ==
sequences_attached_) {
nit: omit curly brackets here since the statement fits on a single line

https://codereview.chromium.org/326813004/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/power/power_button_observer.cc (right):

https://codereview.chromium.org/326813004/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/power/power_button_observer.cc:38: new
SessionStateControllerDelegateChromeos).Pass());
nit: i could be wrong, but i think you don't need Pass() here since this is a
temporary

[bruthig, 2014-09-09 17:55:03]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2014-09-09 17:55:22]

Minor nit fixes, no need to review.

https://codereview.chromium.org/326813004/diff/160001/ash/test/test_session_s...
File ash/test/test_session_state_animator.h (right):

https://codereview.chromium.org/326813004/diff/160001/ash/test/test_session_s...
ash/test/test_session_state_animator.h:39: // animations that will have
completed after |duration| will have it's
On 2014/09/09 17:42:09, Daniel Erat wrote:
> nit: s/it's/its/ (former is contraction of "it" and "is"; latter is
possessive)

Done.

https://codereview.chromium.org/326813004/diff/160001/ash/wm/session_state_an...
File ash/wm/session_state_animator_impl.cc (right):

https://codereview.chromium.org/326813004/diff/160001/ash/wm/session_state_an...
ash/wm/session_state_animator_impl.cc:435: if (sequences_completed_ ==
sequences_attached_) {
On 2014/09/09 17:42:09, Daniel Erat wrote:
> nit: omit curly brackets here since the statement fits on a single line

Done.

https://codereview.chromium.org/326813004/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/power/power_button_observer.cc (right):

https://codereview.chromium.org/326813004/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/power/power_button_observer.cc:38: new
SessionStateControllerDelegateChromeos).Pass());
On 2014/09/09 17:42:09, Daniel Erat wrote:
> nit: i could be wrong, but i think you don't need Pass() here since this is a
> temporary

Done.

[commit-bot: I haz the power, 2014-09-09 18:01:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bruthig@chromium.org/326813004/180001

[bruthig, 2014-09-09 18:43:58]

The CQ bit was unchecked by bruthig@chromium.org

[flackr, 2014-09-10 19:38:38]

lgtm too

https://codereview.chromium.org/326813004/diff/180001/ash/test/test_session_s...
File ash/test/test_session_state_animator.cc (right):

https://codereview.chromium.org/326813004/diff/180001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:22:
SessionStateAnimator::DESKTOP_BACKGROUND,
nit: I believe this should be an indent of 4 (from the previous line)

https://codereview.chromium.org/326813004/diff/180001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller.cc (right):

https://codereview.chromium.org/326813004/diff/180001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller.cc:77: delegate_.reset(delegate.release());
nit: Should be delegate_ = delegate.Pass(); to ensure pointer ownership at all
times.

[bruthig, 2014-09-10 21:07:59]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2014-09-10 21:12:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bruthig@chromium.org/326813004/200001

[bruthig, 2014-09-10 21:12:35]

The CQ bit was unchecked by bruthig@chromium.org

[bruthig, 2014-09-10 21:12:38]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2014-09-10 21:15:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bruthig@chromium.org/326813004/200001

[bruthig, 2014-09-10 21:16:06]

The CQ bit was unchecked by bruthig@chromium.org

[bruthig, 2014-09-12 15:30:07]

Minor nit fixes and disabled LockStateControllerTest.Lock on OS_WIN.  derat@,
are ok with this?

https://codereview.chromium.org/326813004/diff/180001/ash/test/test_session_s...
File ash/test/test_session_state_animator.cc (right):

https://codereview.chromium.org/326813004/diff/180001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:22:
SessionStateAnimator::DESKTOP_BACKGROUND,
On 2014/09/10 19:38:37, flackr wrote:
> nit: I believe this should be an indent of 4 (from the previous line)

Done.

https://codereview.chromium.org/326813004/diff/180001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller.cc (right):

https://codereview.chromium.org/326813004/diff/180001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller.cc:77: delegate_.reset(delegate.release());
On 2014/09/10 19:38:37, flackr wrote:
> nit: Should be delegate_ = delegate.Pass(); to ensure pointer ownership at all
> times.

Done.

https://codereview.chromium.org/326813004/diff/220001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/220001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:665:
EXPECT_DEATH(test_api_->trigger_lock_fail_timeout(), "");
I've spent 2 days trying to figure out why EXPECT_DEATH(...) is hanging and not
able to tell that the process actually crashed but to no avail.  Considering I
was only re-enabling this test and have not changed the production code path I
would like to get this in.

[bruthig, 2014-09-12 15:36:19]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2014-09-12 15:40:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/326813004/220001

[Daniel Erat, 2014-09-12 15:50:15]

lgtm

https://codereview.chromium.org/326813004/diff/220001/ash/test/test_session_s...
File ash/test/test_session_state_animator.cc (right):

https://codereview.chromium.org/326813004/diff/220001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:29: };
nit: i'm not sure what the rules are here, but i think that flackr meant:

const ...
    TestSession... = {
    SessionStateAnimator::...,
    SessionStateAnimator::...,
    ...
};

i have no strong feelings between what you originally had or the above, though
-- i'm not sure what the rules are here. the six-space indent that's there right
now is probably wrong, though.

here's the kinda-crazy thing that clang-format recommends:

const SessionStateAnimator::Container
    TestSessionStateAnimator::kAllContainers[] = {
        SessionStateAnimator::DESKTOP_BACKGROUND,
        SessionStateAnimator::LAUNCHER,
        SessionStateAnimator::NON_LOCK_SCREEN_CONTAINERS,
        SessionStateAnimator::LOCK_SCREEN_BACKGROUND,
        SessionStateAnimator::LOCK_SCREEN_CONTAINERS,
        SessionStateAnimator::LOCK_SCREEN_RELATED_CONTAINERS,
        SessionStateAnimator::ROOT_CONTAINER};

(don't let any of this hold up the change, though)

https://codereview.chromium.org/326813004/diff/220001/ash/wm/lock_state_contr...
File ash/wm/lock_state_controller_unittest.cc (right):

https://codereview.chromium.org/326813004/diff/220001/ash/wm/lock_state_contr...
ash/wm/lock_state_controller_unittest.cc:665:
EXPECT_DEATH(test_api_->trigger_lock_fail_timeout(), "");
On 2014/09/12 15:30:07, bruthig wrote:
> I've spent 2 days trying to figure out why EXPECT_DEATH(...) is hanging and
not
> able to tell that the process actually crashed but to no avail.  Considering I
> was only re-enabling this test and have not changed the production code path I
> would like to get this in.

seems fine to me. i'm not sure that the screen-lock code path even runs on
windows -- presumably we don't show the webui lock screen there. this timeout is
just there to keep the system from sitting around in an unlocked state if the
lock screen never gets displayed or chrome fails to talk to session_manager (a
chrome-os-specific daemon).

[bruthig, 2014-09-12 16:00:57]

The CQ bit was unchecked by bruthig@chromium.org

[bruthig, 2014-09-12 16:01:52]

Minor fix, no need to review.

https://codereview.chromium.org/326813004/diff/220001/ash/test/test_session_s...
File ash/test/test_session_state_animator.cc (right):

https://codereview.chromium.org/326813004/diff/220001/ash/test/test_session_s...
ash/test/test_session_state_animator.cc:29: };
On 2014/09/12 15:50:14, Daniel Erat wrote:
> nit: i'm not sure what the rules are here, but i think that flackr meant:
> 
> const ...
>     TestSession... = {
>     SessionStateAnimator::...,
>     SessionStateAnimator::...,
>     ...
> };
> 
> i have no strong feelings between what you originally had or the above, though
> -- i'm not sure what the rules are here. the six-space indent that's there
right
> now is probably wrong, though.
> 
> here's the kinda-crazy thing that clang-format recommends:
> 
> const SessionStateAnimator::Container
>     TestSessionStateAnimator::kAllContainers[] = {
>         SessionStateAnimator::DESKTOP_BACKGROUND,
>         SessionStateAnimator::LAUNCHER,
>         SessionStateAnimator::NON_LOCK_SCREEN_CONTAINERS,
>         SessionStateAnimator::LOCK_SCREEN_BACKGROUND,
>         SessionStateAnimator::LOCK_SCREEN_CONTAINERS,
>         SessionStateAnimator::LOCK_SCREEN_RELATED_CONTAINERS,
>         SessionStateAnimator::ROOT_CONTAINER};
> 
> (don't let any of this hold up the change, though)

Whoops, that's what I meant to do, not sure how I goofed.

[bruthig, 2014-09-12 16:01:57]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2014-09-12 16:04:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/326813004/240001

[commit-bot: I haz the power, 2014-09-12 17:02:55]

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

[commit-bot: I haz the power, 2014-09-12 17:02:56]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bruthig, 2014-09-12 17:13:06]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2014-09-12 17:14:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/326813004/260001

[bruthig, 2014-09-12 18:12:13]

The CQ bit was unchecked by bruthig@chromium.org

[bruthig, 2014-09-12 18:27:44]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2014-09-12 18:29:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/326813004/280001

[commit-bot: I haz the power, 2014-09-12 20:34:28]

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

[commit-bot: I haz the power, 2014-09-12 20:34:30]

Exceeded time limit waiting for builds to trigger.

[bruthig, 2014-09-12 22:45:53]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2014-09-12 22:53:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/326813004/280001

[commit-bot: I haz the power, 2014-09-12 23:52:26]

Committed patchset #15 (id:280001) as 3dd78428b8ef81a6ec93b81370e8d5bb3b94cca5

[commit-bot: I haz the power, 2014-09-12 23:56:05]

Patchset 15 (id:??) landed as
https://crrev.com/ca2e1496db635fb826949d54c72798fd56f7fcfd
Cr-Commit-Position: refs/heads/master@{#294696}