Adding functions to the window_list_provider for accessing the activities window list

Adding functions to the window_list_provider for accessing the activities
window list

As discussed - added the window/activity order management functions
to the WindowListProvider and changed the use cases (at least partially).
There are still more things to do, but we might want to do that either
 a. incrementally or
 b. as a separate issue since the associated BUG(388085) has nothing to
    do with this.
Note: This is more work - especially because the code in question is being
worked on actively by others.

BUG=407103
TEST=athena_unittest.SimpleChecks, athena_unittest.TestWindowOrderingFunctions

Committed: https://crrev.com/227733e374f946b4335ffd55938da4296057bedf
Cr-Commit-Position: refs/heads/master@{#294103}

[Mr4D (OOO till 08-26), 2014-08-21 22:35:54]

Will call the functions accordingly from the resource manager once my patch has
(finally) landed.

Please have a look!

[Mr4D (OOO till 08-26), 2014-08-21 23:19:50]

Since the V2 app framework has landed I merged the required changes in.

[oshima, 2014-09-05 17:30:11]

can you rebase and upload new patch?

[Mr4D (OOO till 08-26), 2014-09-05 20:34:44]

Rebased!

I will change the new resource manager related todos till after this landed.

Please have another look!

[oshima, 2014-09-05 23:13:06]

I'll have a couple of more comments, but need to think a bit more.

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
File athena/wm/public/window_list_provider.h (right):

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
athena/wm/public/window_list_provider.h:21: virtual aura::Window::Windows
GetCurrentWindowList() const = 0;
Current implies there is something else. What's the motivation to use "Current"?

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
athena/wm/public/window_list_provider.h:24: virtual bool
IsWindowInList(aura::Window* window) const = 0;
I feel method name are too long. How about

Contains
{Move/Stack/Place}Window{FrontOf/BehindTo}?

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
athena/wm/public/window_list_provider.h:32: virtual void AddWindow(aura::Window*
window) = 0;
Do you need this? A window must be added by Widget/WindowTreeClient.

https://codereview.chromium.org/480293003/diff/80001/athena/wm/window_list_pr...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/480293003/diff/80001/athena/wm/window_list_pr...
athena/wm/window_list_provider_impl.cc:62: container_->StackChildAbove(window,
reference_window);
I think you can just get the window below reference window and stack above.

That can eliminate the resource manager depenency (wm should not depend on
resource manager)

[Mr4D (OOO till 08-26), 2014-09-08 17:35:07]

Moved the WindowObservers from the ResourceManager to the WindowListProvider.
Wanted to do that later, but now seems to be a good time as well.

Please have another look!

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
File athena/wm/public/window_list_provider.h (right):

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
athena/wm/public/window_list_provider.h:21: virtual aura::Window::Windows
GetCurrentWindowList() const = 0;
The returned list will not change if the content changes. As such the list will
be an incorrect representation once it did change. I used "Current" to show that
this is capturing the "current" state - but in the future it might be incorrect.

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
athena/wm/public/window_list_provider.h:24: virtual bool
IsWindowInList(aura::Window* window) const = 0;
On 2014/09/05 23:13:06, oshima wrote:
> I feel method name are too long. How about
> 
> Contains
> {Move/Stack/Place}Window{FrontOf/BehindTo}?
> 

Done.

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
athena/wm/public/window_list_provider.h:32: virtual void AddWindow(aura::Window*
window) = 0;
Someone must have the knowledge of where to add an activity, and for the sake of
knowledge encapsulation (what this is about) this knowledge should reside in one
place. Only this way we get a clear separation of the way how it is managed.

We might want to add this later back in again, but for the time being I have
changed it to observe window changes.

https://codereview.chromium.org/480293003/diff/80001/athena/wm/window_list_pr...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/480293003/diff/80001/athena/wm/window_list_pr...
athena/wm/window_list_provider_impl.cc:62: container_->StackChildAbove(window,
reference_window);
Apparently something has changed in the behavior of "Window::StackChild..()". I
added unit tests to make sure that it stays this way. Simplified things.

[oshima, 2014-09-09 18:28:38]

thanks, looks better.

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
File athena/wm/public/window_list_provider.h (right):

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
athena/wm/public/window_list_provider.h:21: virtual aura::Window::Windows
GetCurrentWindowList() const = 0;
On 2014/09/08 17:35:07, Mr4D wrote:
> The returned list will not change if the content changes. As such the list
will
> be an incorrect representation once it did change. I used "Current" to show
that
> this is capturing the "current" state - but in the future it might be
incorrect.

That's same with any API that returns list (aura::Window::children(),
shell::GetAllWindows() etc). I still think current is overkill.

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
athena/wm/public/window_list_provider.h:32: virtual void AddWindow(aura::Window*
window) = 0;
On 2014/09/08 17:35:07, Mr4D wrote:
> Someone must have the knowledge of where to add an activity, and for the sake
of
> knowledge encapsulation (what this is about) this knowledge should reside in
one
> place. Only this way we get a clear separation of the way how it is managed.
> 
> We might want to add this later back in again, but for the time being I have
> changed it to observe window changes.
> 

