Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(140)

Issue 358553004: Added text filtering to Overview Mode (Closed)

Created:
6 years, 6 months ago by Nina
Modified:
6 years, 5 months ago
CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, tfarina, tdanderson+overview_chromium.org, penghuang+watch_chromium.org, nona+watch_chromium.org, kalyank, James Su, ben+ash_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 36

Patch Set 2 : Fixed Terry's comments. #

Total comments: 31

Patch Set 3 : Fixed Rob's comments, added basic test. #

Patch Set 4 : Rebase (this patch set was not uploaded correctly for some reason) #

Patch Set 5 : Thanks chrome team for helping me fix the previous patch #

Total comments: 27

Patch Set 6 : Slight adjustments to test #

Patch Set 7 : Removed unused code. #

Patch Set 8 : Fixed Rob's comments (except for the tests) #

Patch Set 9 : Added another test. #

Total comments: 4

Patch Set 10 : Fix Rob's nits #

Total comments: 1

Patch Set 11 : Removed fix for textfield crash on destruction while handling key #

Total comments: 2

Patch Set 12 : Fixed nit #

Patch Set 13 : Fix compile error #

Patch Set 14 : Fixed unittest #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+408 lines, -54 lines) Patch
M ash/accelerators/accelerator_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +6 lines, -4 lines 2 comments Download
M ash/ash_switches.h View 1 2 4 1 chunk +1 line, -0 lines 0 comments Download
M ash/ash_switches.cc View 1 2 4 1 chunk +4 lines, -0 lines 0 comments Download
M ash/wm/overview/scoped_transform_overview_window.h View 1 4 1 chunk +3 lines, -0 lines 0 comments Download
M ash/wm/overview/scoped_transform_overview_window.cc View 1 2 4 3 chunks +4 lines, -1 line 0 comments Download
M ash/wm/overview/window_grid.h View 1 2 3 4 5 6 7 3 chunks +9 lines, -2 lines 0 comments Download
M ash/wm/overview/window_grid.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +27 lines, -5 lines 0 comments Download
M ash/wm/overview/window_selector.h View 1 2 4 7 chunks +24 lines, -7 lines 0 comments Download
M ash/wm/overview/window_selector.cc View 1 2 3 4 5 6 7 8 9 12 chunks +119 lines, -27 lines 0 comments Download
M ash/wm/overview/window_selector_item.h View 1 2 3 4 2 chunks +12 lines, -0 lines 0 comments Download
M ash/wm/overview/window_selector_item.cc View 1 2 3 4 5 6 7 3 chunks +48 lines, -1 line 0 comments Download
M ash/wm/overview/window_selector_panels.h View 1 2 4 1 chunk +1 line, -0 lines 0 comments Download
M ash/wm/overview/window_selector_panels.cc View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
M ash/wm/overview/window_selector_unittest.cc View 1 2 3 4 5 6 7 8 9 7 chunks +113 lines, -6 lines 0 comments Download
M ash/wm/overview/window_selector_window.h View 1 2 4 1 chunk +1 line, -0 lines 0 comments Download
M ash/wm/overview/window_selector_window.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/app/generated_resources.grd View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +6 lines, -0 lines 0 comments Download
M ui/views/controls/textfield/textfield.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -0 lines 0 comments Download
M ui/views/controls/textfield/textfield.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +6 lines, -1 line 0 comments Download

Messages

