Visual improvements to text filtering in Ash overview mode

Visual improvements to text filtering in Ash overview mode

The following visual improvements are made by this CL:

* The textfield is given a black background with an
  alpha of 70% and rounded corners.
* The length of the textfield takes up 25% of the
  available length instead of 50%.
* Padding surrounds the text within the textfield.
* Sufficient space is reserved at the top of the screen
  to guarantee that the textfield will never overlap
  anything (i.e., selection widget in the top row
  of items).
* The textfield fades in/out in addition to sliding
  up/down.
* There is now space between the top of the screen
  and the textfield.

BUG=394871
TEST=WindowSelectorTest.BasicArrowKeyNavigation modified

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

[tdanderson, 2014-07-17 19:45:27]

Rob, can you PTAL?

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:61: const unsigned char kTextFilterOpacity =
128;
Admittedly a copy of WindowGrid::kWindowOverviewSelectorOpacity. Where would you
like to define shared constants such as this one?

[tdanderson, 2014-07-17 19:50:37]

P.S. See the bug for a screenshot of what this looks like currently.

[flackr, 2014-07-18 13:41:51]

Overall looks good, just a few comments.

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_grid.cc
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:109: const int kTextFilterSpacing = 5;
nit: How about something like kTextFilterBottomMargin which is a bit more
descriptive.

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:61: const unsigned char kTextFilterOpacity =
128;
On 2014/07/17 19:45:26, tdanderson wrote:
> Admittedly a copy of WindowGrid::kWindowOverviewSelectorOpacity. Where would
you
> like to define shared constants such as this one?

As a static class var (like kTransitionMilliseconds in
scoped_transform_overview_window.h) probably on WindowSelector.

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:233: int WindowSelector::TextFilterHeight()
const {
Can this be in anonymous namespace and use a static var which computes the text
height on the first call to it? We use this pattern for many one time
calculations in other ui classes.

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:480: aura::Window* root_window) {
Seems like this could still be in the anonymous namespace, I guess it was moved
out for TextFilterHeight, but that can also be in the anonymous namespace since
it's not unique per WindowSelector.

[tdanderson, 2014-08-07 20:13:07]

Hi Rob, can you please take another look? Sebastien is in the process of
reviewing the corresponding screenshots, which you can see at
https://code.google.com/p/chromium/issues/detail?id=395159.

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_grid.cc
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:109: const int kTextFilterSpacing = 5;
On 2014/07/18 13:41:51, flackr wrote:
> nit: How about something like kTextFilterBottomMargin which is a bit more
> descriptive.

Done.

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:233: int WindowSelector::TextFilterHeight()
const {
On 2014/07/18 13:41:51, flackr wrote:
> Can this be in anonymous namespace and use a static var which computes the
text
> height on the first call to it? We use this pattern for many one time
> calculations in other ui classes.

This is gone in the next patch set.

https://codereview.chromium.org/403493006/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:480: aura::Window* root_window) {
On 2014/07/18 13:41:51, flackr wrote:
> Seems like this could still be in the anonymous namespace, I guess it was
moved
> out for TextFilterHeight, but that can also be in the anonymous namespace
since
> it's not unique per WindowSelector.

Done.

[tdanderson, 2014-08-08 17:33:52]

Hi Rob, Patch Set 3 has design approval and fixes a failing test, so it's ready
for formal review now.

[flackr, 2014-08-08 17:43:59]

LGTM with nits

https://codereview.chromium.org/403493006/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/403493006/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:120: corner_radius_[i] = corner_radius;
nit: As long as these are all the same, we should just have a single int
corner_radius_;. It will be easy to change if we want to have different corners
later.

https://codereview.chromium.org/403493006/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/403493006/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:902: // is not being used).
Could you extend this to still have 3 rows (with 1 item in the last row) just to
verify the vertical up/down is fully tested?

[tdanderson, 2014-08-08 19:25:44]

https://codereview.chromium.org/403493006/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/403493006/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:120: corner_radius_[i] = corner_radius;
On 2014/08/08 17:43:59, flackr wrote:
> nit: As long as these are all the same, we should just have a single int
> corner_radius_;. It will be easy to change if we want to have different
corners
> later.

Done.

https://codereview.chromium.org/403493006/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/403493006/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:902: // is not being used).
On 2014/08/08 17:43:59, flackr wrote:
> Could you extend this to still have 3 rows (with 1 item in the last row) just
to
> verify the vertical up/down is fully tested?

Done.

[tdanderson, 2014-08-08 19:25:49]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-08-08 19:27:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/403493006/60001

[tdanderson, 2014-08-08 21:20:33]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-08-08 21:23:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/403493006/80001

[tdanderson, 2014-08-08 21:34:03]

Rob, note that in patch set 5 I made a small change to the arrow key nav test by
increasing the screen dimensions. Given the reserved space at the top, the
height of the windows were computed as 60. This means that after applying the
kWindowMargin inset (30*2) we were left with windows of height 0 and therefore
triggered the DCHECK(!bounds.IsEmpty()) in
ScopedTransformOverviewWindow::ShrinkRectToFitPreservingAspectRatio().

In the future we may want to consider either making kWindowMargin variable or
making use of it when computing the number of rows and columns. What do you
think?

[tdanderson, 2014-08-08 23:24:43]

The CQ bit was unchecked by tdanderson@chromium.org

[tdanderson, 2014-08-08 23:24:44]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-08-08 23:25:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/403493006/80001

[commit-bot: I haz the power, 2014-08-09 08:53:02]

Change committed as 288546