Refactor PhantomWindowController so that the PhantomWindowController owns the phantom window

This is part 1 of implemeting phantom windows for the window controls when using --ash-enable-alternate-caption-button

Other notable changes:
- Switched to using layer animations instead of gfx::SlideAnimation
- Renamed PhantomWindowController::phantom_widget_ and PhantomWindowController::phantom_widget_start_ to PhantomWindowController::phantom_widget_in_target_root_ and PhantomWindowController::phantom_widget_in_start_root_ respectively.
- Removed PhantomWindowController::Hide(). Hiding the phantom window can now only be done via destroying the PhantomWindowController

BUG=328930
TEST=PhantomWindowControllerTest.*

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

[pkotwicz, 2013-12-16 23:30:02]

Stefan, can you please take a look?

[Mr4D (OOO till 08-26), 2013-12-17 22:55:20]

Except for naming lgtm.

https://codereview.chromium.org/101773004/diff/10008/ash/wm/workspace/phantom...
File ash/wm/workspace/phantom_window_controller.h (right):

https://codereview.chromium.org/101773004/diff/10008/ash/wm/workspace/phantom...
ash/wm/workspace/phantom_window_controller.h:88: // NULL. This allows animation
to progress from one display into the other.
Somehow the name is ... odd. What about naming one target_phantom_widget and the
other start_phantom_widget?

[pkotwicz, 2013-12-18 19:16:46]