Total messages: 29 (0 generated)
Nina
Terry, Can you give this patch an initial review? I'm missing tests for now, but ...
6 years, 6 months ago (2014-06-25 13:58:54 UTC) #1
tdanderson
lgtm with the comments addressed below. Send over to Rob when ready. https://codereview.chromium.org/358553004/diff/1/ash/wm/overview/scoped_transform_overview_window.h File ash/wm/overview/scoped_transform_overview_window.h ...
6 years, 6 months ago (2014-06-25 15:48:23 UTC) #2
Nina
Flackr, want to give this patch a first review? Still haven't looked at Terry's nits, ...
6 years, 6 months ago (2014-06-25 16:02:22 UTC) #3
Nina
OK, I've got the first set of comments fixed. Flackr, the patch looks nicer now. ...
6 years, 6 months ago (2014-06-26 15:20:18 UTC) #4
flackr
https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/scoped_transform_overview_window.cc File ash/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/scoped_transform_overview_window.cc#newcode101 ash/wm/overview/scoped_transform_overview_window.cc:101: opacity_(window->layer()->opacity()) { Use GetTargetOpacity so that if instantiated during ...
6 years, 6 months ago (2014-06-26 17:33:08 UTC) #5
flackr
https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_grid.cc File ash/wm/overview/window_grid.cc (right): https://codereview.chromium.org/358553004/diff/20001/ash/wm/overview/window_grid.cc#newcode320 ash/wm/overview/window_grid.cc:320: if (title.find(target.getTerminatedBuffer()) != base::string16::npos) { Presumably the empty string ...
6 years, 6 months ago (2014-06-26 17:49:43 UTC) #6
Nina
Hey Rob, the patch is ready to receive more flak... ...ok that was a bad ...
6 years, 5 months ago (2014-06-27 15:20:39 UTC) #7
flackr
I guess I started commenting while you were still uploading patches. Let me know if ...
6 years, 5 months ago (2014-06-27 20:00:07 UTC) #8
Nina
Okay, please look at my comments and the updated patch. Hopefully tomorrow I'll also have ...
6 years, 5 months ago (2014-06-27 22:42:02 UTC) #9
Nina
Tests added!
6 years, 5 months ago (2014-07-02 15:17:03 UTC) #10
flackr
Tests look good, thanks! https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfield/textfield.h File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/358553004/diff/80001/ui/views/controls/textfield/textfield.h#newcode124 ui/views/controls/textfield/textfield.h:124: void set_shadows(const gfx::ShadowValues& shadows) { ...
6 years, 5 months ago (2014-07-02 15:50:43 UTC) #11
Nina
Sadrul, Can you please review files ui/*? Thanks! Flackr, I got the issues solved. Please ...
6 years, 5 months ago (2014-07-02 17:39:05 UTC) #12
sadrul
https://codereview.chromium.org/358553004/diff/160001/ui/views/controls/textfield/textfield.cc File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/358553004/diff/160001/ui/views/controls/textfield/textfield.cc#newcode656 ui/views/controls/textfield/textfield.cc:656: // it isn't null before proceeding. This change should ...
6 years, 5 months ago (2014-07-02 17:44:04 UTC) #13
flackr
LGTM
6 years, 5 months ago (2014-07-02 17:54:04 UTC) #14
Nina
Sadrul, this patch is blocked on the textfield change now, but still waiting for approval ...
6 years, 5 months ago (2014-07-02 20:10:52 UTC) #15
sadrul
lgtm
6 years, 5 months ago (2014-07-02 20:21:27 UTC) #16
msw
https://codereview.chromium.org/358553004/diff/180001/ui/views/controls/textfield/textfield.cc File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/358553004/diff/180001/ui/views/controls/textfield/textfield.cc#newcode413 ui/views/controls/textfield/textfield.cc:413: shadows_ = shadows; Just set the shadows on the ...
6 years, 5 months ago (2014-07-03 05:28:55 UTC) #17
Nina
https://codereview.chromium.org/358553004/diff/180001/ui/views/controls/textfield/textfield.cc File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/358553004/diff/180001/ui/views/controls/textfield/textfield.cc#newcode413 ui/views/controls/textfield/textfield.cc:413: shadows_ = shadows; On 2014/07/03 05:28:55, msw wrote: > ...
6 years, 5 months ago (2014-07-04 13:56:48 UTC) #18
Nina
The CQ bit was checked by nsatragno@chromium.org
6 years, 5 months ago (2014-07-04 15:40:09 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/358553004/190001
6 years, 5 months ago (2014-07-04 15:40:58 UTC) #20
Nina
The CQ bit was checked by nsatragno@chromium.org
6 years, 5 months ago (2014-07-04 17:14:14 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/358553004/210001
6 years, 5 months ago (2014-07-04 17:15:26 UTC) #22
Nina
The CQ bit was unchecked by nsatragno@chromium.org
6 years, 5 months ago (2014-07-04 19:49:23 UTC) #23
Nina
Oshima, please review this TBR on the unittest. This is my last day, so please ...
6 years, 5 months ago (2014-07-04 20:51:10 UTC) #24
Nina
The CQ bit was checked by nsatragno@chromium.org
6 years, 5 months ago (2014-07-04 20:51:21 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nsatragno@chromium.org/358553004/230001
6 years, 5 months ago (2014-07-04 20:51:40 UTC) #26
commit-bot: I haz the power
Change committed as 281450
6 years, 5 months ago (2014-07-04 22:51:11 UTC) #27
oshima
Rob, I think we should fix this. https://codereview.chromium.org/358553004/diff/230001/ash/accelerators/accelerator_controller_unittest.cc File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/358553004/diff/230001/ash/accelerators/accelerator_controller_unittest.cc#newcode1235 ash/accelerators/accelerator_controller_unittest.cc:1235: window.reset(CreateTestWindowInShellWithBounds(gfx::Rect(5, 5, ...
6 years, 5 months ago (2014-07-07 23:30:58 UTC) #28
Nina
6 years, 5 months ago (2014-07-08 02:22:51 UTC) #29
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698