Converts MruWindowTracker to work with common types

Converts MruWindowTracker to work with common types

BUG=617789
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/948acf4e7e86ad43245929648514d00a805eae5e
Cr-Commit-Position: refs/heads/master@{#398315}

[sky, 2016-06-06 22:58:39]

I'll TBR a file move next.

[James Cook, 2016-06-07 01:21:05]

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm/focus_rul...
File ash/common/wm/focus_rules.cc (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm/focus_rul...
ash/common/wm/focus_rules.cc:24: bool IsWindowConsideredActivatable(WmWindow*
window) {
Do you need to handle a null window? I see the BaseFocusRules version does,
though the ash version does not.

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm/focus_rul...
File ash/common/wm/focus_rules.h (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm/focus_rul...
ash/common/wm/focus_rules.h:20: }  // namepsace ash
nit: namespace

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:76: // intended to check if the current users matches the
user associated with
nit: "current users" -> "current user" (unless this is somehow dealing with
multiprofile/multiuser, which I'm guessing not).

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_window.h#...
ash/common/wm_window.h:57: using Windows = std::vector<WmWindow*>;
optional: I know this is style-guide compliant, and I know we do it in lots of
places, but I'm not a fan. I think it's important to show, at the call site
manipulating the windows, the type of the container (set? vector? other?), since
it affects both performance and what STL API functions you can use (insert?
push_back?).

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
File ash/wm/mru_window_tracker.cc (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker.cc:54: for (WmWindow* window :
WmShell::Get()->GetAllRootWindows()) {
Hooray for nice ranged for loops!

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
File ash/wm/mru_window_tracker_unittest.cc (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker_unittest.cc:5: #include "ash/wm/mru_window_tracker.h"
Thanks for moving this up. It's one of my pet peeves in test code.

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker_unittest.cc:12: #include
"ash/test/test_shelf_delegate.h"
While you're here, I don't think this is needed.

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker_unittest.cc:15: #include "base/compiler_specific.h"
or this

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker_unittest.cc:16: #include
"ui/aura/test/test_window_delegate.h"
or this

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker_unittest.cc:42: std::unique_ptr<aura::Window>
w1(CreateWindow());
It seems like this test could be converted to create test widgets, instead of
aura::Windows, which would let it move over to common MruWindowTracker.

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/window_cycle_lis...
File ash/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:95: if (params.target == window_ &&
params.new_parent == nullptr) {
nit: I think this would be clearer as an early exit on (params.new_parent !=
nullptr) with a comment that it only looks for removal.

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:110: for (WmWindow* window : windows_)
Thank you for fixing this to use explicit types.

[sky, 2016-06-07 02:46:21]

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm/focus_rul...
File ash/common/wm/focus_rules.cc (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm/focus_rul...
ash/common/wm/focus_rules.cc:24: bool IsWindowConsideredActivatable(WmWindow*
window) {
On 2016/06/07 01:21:04, James Cook wrote:
> Do you need to handle a null window? I see the BaseFocusRules version does,
> though the ash version does not.

Seem weird to me to null check. I added a DCHECK.

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm/focus_rul...
File ash/common/wm/focus_rules.h (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm/focus_rul...
ash/common/wm/focus_rules.h:20: }  // namepsace ash
On 2016/06/07 01:21:04, James Cook wrote:
> nit: namespace

Done.

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_shell.h#n...
ash/common/wm_shell.h:76: // intended to check if the current users matches the
user associated with
On 2016/06/07 01:21:04, James Cook wrote:
> nit: "current users" -> "current user" (unless this is somehow dealing with
> multiprofile/multiuser, which I'm guessing not).

Done.

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_window.h#...
ash/common/wm_window.h:57: using Windows = std::vector<WmWindow*>;
On 2016/06/07 01:21:05, James Cook wrote:
> optional: I know this is style-guide compliant, and I know we do it in lots of
> places, but I'm not a fan. I think it's important to show, at the call site
> manipulating the windows, the type of the container (set? vector? other?),
since
> it affects both performance and what STL API functions you can use (insert?
> push_back?).

I left it for the following reasons:
. as you say, this style is used in plenty of places.
. when performance matters you have to know the type and will look it up.

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
File ash/wm/mru_window_tracker_unittest.cc (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker_unittest.cc:12: #include
"ash/test/test_shelf_delegate.h"
On 2016/06/07 01:21:05, James Cook wrote:
> While you're here, I don't think this is needed.

Done.

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker_unittest.cc:15: #include "base/compiler_specific.h"
On 2016/06/07 01:21:05, James Cook wrote:
> or this

Done.

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker_unittest.cc:16: #include
"ui/aura/test/test_window_delegate.h"
On 2016/06/07 01:21:05, James Cook wrote:
> or this

Done.

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker_unittest.cc:42: std::unique_ptr<aura::Window>
w1(CreateWindow());
On 2016/06/07 01:21:05, James Cook wrote:
> It seems like this test could be converted to create test widgets, instead of
> aura::Windows, which would let it move over to common MruWindowTracker.

I will convert the test separately.

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/window_cycle_lis...
File ash/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:95: if (params.target == window_ &&
params.new_parent == nullptr) {
On 2016/06/07 01:21:05, James Cook wrote:
> nit: I think this would be clearer as an early exit on (params.new_parent !=
> nullptr) with a comment that it only looks for removal.

Done.

[James Cook, 2016-06-07 14:05:38]

LGTM

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_window.h#...
ash/common/wm_window.h:57: using Windows = std::vector<WmWindow*>;
On 2016/06/07 02:46:21, sky wrote:
> On 2016/06/07 01:21:05, James Cook wrote:
> > optional: I know this is style-guide compliant, and I know we do it in lots
of
> > places, but I'm not a fan. I think it's important to show, at the call site
> > manipulating the windows, the type of the container (set? vector? other?),
> since
> > it affects both performance and what STL API functions you can use (insert?
> > push_back?).
> 
> I left it for the following reasons:
> . as you say, this style is used in plenty of places.
> . when performance matters you have to know the type and will look it up.

still optional: How about WindowList as a name, then?

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
File ash/wm/mru_window_tracker_unittest.cc (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/wm/mru_window_track...
ash/wm/mru_window_tracker_unittest.cc:42: std::unique_ptr<aura::Window>
w1(CreateWindow());
On 2016/06/07 02:46:21, sky wrote:
> On 2016/06/07 01:21:05, James Cook wrote:
> > It seems like this test could be converted to create test widgets, instead
of
> > aura::Windows, which would let it move over to common MruWindowTracker.
> 
> I will convert the test separately.

Acknowledged.

[sky, 2016-06-07 15:34:22]

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2042913002/diff/20001/ash/common/wm_window.h#...
ash/common/wm_window.h:57: using Windows = std::vector<WmWindow*>;
On 2016/06/07 14:05:38, James Cook wrote:
> On 2016/06/07 02:46:21, sky wrote:
> > On 2016/06/07 01:21:05, James Cook wrote:
> > > optional: I know this is style-guide compliant, and I know we do it in
lots
> of
> > > places, but I'm not a fan. I think it's important to show, at the call
site
> > > manipulating the windows, the type of the container (set? vector? other?),
> > since
> > > it affects both performance and what STL API functions you can use
(insert?
> > > push_back?).
> > 
> > I left it for the following reasons:
> > . as you say, this style is used in plenty of places.
> > . when performance matters you have to know the type and will look it up.
> 
> still optional: How about WindowList as a name, then?

I went with Windows because of:
https://cs.chromium.org/chromium/src/ui/aura/window.h?rcl=0&l=85 .

[sky, 2016-06-07 15:34:28]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-06-07 15:34:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2042913002/60001

[commit-bot: I haz the power, 2016-06-07 17:05:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-07 17:06:31]

Description was changed from

==========
Converts MruWindowTracker to work with common types

BUG=617789
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Converts MruWindowTracker to work with common types

BUG=617789
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/948acf4e7e86ad43245929648514d00a805eae5e
Cr-Commit-Position: refs/heads/master@{#398315}
==========

[commit-bot: I haz the power, 2016-06-07 17:06:32]

Patchset 4 (id:??) landed as
https://crrev.com/948acf4e7e86ad43245929648514d00a805eae5e
Cr-Commit-Position: refs/heads/master@{#398315}