athena: Add a minimized state for the home-card.

athena: Add a minimized state for the home-card.

The home-card can be in a minimized state. In this state, only a narrow line
is displayed at the bottom of the screen, and dragging it up shows the overview
mode. When a window is selected in the overview mode, the home-card is minimized
again.

BUG=381224
R=mukai@chromium.org, oshima@chromium.org

Previously landed in r283539, reverted in r283564 because it broke tests.

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

[Jun Mukai, 2014-07-16 05:53:58]

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:260: view->ShowWhenReady();
Please remove line 258-260.
ShowWhenReady() seems to reschedule showing if it's not ready (due to icon load
or something).  Keeping this is okay right now but might cause weird effect in
the future.

It seems like InitAsBubbleAtFixedLocation() creates Widget, so it's okay to just
remove these lines for now. I'll deal with icon loading issue when necessary.

https://codereview.chromium.org/394043002/diff/40001/athena/wm/DEPS
File athena/wm/DEPS (right):

https://codereview.chromium.org/394043002/diff/40001/athena/wm/DEPS#newcode2
athena/wm/DEPS:2: "+athena/home/public",
I think it's better to avoid the mutual dependencies among athena directories.

The usage in athena/wm is really simple. Can we:
- add some observer, like OverviewModeObserver
- Let HomeCardImpl to set its status by itself

[oshima, 2014-07-16 16:24:31]

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:37: virtual int GetHorizontalMargin() const = 0;
How about returning preferred size?

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:81: }
can we just pass the window of the home card instead?

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:95: // TODO(mukai): stops using the bubble and
remove this.
Can't we just disable bubble's shadow using AppListView::set_shadow?

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:208: if (state == VISIBLE_MINIMIZED) {
would you mind using switch instead?

https://codereview.chromium.org/394043002/diff/40001/athena/wm/DEPS
File athena/wm/DEPS (right):

https://codereview.chromium.org/394043002/diff/40001/athena/wm/DEPS#newcode2
athena/wm/DEPS:2: "+athena/home/public",
On 2014/07/16 05:53:58, Jun Mukai wrote:
> I think it's better to avoid the mutual dependencies among athena directories.
> 
> The usage in athena/wm is really simple. Can we:
> - add some observer, like OverviewModeObserver
> - Let HomeCardImpl to set its status by itself

Yep, no circular dependency. Let's have a
WindowManagerObserver::OnOverviewModeChanged.

[sadrul, 2014-07-16 16:58:27]

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:37: virtual int GetHorizontalMargin() const = 0;
On 2014/07/16 16:24:31, oshima wrote:
> How about returning preferred size?

I considered doing that, but that would require the delegate to have to query
the screen-size and apply the margin (if applicable) to decide the width. It
feels better to let the layout-manager take care of that.

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:81: }
On 2014/07/16 16:24:31, oshima wrote:
> can we just pass the window of the home card instead?

Done (introduced HomeCard::GetNativeWindow()).

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:95: // TODO(mukai): stops using the bubble and
remove this.
On 2014/07/16 16:24:31, oshima wrote:
> Can't we just disable bubble's shadow using AppListView::set_shadow?

I tried that, but it looks like we still need this piece of code to make the
bubble bottom-aligned with the screen. mukai@ would know more.

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:208: if (state == VISIBLE_MINIMIZED) {
On 2014/07/16 16:24:31, oshima wrote:
> would you mind using switch instead?

Done.

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:260: view->ShowWhenReady();
On 2014/07/16 05:53:58, Jun Mukai wrote:
> Please remove line 258-260.
> ShowWhenReady() seems to reschedule showing if it's not ready (due to icon
load
> or something).  Keeping this is okay right now but might cause weird effect in
> the future.
> 
> It seems like InitAsBubbleAtFixedLocation() creates Widget, so it's okay to
just
> remove these lines for now. I'll deal with icon loading issue when necessary.

Done.

https://codereview.chromium.org/394043002/diff/40001/athena/wm/DEPS
File athena/wm/DEPS (right):