Stefan, can you please take another look? I fixed a couple of bugs I found in my
code and refactored the tests

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
File ash/wm/workspace/phantom_window_controller.cc (right):

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.cc:90: widget->Hide();
Changed so that Hide() gets called before setting the opacity. Otherwise,
VisibilityController makes the call to Hide() a no-op

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.cc:171:
!target_phantom_widget_->GetNativeView()->layer()->IsDrawn()) {
I made the phantom window start animating from |window_|'s bounds if Show() is
called when it is fully hidden. I have added a test at
PhantomWindowControllerTest.PhantomWindowShowAfterHide to test this.

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.cc:180: start_bounds =
GetBoundsWithShadow(window_->GetBoundsInScreen());
Fixed when GetBoundsWithShadow() is called. In the previous CL we were getting
incorrect bounds here

[pkotwicz, 2013-12-19 16:10:48]

Stefan, ping?

[Mr4D (OOO till 08-26), 2013-12-19 17:56:54]

A few comments.

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
File ash/wm/workspace/phantom_window_controller.cc (right):

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.cc:90: widget->Hide();
I just checked with sky and all you would need to do is to set the scoped layer
animation settings and then call show or hide and the animation should be done
by itself. No need for the hand called layer->SetOpacity. (unless I am missing
something)

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
File ash/wm/workspace/phantom_window_controller_unittest.cc (right):

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller_unittest.cc:74: 
Just wondering: Why did you get rid of the animation completion? Because you
don't need the workspace-resizer anymore I suppose?

After all these changes - does the old phantom window still work as it used to?
If so can we remove the old unit tests before it goes away?

[pkotwicz, 2013-12-19 18:18:21]

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
File ash/wm/workspace/phantom_window_controller.cc (right):

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.cc:90: widget->Hide();
I am unsure if I understand. I need to call both ui::Layer::SetOpacity() and
ui::Layer::Show()/Hide()

What Scott is probably referring to is that I could use
views::corewm::SetWindowVisibilityAnimationType() and things would just work. I
opted not to do this because kChildWindowVisibilityChangesAnimatedKey is false
for kShellWindowId_ShelfContainer.

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
File ash/wm/workspace/phantom_window_controller_unittest.cc (right):

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller_unittest.cc:74: 
I got rid of the animation completion because I am not using
gfx::SlideAnimation. ash::test::AshTestBase sets
zero_duration_mode_.reset(new ui::ScopedAnimationDurationScaleMode(
      ui::ScopedAnimationDurationScaleMode::ZERO_DURATION));
which ensures that animations complete instantaneously.

After all these changes - the old phantom window should work as it used to. I am
unsure if I understand your comment about removing the old unit tests

[Mr4D (OOO till 08-26), 2013-12-19 18:57:52]

The unit tests are a refactor and still test the legacy case. As such that is
fine. lgtm

[pkotwicz, 2013-12-19 19:03:38]

Scott, can you please take a look?

[sky, 2013-12-19 21:25:42]

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
File ash/wm/workspace/phantom_window_controller.cc (right):

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.cc:65:
ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
Why do you want strategy?

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.cc:73: void
AnimateToVisibility(views::Widget* widget, bool new_visibility) {
Seems like this is the same as an AnimationType of
WINDOW_VISIBILITY_ANIMATION_TYPE_FADE. Is there a reason you can't use it?

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
File ash/wm/workspace/phantom_window_controller.h (right):

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.h:43: void StartFadeOut();
Instead make Hide() take an argument indicating if it should animate.

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.h:63: views::Widget*
CreatePhantomWidget(
return scoped_ptr?

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.h:91: scoped_ptr<views::Widget>
start_phantom_widget_;
start|target aren't particular descriptive by themselves, maybe the more verbose
phantom_widget_in_target_root_ and phantom_widget_in_start_root_.

[pkotwicz, 2013-12-20 01:57:19]

Scott, can you please take another look?

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
File ash/wm/workspace/phantom_window_controller.cc (right):

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.cc:65:
ui::LayerAnimator::IMMEDIATELY_ANIMATE_TO_NEW_TARGET);
I think that IMMEDIATELY_ANIMATE_TO_NEW_TARGET should be the default preemption
strategy. (It is not)

Using IMMEDIATELY_ANIMATE_TO_NEW_TARGET is equivalent to what the old
SlideAnimation code was doing. It also results in a slightly nicer animation
when a user is dragging a window to the edge of the screen quickly and
--ash-enable-docked-windows is set.

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.cc:73: void
AnimateToVisibility(views::Widget* widget, bool new_visibility) {
kChildWindowVisibilityChangesAnimatedKey is false on
kShellWindowId_ShelfContainer.
It is unclear whether the value should be changed.

I am also unsure as to whether the UX designers will want to change the
animations once they actually play with the new phantom windows. (They have only
seen videos thus far)

[sky, 2014-01-06 17:30:05]

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
File ash/wm/workspace/phantom_window_controller.cc (right):

https://codereview.chromium.org/101773004/diff/130001/ash/wm/workspace/phanto...
ash/wm/workspace/phantom_window_controller.cc:73: void
AnimateToVisibility(views::Widget* widget, bool new_visibility) {
On 2013/12/20 01:57:20, pkotwicz wrote:
> kChildWindowVisibilityChangesAnimatedKey is false on
> kShellWindowId_ShelfContainer.
> It is unclear whether the value should be changed.
> 
> I am also unsure as to whether the UX designers will want to change the
> animations once they actually play with the new phantom windows. (They have
only
> seen videos thus far)

Can't you temporarily enable animations when you change the visibility?

[pkotwicz, 2014-01-09 04:54:09]

Scott, can you please take a look?

After some discussion with the UX designers it turns out we want some pretty
different animations than what I originally implemented. This CL is now a
refactor only (and is still useful for part 2)

I did not temporarily enable kChildWindowVisibilityChangesAnimatedKey for the
phantom window's parent container. We currently do not have any getters for
kChildWindowVisibilityChangesAnimatedKey or a way of setting
kChildWindowVisibilityChangesAnimatedKey back to false once it has been set to
true. I think that a bigger (related) deficiency in our code base is that we
have 3 different ways of temporarily disabling animations.
You also have plenty of other opportunities to bug me about animations as well.
(e.g. https://codereview.chromium.org/108303011/)

[sky, 2014-01-09 16:41:17]

LGTM

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/101773004/250001

[commit-bot: I haz the power, 2014-01-09 21:04:56]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-09 21:33:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/101773004/250001

[commit-bot: I haz the power, 2014-01-10 02:33:58]

Change committed as 244050