Added text filtering to Overview Mode

Added text filtering to Overview Mode

This patch adds a text widget to the window overview in Ash that allows the user
to filter the windows, making it easier to select among multiple active items.

Windows that do not match the pattern are set to 0.5 transparency and their
selection is prevented. The pretarget event handler in WindowSelector has been
removed in favour of handling the key events in the widget, which produces
cleaner code.

BUG=388726
TEST=WindowSelectorTest.BasicTextFiltering
TBR=oshima@chromium.org

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

[Nina, 2014-06-25 13:58:54]

Terry,

Can you give this patch an initial review? I'm missing tests for now, but I
first want to get sure the structure of the code is not going to change too
much.

Cheers!

[tdanderson, 2014-06-25 15:48:23]

lgtm with the comments addressed below. Send over to Rob when ready.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/scoped_trans...
File ash/wm/overview/scoped_transform_overview_window.h (right):

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/scoped_trans...
ash/wm/overview/scoped_transform_overview_window.h:115: // The original opacity
before entering overview mode.
opactity of the window

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:309: // letters to lowercase in a l10n sensitive
context.
I would instead mention the fact that your filtering is accent- and
case-insensitive in the method documentation for Filter() (in the header).

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:332: // shows up.
"shows up on the first unfiltered item"?

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.h:71: void Filter(const base::string16& pattern);
I'd prefer a slightly more descriptive name such as "FilterItems"

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.h:131: // Indicates if there is at least one non
filtered item in the overview.
I notice that you are using four different terms in your comments, method names,
and variable names to all mean essentially the same thing: dimmed, filtered,
active, and visible. I suggest using the same one everywhere for consistency.
"Visible" is a bit misleading since the items are still visible (just
transparent), and "active" is overloaded. So I'd go with either "dimmed" or
"filtered".

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.h:132: bool item_active_;
I would re-name to be more descriptive, e.g., something like has_active_item_.
The name as-is suggests that there is only one "item" and it is either active or
not.

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:92: views::Widget*
CreateTextFilter(views::TextfieldController* controller,
Add some documentation on line 91 for this method.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:264: return true;
Should this be return false?

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:320: if (ignore_activations_ ||
!gained_active ||
Use three lines.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:365: iter != grid_list_.end(); iter++) {
Align iter with ScopedVector.

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.h:95: 
Remove blank line

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.h:97: const ui::KeyEvent& key_event);
OVERRIDE

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.h:158: // Window text filter widget.
Explain in a bit more detail what this is / what it's used for (since we own
this member, it should be our responsibility to give this explanation).

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_panels.cc:204: iter != transform_windows_.end();
iter++) {
Align iter with WindowList.

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_window.cc:75: float opacity = visible ? 1 : 0.5;
Local variable not needed.

https://codereview.chromium.org/358553004/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/358553004/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield.cc:437: SchedulePaint();
It should be the responsibility of the caller to invoke SchedulePaint() if
needed; we should not be doing it inside a mutator.

https://codereview.chromium.org/358553004/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield.cc:1601:
shadows.push_back(gfx::ShadowValue(gfx::Point(0, 1), 10, SK_ColorBLACK));
I wonder if it's worthwhile defining constants for these arguments (as well as
any other hard-coded values we have in the overview mode code such as 1 for
"unfiltered opacity" and 0.5 as "filtered opacity". Then all of the values
controlling the look and feel would be in one place.

It might also be worth making a helper to avoid repetition of the logic: active
? 1 : 0.5.

https://codereview.chromium.org/358553004/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/358553004/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield.h:124: void DisplayShadow(bool
display_shadow);
If the instance variable is has_shadow_, this should be called SetHasShadow().

[Nina, 2014-06-25 16:02:22]

Flackr, want to give this patch a first review? Still haven't looked at Terry's
nits, though.

[Nina, 2014-06-26 15:20:18]

OK, I've got the first set of comments fixed. Flackr, the patch looks nicer now.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/scoped_trans...
File ash/wm/overview/scoped_transform_overview_window.h (right):

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/scoped_trans...
ash/wm/overview/scoped_transform_overview_window.h:115: // The original opacity
before entering overview mode.
On 2014/06/25 15:48:22, tdanderson wrote:
> opactity of the window