https://codereview.chromium.org/394043002/diff/40001/athena/wm/DEPS#newcode2
athena/wm/DEPS:2: "+athena/home/public",
On 2014/07/16 16:24:31, oshima wrote:
> On 2014/07/16 05:53:58, Jun Mukai wrote:
> > I think it's better to avoid the mutual dependencies among athena
directories.
> > 
> > The usage in athena/wm is really simple. Can we:
> > - add some observer, like OverviewModeObserver
> > - Let HomeCardImpl to set its status by itself
> 
> Yep, no circular dependency. Let's have a
> WindowManagerObserver::OnOverviewModeChanged.

Good idea. Thanks! Done. In keeping with the ash style, added
OnOverviewModeEnter and OnOverviewModeExit

[Jun Mukai, 2014-07-16 18:31:56]

lgtm

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:95: // TODO(mukai): stops using the bubble and
remove this.
On 2014/07/16 16:58:26, sadrul wrote:
> On 2014/07/16 16:24:31, oshima wrote:
> > Can't we just disable bubble's shadow using AppListView::set_shadow?
> 
> I tried that, but it looks like we still need this piece of code to make the
> bubble bottom-aligned with the screen. mukai@ would know more.

That is right, unfortunately it's hard to disable shadows of bubbles.
Eventually I'll remove the dependency on bubble and let athena homecard be a
widget.  Please keep this until then.

[oshima, 2014-07-16 19:10:38]

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/394043002/diff/40001/athena/home/home_card_im...
athena/home/home_card_impl.cc:37: virtual int GetHorizontalMargin() const = 0;
On 2014/07/16 16:58:26, sadrul wrote:
> On 2014/07/16 16:24:31, oshima wrote:
> > How about returning preferred size?
> 
> I considered doing that, but that would require the delegate to have to query
> the screen-size and apply the margin (if applicable) to decide the width. It
> feels better to let the layout-manager take care of that.

OK, i wont block CL, but asking home card horizontal margin is also a bit weird.
We probably can revisit this later when we have better window management.

https://codereview.chromium.org/394043002/diff/50001/athena/home/home_card_im...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/394043002/diff/50001/athena/home/home_card_im...
athena/home/home_card_impl.cc:99: aura::Window* container_;
I believe you can remove this.

https://codereview.chromium.org/394043002/diff/50001/athena/home/public/home_...
File athena/home/public/home_card.h (right):

https://codereview.chromium.org/394043002/diff/50001/athena/home/public/home_...
athena/home/public/home_card.h:66: virtual aura::Window* GetNativeWindow() = 0;
Do you need this here? Can this be in Delegate instead?

[sadrul, 2014-07-16 19:32:59]

https://codereview.chromium.org/394043002/diff/50001/athena/home/home_card_im...
File athena/home/home_card_impl.cc (right):

https://codereview.chromium.org/394043002/diff/50001/athena/home/home_card_im...
athena/home/home_card_impl.cc:99: aura::Window* container_;
On 2014/07/16 19:10:38, oshima wrote:
> I believe you can remove this.

Yep. cool. done.

https://codereview.chromium.org/394043002/diff/50001/athena/home/public/home_...
File athena/home/public/home_card.h (right):

https://codereview.chromium.org/394043002/diff/50001/athena/home/public/home_...
athena/home/public/home_card.h:66: virtual aura::Window* GetNativeWindow() = 0;
On 2014/07/16 19:10:38, oshima wrote:
> Do you need this here? Can this be in Delegate instead?

Yes! I should've just done that. Done.

[oshima, 2014-07-16 20:16:19]

lgtm

[sadrul, 2014-07-16 22:13:05]

Committed patchset #6 manually as r283539 (presubmit successful).

[kuan, 2014-07-16 23:04:46]

A revert of this CL has been created in
https://codereview.chromium.org/391383004/ by kuan@chromium.org.

The reason for reverting is: it broke "athena_unittests,steps" on "Linux
ChromiumOS Tests (1)":
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

[sadrul, 2014-07-17 17:41:38]

Committed patchset #8 manually as r283804 (presubmit successful).