Adding is implicit now, so having AddWindow is confusing. I agree that we may
need this when we redesign the list management. We can add it when we need it.

https://codereview.chromium.org/480293003/diff/80001/athena/wm/window_list_pr...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/480293003/diff/80001/athena/wm/window_list_pr...
athena/wm/window_list_provider_impl.cc:62: container_->StackChildAbove(window,
reference_window);
On 2014/09/08 17:35:07, Mr4D wrote:
> Apparently something has changed in the behavior of "Window::StackChild..()".
I
> added unit tests to make sure that it stays this way. Simplified things.

Hmm Ok. There is another way to control stacking, StackingClient. This is
currently used to manage transient window, and we can extend
TransientWindowStackingClient if that's better way.

https://codereview.chromium.org/480293003/diff/100001/athena/resource_manager...
File athena/resource_manager/resource_manager_impl.cc (right):

https://codereview.chromium.org/480293003/diff/100001/athena/resource_manager...
athena/resource_manager/resource_manager_impl.cc:200: if (pause_) {
we don't need pause now, do we?

https://codereview.chromium.org/480293003/diff/100001/athena/wm/public/window...
File athena/wm/public/window_list_provider_observer.h (right):

https://codereview.chromium.org/480293003/diff/100001/athena/wm/public/window...
athena/wm/public/window_list_provider_observer.h:18: virtual void
OnActivityStackingChanged() = 0;
OnWindowOrderChanged. Let's not use activity in athena/wm

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:56: return window->type() ==
ui::wm::WINDOW_TYPE_NORMAL;
Note: This is OK for now, but I think this logic should be injected somehow in
the future.

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:83: if (!IsValidWindow(window) ||
window->parent() != container_)
IsWindowInList

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:89: if (!IsValidWindow(window) ||
window->parent() != container_)
ditto

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:95: if (window == container_)
ditto.

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
File athena/wm/window_list_provider_impl_unittest.cc (right):

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl_unittest.cc:44: output += (output.size() ? "
" : "") + std::to_string(i + 1);
s/""/std::string()/

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl_unittest.cc:54:
WindowListObserver(WindowListProvider* provider) : calls_(0),
explicit

[Mr4D (OOO till 08-26), 2014-09-10 00:01:59]

Rebased and addresse. Please have another look!

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
File athena/wm/public/window_list_provider.h (right):

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
athena/wm/public/window_list_provider.h:21: virtual aura::Window::Windows
GetCurrentWindowList() const = 0;
On 2014/09/09 18:28:37, oshima wrote:
> On 2014/09/08 17:35:07, Mr4D wrote:
> > The returned list will not change if the content changes. As such the list
> will
> > be an incorrect representation once it did change. I used "Current" to show
> that
> > this is capturing the "current" state - but in the future it might be
> incorrect.
> 
> That's same with any API that returns list (aura::Window::children(),
> shell::GetAllWindows() etc). I still think current is overkill.

Done.

https://codereview.chromium.org/480293003/diff/80001/athena/wm/public/window_...
athena/wm/public/window_list_provider.h:32: virtual void AddWindow(aura::Window*
window) = 0;
On 2014/09/09 18:28:37, oshima wrote:
> On 2014/09/08 17:35:07, Mr4D wrote:
> > Someone must have the knowledge of where to add an activity, and for the
sake
> of
> > knowledge encapsulation (what this is about) this knowledge should reside in
> one
> > place. Only this way we get a clear separation of the way how it is managed.
> > 
> > We might want to add this later back in again, but for the time being I have
> > changed it to observe window changes.
> > 
> 
> Adding is implicit now, so having AddWindow is confusing. I agree that we may
> need this when we redesign the list management. We can add it when we need it.

Acknowledged.

https://codereview.chromium.org/480293003/diff/80001/athena/wm/window_list_pr...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/480293003/diff/80001/athena/wm/window_list_pr...
athena/wm/window_list_provider_impl.cc:62: container_->StackChildAbove(window,
reference_window);
On 2014/09/09 18:28:37, oshima wrote:
> On 2014/09/08 17:35:07, Mr4D wrote:
> > Apparently something has changed in the behavior of
"Window::StackChild..()".
> I
> > added unit tests to make sure that it stays this way. Simplified things.
> 
> Hmm Ok. There is another way to control stacking, StackingClient. This is
> currently used to manage transient window, and we can extend
> TransientWindowStackingClient if that's better way.

Acknowledged.

https://codereview.chromium.org/480293003/diff/100001/athena/resource_manager...
File athena/resource_manager/resource_manager_impl.cc (right):

https://codereview.chromium.org/480293003/diff/100001/athena/resource_manager...
athena/resource_manager/resource_manager_impl.cc:200: if (pause_) {
It's used for unit tests (and it might come back for other things as well). We
can rename / remove once this stabelizes and only unittests need it.

https://codereview.chromium.org/480293003/diff/100001/athena/wm/public/window...
File athena/wm/public/window_list_provider_observer.h (right):

https://codereview.chromium.org/480293003/diff/100001/athena/wm/public/window...
athena/wm/public/window_list_provider_observer.h:18: virtual void
OnActivityStackingChanged() = 0;
On 2014/09/09 18:28:37, oshima wrote:
> OnWindowOrderChanged. Let's not use activity in athena/wm

Done.

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:56: return window->type() ==
ui::wm::WINDOW_TYPE_NORMAL;
On 2014/09/09 18:28:37, oshima wrote:
> Note: This is OK for now, but I think this logic should be injected somehow in
> the future.

Acknowledged.

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:83: if (!IsValidWindow(window) ||
window->parent() != container_)
On 2014/09/09 18:28:37, oshima wrote:
> IsWindowInList

Done.

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:89: if (!IsValidWindow(window) ||
window->parent() != container_)
On 2014/09/09 18:28:37, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:95: if (window == container_)
On 2014/09/09 18:28:37, oshima wrote:
> ditto.

Done.

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
File athena/wm/window_list_provider_impl_unittest.cc (right):

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl_unittest.cc:44: output += (output.size() ? "
" : "") + std::to_string(i + 1);
On 2014/09/09 18:28:38, oshima wrote:
> s/""/std::string()/

Done.

https://codereview.chromium.org/480293003/diff/100001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl_unittest.cc:54:
WindowListObserver(WindowListProvider* provider) : calls_(0),
On 2014/09/09 18:28:37, oshima wrote:
> explicit

Done.

[oshima, 2014-09-10 01:03:31]

lgtm with nits

https://codereview.chromium.org/480293003/diff/100001/athena/resource_manager...
File athena/resource_manager/resource_manager_impl.cc (right):

https://codereview.chromium.org/480293003/diff/100001/athena/resource_manager...
athena/resource_manager/resource_manager_impl.cc:200: if (pause_) {
On 2014/09/10 00:01:58, Mr4D wrote:
> It's used for unit tests (and it might come back for other things as well). We
> can rename / remove once this stabelizes and only unittests need it.

Acknowledged.

https://codereview.chromium.org/480293003/diff/120001/athena/wm/window_list_p...
File athena/wm/window_list_provider_impl_unittest.cc (right):

https://codereview.chromium.org/480293003/diff/120001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl_unittest.cc:44: output += (output.size() ?
std::string(" ") : std::string("")) +
" " can be just " ", but use std::string() instead of "".

https://codereview.chromium.org/480293003/diff/120001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl_unittest.cc:214: // Move 1 (from the back)
in front of 2.
in front of 3

[Mr4D (OOO till 08-26), 2014-09-10 03:20:55]

Thanks!

https://codereview.chromium.org/480293003/diff/120001/athena/wm/window_list_p...
File athena/wm/window_list_provider_impl_unittest.cc (right):

https://codereview.chromium.org/480293003/diff/120001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl_unittest.cc:44: output += (output.size() ?
std::string(" ") : std::string("")) +
On 2014/09/10 01:03:30, oshima wrote:
> " " can be just " ", but use std::string() instead of "".

Done.

https://codereview.chromium.org/480293003/diff/120001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl_unittest.cc:214: // Move 1 (from the back)
in front of 2.
On 2014/09/10 01:03:30, oshima wrote:
> in front of 3

Done.

[Mr4D (OOO till 08-26), 2014-09-10 03:21:21]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-09-10 03:26:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skuhne@chromium.org/480293003/140001

[commit-bot: I haz the power, 2014-09-10 04:48:23]

Committed patchset #8 (id:140001) as e42ff999d35e3e6bf22b91fa7672e43d8a8c838d

[commit-bot: I haz the power, 2014-09-10 04:53:32]

Patchset 8 (id:??) landed as
https://crrev.com/227733e374f946b4335ffd55938da4296057bedf
Cr-Commit-Position: refs/heads/master@{#294103}

[Dan Beam, 2014-12-11 21:33:51]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2014-12-11 21:33:57]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2014-12-11 22:28:31]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2014-12-11 22:28:32]

https://codereview.chromium.org/480293003/diff/140001/athena/wm/window_list_p...
File athena/wm/window_list_provider_impl_unittest.cc (right):

https://codereview.chromium.org/480293003/diff/140001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl_unittest.cc:45: std::to_string(i + 1);
are we allowed to use std::to_string() in chromium?

[Nico, 2014-12-11 22:33:34]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org