Done.

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:309: // letters to lowercase in a l10n sensitive
context.
On 2014/06/25 15:48:23, tdanderson wrote:
> I would instead mention the fact that your filtering is accent- and
> case-insensitive in the method documentation for Filter() (in the header).

Done.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.cc:332: // shows up.
On 2014/06/25 15:48:23, tdanderson wrote:
> "shows up on the first unfiltered item"?

Done.

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.h:71: void Filter(const base::string16& pattern);
On 2014/06/25 15:48:23, tdanderson wrote:
> I'd prefer a slightly more descriptive name such as "FilterItems"

Done.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.h:131: // Indicates if there is at least one non
filtered item in the overview.
On 2014/06/25 15:48:23, tdanderson wrote:
> I notice that you are using four different terms in your comments, method
names,
> and variable names to all mean essentially the same thing: dimmed, filtered,
> active, and visible. I suggest using the same one everywhere for consistency.
> "Visible" is a bit misleading since the items are still visible (just
> transparent), and "active" is overloaded. So I'd go with either "dimmed" or
> "filtered".

The echoes of my English teacher - don't repeat the same word, use synonyms!
Actually dimmed and filtered mean one thing, active and visible another thing.
Changed.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_grid....
ash/wm/overview/window_grid.h:132: bool item_active_;
On 2014/06/25 15:48:23, tdanderson wrote:
> I would re-name to be more descriptive, e.g., something like has_active_item_.
> The name as-is suggests that there is only one "item" and it is either active
or
> not.

Done.

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:92: views::Widget*
CreateTextFilter(views::TextfieldController* controller,
On 2014/06/25 15:48:23, tdanderson wrote:
> Add some documentation on line 91 for this method.

Done.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:264: return true;
On 2014/06/25 15:48:23, tdanderson wrote:
> Should this be return false?

I don't think we want the user to be able to write line breaks on the textfield.
Returning true means we handled the event and the textfield will not attempt to
use it.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:320: if (ignore_activations_ ||
!gained_active ||
On 2014/06/25 15:48:23, tdanderson wrote:
> Use three lines.

Done.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:365: iter != grid_list_.end(); iter++) {
On 2014/06/25 15:48:23, tdanderson wrote:
> Align iter with ScopedVector.

Done.

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.h:95: 
On 2014/06/25 15:48:23, tdanderson wrote:
> Remove blank line

Done.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.h:97: const ui::KeyEvent& key_event);
On 2014/06/25 15:48:23, tdanderson wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.h:158: // Window text filter widget.
On 2014/06/25 15:48:23, tdanderson wrote:
> Explain in a bit more detail what this is / what it's used for (since we own
> this member, it should be our responsibility to give this explanation).

Done.

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_panels.cc:204: iter != transform_windows_.end();
iter++) {
On 2014/06/25 15:48:23, tdanderson wrote:
> Align iter with WindowList.

Done.

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

https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_window.cc:75: float opacity = visible ? 1 : 0.5;
On 2014/06/25 15:48:23, tdanderson wrote:
> Local variable not needed.

Removed.

https://codereview.chromium.org/358553004/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/358553004/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield.cc:437: SchedulePaint();
On 2014/06/25 15:48:23, tdanderson wrote:
> It should be the responsibility of the caller to invoke SchedulePaint() if
> needed; we should not be doing it inside a mutator.

Hmmm all the other setters call SchedulePaint(). If this is better, we might
have to change those to inlined functions and make sure their callers call
SchedulePaint(), right?

https://codereview.chromium.org/358553004/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield.cc:1601:
shadows.push_back(gfx::ShadowValue(gfx::Point(0, 1), 10, SK_ColorBLACK));
On 2014/06/25 15:48:23, tdanderson wrote:
> I wonder if it's worthwhile defining constants for these arguments (as well as
> any other hard-coded values we have in the overview mode code such as 1 for
> "unfiltered opacity" and 0.5 as "filtered opacity". Then all of the values
> controlling the look and feel would be in one place.
> 
> It might also be worth making a helper to avoid repetition of the logic:
active
> ? 1 : 0.5.

As we discussed off the patch, we might want to change this later.

https://codereview.chromium.org/358553004/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/358553004/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield.h:124: void DisplayShadow(bool
display_shadow);
On 2014/06/25 15:48:23, tdanderson wrote:
> If the instance variable is has_shadow_, this should be called SetHasShadow().

