Issue 20708005: Refactor most recently used window tracking into a separate class.

Issue 20708005: Refactor most recently used window tracking into a separate class. (Closed)

Created:
7 years, 5 months ago by flackr

Modified:
7 years, 4 months ago

Reviewers:
sky

CC:
chromium-reviews, extensions-reviews_chromium.org, sadrul, nkostylev+watch_chromium.org, ben+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org

Base URL:
http://git.chromium.org/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Refactor most recently used window tracking into a separate class. BUG=263477 TEST=None Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214609

Patch Set 1 #

Total comments: 11

Patch Set 2 : Address comments. #

Created: 7 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+204 lines, -396 lines)			Patch
M	ash/ash.gyp	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	ash/focus_cycler.cc	View	1	2 chunks	+2 lines, -1 line	0 comments	Download
M	ash/shelf/shelf_layout_manager.cc	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	ash/shell.h	View		3 chunks	+5 lines, -0 lines	0 comments	Download
M	ash/shell.cc	View	1	4 chunks	+5 lines, -2 lines	0 comments	Download
A	ash/wm/mru_window_tracker.h	View	1	1 chunk	+88 lines, -0 lines	0 comments	Download
A +	ash/wm/mru_window_tracker.cc	View	1	8 chunks	+83 lines, -162 lines	0 comments	Download
M	ash/wm/window_cycle_controller.h	View		4 chunks	+2 lines, -52 lines	0 comments	Download
M	ash/wm/window_cycle_controller.cc	View	1	7 chunks	+5 lines, -171 lines	0 comments	Download
M	ash/wm/window_cycle_list.cc	View	1	3 chunks	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/chromeos/extensions/wallpaper_private_api.cc	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/ash/window_positioner.cc	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/window_sizer/window_sizer_ash.cc	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

flackr

Separated the refactoring of MRU window tracking into this CL. PTAL, thanks!

7 years, 5 months ago (2013-07-27 00:39:05 UTC) #1

sky

https://codereview.chromium.org/20708005/diff/1/ash/wm/mru_window_tracker.h File ash/wm/mru_window_tracker.h (right): https://codereview.chromium.org/20708005/diff/1/ash/wm/mru_window_tracker.h#newcode50 ash/wm/mru_window_tracker.h:50: // of the default descending. It isn't clear what ...

7 years, 4 months ago (2013-07-29 15:31:05 UTC) #2

flackr

7 years, 4 months ago (2013-07-29 22:49:11 UTC) #3

https://codereview.chromium.org/20708005/diff/1/ash/wm/mru_window_tracker.h
File ash/wm/mru_window_tracker.h (right):

https://codereview.chromium.org/20708005/diff/1/ash/wm/mru_window_tracker.h#n...
ash/wm/mru_window_tracker.h:50: // of the default descending.
On 2013/07/29 15:31:05, sky wrote:
> It isn't clear what descending/ascending means here. Document it.

Done.

https://codereview.chromium.org/20708005/diff/1/ash/wm/mru_window_tracker.h#n...
ash/wm/mru_window_tracker.h:52: const std::list<aura::Window*>* mru_windows,
On 2013/07/29 15:31:05, sky wrote:
> Why is mru_windows part of the API? Seems like its best done by the caller.

From a codesearch, the only callers who actually care about the order are
methods in window_cycle_controller. LinearCycle uses the stacking order while
alt-tab uses MRU order. The difference is that alt-tab currently activates every
window as you pass it and brings it to the front as you cycle. This is the
reason we maintain the MRU window list.

I've removed mru_windows from the static function, however, I think it would be
fine to just have one function which builds and use the Mru window list in all
cases since the callers that don't expect Mru order don't care about the order
(except for HandleLinearCycle in WindowCycleController which explicitly uses a
LRU list to ensure that you can single step through all windows).

https://codereview.chromium.org/20708005/diff/1/ash/wm/mru_window_tracker.h#n...
ash/wm/mru_window_tracker.h:64: void IgnoreActivations(bool ignore);
On 2013/07/29 15:31:05, sky wrote:
> SetIgnoreActivations. That said, since you're going to have two classes using
> this it feels rather brittle to have it a simple boolean. Should it be a
> counter?

Done.

Overview mode won't actually need to ignore activations since it doesn't need to
activate windows to make them visible. Window cycler makes windows active as you
alt tab through them. I can still change this to a counter if you think it would
be safer.

https://codereview.chromium.org/20708005/diff/1/ash/wm/mru_window_tracker.h#n...
ash/wm/mru_window_tracker.h:65: private:
On 2013/07/29 15:31:05, sky wrote:
> nit: newline between 64/65.

Done.

https://codereview.chromium.org/20708005/diff/1/ash/wm/window_cycle_controlle...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/20708005/diff/1/ash/wm/window_cycle_controlle...
ash/wm/window_cycle_controller.cc:103: WindowCycleList
cycle_list(MruWindowTracker::BuildWindowList(NULL,true));
On 2013/07/29 15:31:05, sky wrote:
> nit: space after 'NULL,'

NULL removed.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/20708005/8001

7 years, 4 months ago (2013-07-30 08:27:21 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/20708005/8001

7 years, 4 months ago (2013-07-30 15:39:45 UTC) #6

Message was sent while issue was closed.

Change committed as 214609

Expand Messages | Collapse Messages