[ash-md] Fixed ShelfBackgroundAnimator to not snap target values when being preempted.

[ash-md] Fixed ShelfBackgroundAnimator to not snap target values when being preempted.

Snapping to the target values was causing the shelf to flicker when dragging a
window while in OVERLAP mode. And on some platforms this contributed to the
shelf incorrectly being painted as DEFAULT mode.

BUG=695901
Review-Url: https://codereview.chromium.org/2749123003
Cr-Commit-Position: refs/heads/master@{#456892}
Committed: https://chromium.googlesource.com/chromium/src/+/27dd2542a15215360c4f4fc8eeca9d72780d5167

[bruthig, 2017-03-14 21:17:24]

The CQ bit was checked by bruthig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 21:18:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bruthig, 2017-03-14 22:00:47]

The CQ bit was checked by bruthig@chromium.org to run a CQ dry run

[bruthig, 2017-03-14 22:01:04]

bruthig@chromium.org changed reviewers:
+ msw@chromium.org

[bruthig, 2017-03-14 22:01:05]

msw@, can you take a quick look?

[commit-bot: I haz the power, 2017-03-14 22:01:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2017-03-14 22:29:10]

msw@chromium.org changed reviewers:
+ jamescook@chromium.org

[msw, 2017-03-14 22:29:11]

Please bear with my questions; I've looping in James, who may be more familiar
with this specific class.

https://codereview.chromium.org/2749123003/diff/20001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_background_animator.cc (left):

https://codereview.chromium.org/2749123003/diff/20001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:101: void
ShelfBackgroundAnimator::AnimationCanceled(
I don't quite understand how this fixes the issue in the CL. Why is a shelf
background animation started and cancelled mid-drag, when the window never stops
overlapping the shelf?

What was the original purpose of this code? What would preempt this animation
such that we'd ever want to snap to the end state? We should make sure this
doesn't introduce another regression (ie. that this really isn't needed).

[bruthig, 2017-03-14 22:50:00]

No prob on the questions.  I've tried to answer them, PTAL.

https://codereview.chromium.org/2749123003/diff/20001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_background_animator.cc (left):

https://codereview.chromium.org/2749123003/diff/20001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_background_animator.cc:101: void
ShelfBackgroundAnimator::AnimationCanceled(
On 2017/03/14 22:29:11, msw wrote:
> I don't quite understand how this fixes the issue in the CL. Why is a shelf
> background animation started and cancelled mid-drag, when the window never
stops
> overlapping the shelf?

You're right, there is another confounding factor here.  When a window is being
dragged it is moved from the kShellWindowId_DefaultContainer to the
kShellWindowId_DockedContainer (for reasons I'm not really sure, but I think it
has to do with z-order of the dragged window relative to the docked windows).

When this re-parenting happens ShelfLayoutManager::UpdateVisibilityState() is
called and WorkspaceController::GetWindowState() incorrectly reports that the
shelf is in WORKSPACE_WINDOW_STATE_DEFAULT mode because for a period of time the
dragged window is not in either container.

So the net effect was the background was starting to animate to the DEFAULT
state, getting preempted immediately and causing the shelf to snap to the
DEFAULT state before ending in the correct OVERLAP state.

> 
> What was the original purpose of this code? What would preempt this animation
> such that we'd ever want to snap to the end state? We should make sure this
> doesn't introduce another regression (ie. that this really isn't needed).

This code never should have been included in the original change, it was an
oversight on my part.

A valid preemption will occur if a user quickly drags a window to overlap and
then to not-overlap we don't want the shelf to snap to the DEFAULT (aka
not-overlapped) state, we want it to smoothly animate back and forth.

[msw, 2017-03-14 23:06:58]

That seems pretty reasonable, thanks. lgtm

[bruthig, 2017-03-14 23:12:46]

The CQ bit was unchecked by bruthig@chromium.org

[bruthig, 2017-03-14 23:12:47]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2017-03-14 23:13:28]

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

[James Cook, 2017-03-14 23:13:57]

Sounds like you guys worked it out. rubber stamp lgtm

[commit-bot: I haz the power, 2017-03-14 23:36:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489533167184590,
"parent_rev": "f54e53e1f9c7e353276485af8d733f367a1bf7b2", "commit_rev":
"27dd2542a15215360c4f4fc8eeca9d72780d5167"}

[commit-bot: I haz the power, 2017-03-14 23:38:00]

Description was changed from

==========
[ash-md] Fixed ShelfBackgroundAnimator to not snap target values when being
preempted.

Snapping to the target values was causing the shelf to flicker when dragging a
window while in OVERLAP mode. And on some platforms this contributed to the
shelf incorrectly being painted as DEFAULT mode.

BUG=695901
==========

to

==========
[ash-md] Fixed ShelfBackgroundAnimator to not snap target values when being
preempted.

Snapping to the target values was causing the shelf to flicker when dragging a
window while in OVERLAP mode. And on some platforms this contributed to the
shelf incorrectly being painted as DEFAULT mode.

BUG=695901

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

[commit-bot: I haz the power, 2017-03-14 23:38:01]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/27dd2542a15215360c4f4fc8eeca...