Done.

[flackr, 2014-06-26 17:33:08]

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/scoped_t...
File ash/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/scoped_t...
ash/wm/overview/scoped_transform_overview_window.cc:101:
opacity_(window->layer()->opacity()) {
Use GetTargetOpacity so that if instantiated during an animation we exit to the
correct opacity.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/scoped_t...
ash/wm/overview/scoped_transform_overview_window.cc:162: original_transform_ =
gfx::Transform();
Probably should set opacity_ = 1.0f here so that the selected window will not
restore an old opacity of 0.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:228: return false;
Does this mean you could get stuck if you move the selection to a screen with no
active items?

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:312: translit->transliterate(target);
I can't find any occurrences in chrome/ which directly instantiate
Transliterator. Are you sure this shouldn't be calling one of the helper
functions in base/i18n/ instead like
base::FixedPatternStringSearchIgnoringCaseAndAccents?
https://code.google.com/p/chromium/codesearch#chromium/src/base/i18n/string_s...

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:332: Move(WindowSelector::RIGHT);
Just to check, we probably don't want this to do a slide animation, does calling
Move(RIGHT) do this?

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.h (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.h:69: // Dims the items that do not match the given
pattern and prevents their
nit:s/that do not match the given pattern/whose titles do not contain |pattern|

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

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:105: root_window->bounds().width() / 2, 0);
nit: use constants, in this case something like kTextFilterScreenProportion =
0.5 which implies a centered rectangle that is 50% of the screen width.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:132: showing_selection_widget_(false) {
Do we need this variable or can we just check if the text contents are empty?

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:354: if (!showing_selection_widget_ ||
new_contents.empty()) {
We did say this should be behind a flag right? Can you add a flag which if not
set you won't be able to search? You can and still should create the hidden
textbox to handle input as this makes the design much cleaner, thanks again for
doing it this way!

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:364: showing_selection_widget_ ? 0 : 50));
To do this animation on the compositor this should use a translate transform to
slide in from above the screen. I believe bounds animations (especially when the
height changes) require re-rastering every frame.

Also, use a constant for the 50.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:241:
close_button_->GetNativeWindow()->layer()->SetOpacity(dimmed ? 0.5 : 1);
These should be done as part of ChangeItemVisibility / UpdateItemOpacity.

Also constant for 0.5f.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.h:100: virtual void
ChangeItemVisibility(bool visible) = 0;
nit: Items aren't becoming invisible, just not full opacity:
s/Visibility/Opacity.

Instead of bool, can we pass float opacity?

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:205:
(*iter)->window()->layer()->SetOpacity(visible ? 1 : 0.5);
This may deserve a TODO as I think this will let you see through the individual
panels to the ones behind them which is likely to not be the desired effect.

https://codereview.chromium.org/358553004/diff/20001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:656: base::WeakPtr<Textfield>
textfield(weak_ptr_factory_.GetWeakPtr());
nit: Might be worth a comment that HandleKeyEvent may delete |this|.

https://codereview.chromium.org/358553004/diff/20001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:1605:
shadows.push_back(gfx::ShadowValue(gfx::Point(0, 1), 10, SK_ColorBLACK));
Maybe we should pass the ShadowValues in when setting a shadow so this shadow
support works for any kind of shadow.

[flackr, 2014-06-26 17:49:43]

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:320: if (title.find(target.getTerminatedBuffer())
!= base::string16::npos) {
Presumably the empty string (i.e. when entire search query is backspaced)
matches everything? Add a comment if so.

[Nina, 2014-06-27 15:20:39]

Hey Rob, the patch is ready to receive more flak...

...ok that was a bad pun. Anyways, I added a basic test, more tests to follow.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/scoped_t...
File ash/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/scoped_t...
ash/wm/overview/scoped_transform_overview_window.cc:101:
opacity_(window->layer()->opacity()) {
On 2014/06/26 17:33:07, flackr wrote:
> Use GetTargetOpacity so that if instantiated during an animation we exit to
the
> correct opacity.

Done.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/scoped_t...
ash/wm/overview/scoped_transform_overview_window.cc:162: original_transform_ =
gfx::Transform();
On 2014/06/26 17:33:07, flackr wrote:
> Probably should set opacity_ = 1.0f here so that the selected window will not
> restore an old opacity of 0.

Wow this was a really serious bug, since minimized windows got a free upgrade to
"ghost windows" if selected in the overview. Fixed.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:228: return false;
On 2014/06/26 17:33:07, flackr wrote:
> Does this mean you could get stuck if you move the selection to a screen with
no
> active items?

Yes. And panic, this is not the only multi monitor bug. Fixed.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:312: translit->transliterate(target);
On 2014/06/26 17:33:07, flackr wrote:
> I can't find any occurrences in chrome/ which directly instantiate
> Transliterator. Are you sure this shouldn't be calling one of the helper
> functions in base/i18n/ instead like
> base::FixedPatternStringSearchIgnoringCaseAndAccents?
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/i18n/string_s...

OMG that function is pure awesomeness. Switched to that.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:320: if (title.find(target.getTerminatedBuffer())
!= base::string16::npos) {
On 2014/06/26 17:49:43, flackr wrote:
> Presumably the empty string (i.e. when entire search query is backspaced)
> matches everything? Add a comment if so.

Added comment to header file.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:332: Move(WindowSelector::RIGHT);
On 2014/06/26 17:33:07, flackr wrote:
> Just to check, we probably don't want this to do a slide animation, does
calling
> Move(RIGHT) do this?

Move() does animate it. Modified to receive a bool param.
FYI This code has been moved to the WindowSelector now.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.h (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.h:69: // Dims the items that do not match the given
pattern and prevents their
On 2014/06/26 17:33:07, flackr wrote:
> nit:s/that do not match the given pattern/whose titles do not contain
|pattern|

Done.

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

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:105: root_window->bounds().width() / 2, 0);
On 2014/06/26 17:33:07, flackr wrote:
> nit: use constants, in this case something like kTextFilterScreenProportion =
> 0.5 which implies a centered rectangle that is 50% of the screen width.

Done.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:132: showing_selection_widget_(false) {
On 2014/06/26 17:33:07, flackr wrote:
> Do we need this variable or can we just check if the text contents are empty?

We could remove the variable at the cost of having to animate the widget moving
in every single time the user wrote something, as we have no idea what the
contents were _before_ he did. I'd rather leave it there.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:354: if (!showing_selection_widget_ ||
new_contents.empty()) {
On 2014/06/26 17:33:07, flackr wrote:
> We did say this should be behind a flag right? Can you add a flag which if not
> set you won't be able to search? You can and still should create the hidden
> textbox to handle input as this makes the design much cleaner, thanks again
for
> doing it this way!

Added --disable* flag

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:364: showing_selection_widget_ ? 0 : 50));
On 2014/06/26 17:33:07, flackr wrote:
> To do this animation on the compositor this should use a translate transform
to
> slide in from above the screen. I believe bounds animations (especially when
the
> height changes) require re-rastering every frame.
> 
> Also, use a constant for the 50.

Done!

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:241:
close_button_->GetNativeWindow()->layer()->SetOpacity(dimmed ? 0.5 : 1);
On 2014/06/26 17:33:08, flackr wrote:
> These should be done as part of ChangeItemVisibility / UpdateItemOpacity.
> 
> Also constant for 0.5f.

Done.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.h:100: virtual void
ChangeItemVisibility(bool visible) = 0;
On 2014/06/26 17:33:08, flackr wrote:
> nit: Items aren't becoming invisible, just not full opacity:
> s/Visibility/Opacity.
> 
> Instead of bool, can we pass float opacity?

Done.

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:205:
(*iter)->window()->layer()->SetOpacity(visible ? 1 : 0.5);
On 2014/06/26 17:33:08, flackr wrote:
> This may deserve a TODO as I think this will let you see through the
individual
> panels to the ones behind them which is likely to not be the desired effect.

True, I've already seen that and I don't like how it looks. Added TODO(flackr).

https://codereview.chromium.org/358553004/diff/20001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/358553004/diff/20001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:656: base::WeakPtr<Textfield>
textfield(weak_ptr_factory_.GetWeakPtr());
On 2014/06/26 17:33:08, flackr wrote:
> nit: Might be worth a comment that HandleKeyEvent may delete |this|.

Done.

[flackr, 2014-06-27 20:00:07]

I guess I started commenting while you were still uploading patches. Let me know
if you addressed anything already.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:225: if (!has_active_item_)
This is the only place where has_active_item_ is used and you have to handle
overflowing the list of items anyways. I suggest removing has_active_item_.

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

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:122: -kTextFilterHeight,
Should this be created at 0 and transformed to -kTextFilterHeight so that it's
ready to slide in?

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:182: CreateTextFilter(this,
Shell::GetPrimaryRootWindow()));
Do we have to worry about the text input element losing focus? Maybe we should
exit overview mode if that happens? If you don't think it can happen without
another window being activated (which would cause us to exit overview mode) then
that's fine.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:286: return true;
Isn't this technically not handled? Should we return false here?

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:375:
switches::kAshDisableTextFilteringInOverviewMode)) {
nit: Indend 4 from '(', I guess a total of 10 spaces?

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:377: }
nit: blank line to separate this early return from rest of method.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:378: if (!showing_selection_widget_ ||
new_contents.empty()) {
nit: To make sure this always does the right thing (when other people change
it), I'd suggest adding a bool should_should_selection_widget =
!new_contents.empty();
Then if can be:
if (showing_selection_widget_ != should_should_selection_widget)
And you can update showing_... to be should_show_... at the end of the block.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:387: if (showing_selection_widget_)
This is confusing because you're doing the opposite of
showing_selection_widget_, i think my suggestion above will fix the confusion
though.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:390: transform.Translate(0,
kTextFilterHeight);
This should just be transformed to (0, 0), we should initialize it to be
transformed offscreen.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:174: SetOpacity(dimmed ?
kDimmedItemOpacity : 1);
nit: Use 1.0f as this is used as a float.

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

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:1050: }
Cool, nice and simple!

Maybe a few more tests like moving selection past dimmed items and undimming.

https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.cc:1598: 
nit: no need for an extra newline here.

https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.h:124: void set_shadows(const
gfx::ShadowValues& shadows) { shadows_ = shadows; }
Does this need to SchedulePaint?

[Nina, 2014-06-27 22:42:02]

Okay, please look at my comments and the updated patch. Hopefully tomorrow I'll
also have more tests.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:225: if (!has_active_item_)
On 2014/06/27 20:00:06, flackr wrote:
> This is the only place where has_active_item_ is used and you have to handle
> overflowing the list of items anyways. I suggest removing has_active_item_.

Done.

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

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:122: -kTextFilterHeight,
On 2014/06/27 20:00:06, flackr wrote:
> Should this be created at 0 and transformed to -kTextFilterHeight so that it's
> ready to slide in?

Done.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:182: CreateTextFilter(this,
Shell::GetPrimaryRootWindow()));
On 2014/06/27 20:00:07, flackr wrote:
> Do we have to worry about the text input element losing focus? Maybe we should
> exit overview mode if that happens? If you don't think it can happen without
> another window being activated (which would cause us to exit overview mode)
then
> that's fine.

I really don't think it can happen, as we are observing most of the containers,
and when a window is activated we just cancel the overview.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:286: return true;
On 2014/06/27 20:00:06, flackr wrote:
> Isn't this technically not handled? Should we return false here?

Well, apparently both options have the same effect, and since we don't want
enter to do anything besides selecting, I think returning true is the best
course of action.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:375:
switches::kAshDisableTextFilteringInOverviewMode)) {
On 2014/06/27 20:00:06, flackr wrote:
> nit: Indend 4 from '(', I guess a total of 10 spaces?

Damn Eclipse!

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:377: }
On 2014/06/27 20:00:06, flackr wrote:
> nit: blank line to separate this early return from rest of method.

Done.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:378: if (!showing_selection_widget_ ||
new_contents.empty()) {
On 2014/06/27 20:00:06, flackr wrote:
> nit: To make sure this always does the right thing (when other people change
> it), I'd suggest adding a bool should_should_selection_widget =
> !new_contents.empty();
> Then if can be:
> if (showing_selection_widget_ != should_should_selection_widget)
> And you can update showing_... to be should_show_... at the end of the block.

I'll take you meant should_show_*. Done.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:387: if (showing_selection_widget_)
On 2014/06/27 20:00:06, flackr wrote:
> This is confusing because you're doing the opposite of
> showing_selection_widget_, i think my suggestion above will fix the confusion
> though.

Done.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:390: transform.Translate(0,
kTextFilterHeight);
On 2014/06/27 20:00:06, flackr wrote:
> This should just be transformed to (0, 0), we should initialize it to be
> transformed offscreen.

Done.

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:174: SetOpacity(dimmed ?
kDimmedItemOpacity : 1);
On 2014/06/27 20:00:07, flackr wrote:
> nit: Use 1.0f as this is used as a float.

Done.

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

https://codereview.chromium.org/358553004/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:1050: }
On 2014/06/27 20:00:07, flackr wrote:
> Cool, nice and simple!
> 
> Maybe a few more tests like moving selection past dimmed items and undimming.

There's a v2 of this which you might want to see again. It's a bit better. More
tests coming tomorrow.

https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.h:124: void set_shadows(const
gfx::ShadowValues& shadows) { shadows_ = shadows; }
On 2014/06/27 20:00:07, flackr wrote:
> Does this need to SchedulePaint?

Not if we call this before we show the widget. Also, this is how the label works
(apparently they changed it, I'm sure it used to be different).

[Nina, 2014-07-02 15:17:03]

Tests added!

[flackr, 2014-07-02 15:50:43]

Tests look good, thanks!

https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.h:124: void set_shadows(const
gfx::ShadowValues& shadows) { shadows_ = shadows; }
On 2014/06/27 22:42:02, Nicolás wrote:
> On 2014/06/27 20:00:07, flackr wrote:
> > Does this need to SchedulePaint?
> 
> Not if we call this before we show the widget. Also, this is how the label
works
> (apparently they changed it, I'm sure it used to be different).

As far as I can tell, most methods which modify the appearance of the view call
SchedulePaint to ensure that it gets redrawn. Can you change this one to do so
as well? It may work in your scenario but others may call set_shadows after
showing and expect it to work.

I imagine label should be changed as well (may be implied by the size
invalidation) but that can be done later.

https://codereview.chromium.org/358553004/diff/140001/ash/wm/overview/window_...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/358553004/diff/140001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:384: 
nit: no need for newline here.

https://codereview.chromium.org/358553004/diff/140001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/358553004/diff/140001/ash/wm/overview/window_...
ash/wm/overview/window_selector_unittest.cc:216: GetTargetTransform() ==
gfx::Transform());
nit: GetTargetTransform().IsIdentity()

[Nina, 2014-07-02 17:39:05]

Sadrul,

Can you please review files ui/*? Thanks!

Flackr,

I got the issues solved. Please give the patch a new look. Thanks!

https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.h:124: void set_shadows(const
gfx::ShadowValues& shadows) { shadows_ = shadows; }
On 2014/07/02 15:50:42, flackr wrote:
> On 2014/06/27 22:42:02, Nicolás wrote:
> > On 2014/06/27 20:00:07, flackr wrote:
> > > Does this need to SchedulePaint?
> > 
> > Not if we call this before we show the widget. Also, this is how the label
> works
> > (apparently they changed it, I'm sure it used to be different).
> 
> As far as I can tell, most methods which modify the appearance of the view
call
> SchedulePaint to ensure that it gets redrawn. Can you change this one to do so
> as well? It may work in your scenario but others may call set_shadows after
> showing and expect it to work.
> 
> I imagine label should be changed as well (may be implied by the size
> invalidation) but that can be done later.

Okay, done.

https://codereview.chromium.org/358553004/diff/140001/ash/wm/overview/window_...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/358553004/diff/140001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:384: 
On 2014/07/02 15:50:42, flackr wrote:
> nit: no need for newline here.

Done.

https://codereview.chromium.org/358553004/diff/140001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/358553004/diff/140001/ash/wm/overview/window_...
ash/wm/overview/window_selector_unittest.cc:216: GetTargetTransform() ==
gfx::Transform());
On 2014/07/02 15:50:42, flackr wrote:
> nit: GetTargetTransform().IsIdentity()

Just another use of those linear algebra classes! Done.

[sadrul, 2014-07-02 17:44:04]

https://codereview.chromium.org/358553004/diff/160001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/358553004/diff/160001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:656: // it isn't null before
proceeding.
This change should be in a separate CL, with a corresponding test.

[flackr, 2014-07-02 17:54:04]

LGTM

[Nina, 2014-07-02 20:10:52]

Sadrul, this patch is blocked on the textfield change now, but still waiting for
approval (I removed the code that we put in the other patch).

Cheers!

[sadrul, 2014-07-02 20:21:27]

lgtm

[msw, 2014-07-03 05:28:55]

https://codereview.chromium.org/358553004/diff/180001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/358553004/diff/180001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:413: shadows_ = shadows;
Just set the shadows on the RenderText here, you don't need to add a
Textfield::shadows_ member.

[Nina, 2014-07-04 13:56:48]

https://codereview.chromium.org/358553004/diff/180001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/358553004/diff/180001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:413: shadows_ = shadows;
On 2014/07/03 05:28:55, msw wrote:
> Just set the shadows on the RenderText here, you don't need to add a
> Textfield::shadows_ member.

Done.

[Nina, 2014-07-04 15:40:09]

The CQ bit was checked by nsatragno@chromium.org

[commit-bot: I haz the power, 2014-07-04 15:40:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nsatragno@chromium.org/358553004/190001

[Nina, 2014-07-04 17:14:14]

The CQ bit was checked by nsatragno@chromium.org

[commit-bot: I haz the power, 2014-07-04 17:15:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nsatragno@chromium.org/358553004/210001

[Nina, 2014-07-04 19:49:23]

The CQ bit was unchecked by nsatragno@chromium.org

[Nina, 2014-07-04 20:51:10]

Oshima, please review this TBR on the unittest. This is my last day, so please
direct any questions or comments to tdanderson@ or flackr@. Thanks!

https://codereview.chromium.org/358553004/diff/230001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/358553004/diff/230001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1235:
window.reset(CreateTestWindowInShellWithBounds(gfx::Rect(5, 5, 20, 20)));
This is to avoid calling "minimize" on a  window in overview mode, which
crashes. Since the test has no way of knowing what the actions it calls do, it's
nicer to reset the state of the window each time an action is executed. WDYT?

[Nina, 2014-07-04 20:51:21]

The CQ bit was checked by nsatragno@chromium.org

[commit-bot: I haz the power, 2014-07-04 20:51:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nsatragno@chromium.org/358553004/230001

[commit-bot: I haz the power, 2014-07-04 22:51:11]

Change committed as 281450

[oshima, 2014-07-07 23:30:58]

Rob, I think we should fix this.

https://codereview.chromium.org/358553004/diff/230001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/358553004/diff/230001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1235:
window.reset(CreateTestWindowInShellWithBounds(gfx::Rect(5, 5, 20, 20)));
On 2014/07/04 20:51:10, Nicolás wrote:
> This is to avoid calling "minimize" on a window in overview mode, which
> crashes. Since the test has no way of knowing what the actions it calls do,
it's
> nicer to reset the state of the window each time an action is executed. WDYT?

Requesting minimize in overview mode should not cause a crash. (App window can
minimize it self, for example)

[Nina, 2014-07-08 02:22:51]

On 2014/07/07 23:30:58, oshima wrote:
> Rob, I think we should fix this.
> 
>
https://codereview.chromium.org/358553004/diff/230001/ash/accelerators/accele...
> File ash/accelerators/accelerator_controller_unittest.cc (right):
> 
>
https://codereview.chromium.org/358553004/diff/230001/ash/accelerators/accele...
> ash/accelerators/accelerator_controller_unittest.cc:1235:
> window.reset(CreateTestWindowInShellWithBounds(gfx::Rect(5, 5, 20, 20)));
> On 2014/07/04 20:51:10, Nicolás wrote:
> > This is to avoid calling "minimize" on a window in overview mode, which
> > crashes. Since the test has no way of knowing what the actions it calls do,
> it's
> > nicer to reset the state of the window each time an action is executed.
WDYT?
> 
> Requesting minimize in overview mode should not cause a crash. (App window can
> minimize it self, for example)

I still feel the test is not the most appropriate one. Maybe fill a different
bug for this issue? If we expect windows to minimize themselves, then I agree we
must not crash if that happens.