Mus Demo: Refactor code to prepare multiple windows in external mode

Mus Demo: Refactor code to prepare multiple windows in external mode

Support for external mode in Mus Demo was added in [1]. However, the
Mus Demo code is currently designed to work with a single
WindowTreeData. This CL refactors the code to be able to handle a
list of WindowTreeDatas instead. Actual support for multiple windows
will be handled in follow-up CLs.

BUG=666958
[1] https://codereview.chromium.org/2622103004/

Review-Url: https://codereview.chromium.org/2700493005
Cr-Commit-Position: refs/heads/master@{#452971}
Committed: https://chromium.googlesource.com/chromium/src/+/84358c00a81eab2e2ccc5485fbfe44ffc50307dc

[fwang, 2017-02-16 10:36:34]

Description was changed from

==========
[WIP] Mus Demo: Open multiple windows in external mode.

Support for external mode in Mus Demo was added in [1]. However, the
demo currently only opens a single ws::Display due do limitations in of
the WindowTreeHostFactory interface. This CL modifies that interface so
that it is possible to create multiple ws::Display using a single
WindowTree/WindowTreeClient. It also updates MusDemoExternal to be able
to open multiple windows.

BUG=666958

[1] https://codereview.chromium.org/2622103004/
==========

to

==========
[WIP] Mus Demo: Open multiple windows in external mode.

Support for external mode in Mus Demo was added in [1]. However, the
demo currently only opens a single ws::Display due do limitations in of
the WindowTreeHostFactory interface. This CL modifies that interface so
that it is possible to create multiple ws::Display using a single
WindowTree/WindowTreeClient. It also updates MusDemoExternal to be able
to open multiple windows.

BUG=666958

[1] https://codereview.chromium.org/2622103004/
==========

[fwang, 2017-02-22 11:02:23]

The CQ bit was checked by fwang@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 11:02:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 11:39:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 11:39:22]

Dry run: This issue passed the CQ dry run.

[fwang, 2017-02-22 14:09:51]

Description was changed from

==========
[WIP] Mus Demo: Open multiple windows in external mode.

Support for external mode in Mus Demo was added in [1]. However, the
demo currently only opens a single ws::Display due do limitations in of
the WindowTreeHostFactory interface. This CL modifies that interface so
that it is possible to create multiple ws::Display using a single
WindowTree/WindowTreeClient. It also updates MusDemoExternal to be able
to open multiple windows.

BUG=666958

[1] https://codereview.chromium.org/2622103004/
==========

to

==========
[WIP] Mus Demo: Open multiple windows in external mode.

Support for external mode in Mus Demo was added in [1]. However, the
demo currently only opens a single ws::Display due do limitations in of
the WindowTreeHostFactory interface. This updates MusDemoExternal to be
able to open multiple windows.

BUG=666958

[1] https://codereview.chromium.org/2622103004/
==========

[fwang, 2017-02-23 08:13:30]

Patchset #5 (id:80001) has been deleted

[fwang, 2017-02-23 08:21:59]

Description was changed from

==========
[WIP] Mus Demo: Open multiple windows in external mode.

Support for external mode in Mus Demo was added in [1]. However, the
demo currently only opens a single ws::Display due do limitations in of
the WindowTreeHostFactory interface. This updates MusDemoExternal to be
able to open multiple windows.

BUG=666958

[1] https://codereview.chromium.org/2622103004/
==========

to

==========
Mus Demo: Refactor code to prepare multiple windows in external mode

Support for external mode in Mus Demo was added in [1]. However, the
Mus Demo code is currently designed to work with a single
WindowTreeData. This CL refactors the code to be able to handle a
list of WindowTreeDatas instead. Actual support for multiple windows
will be handled in follow-up CLs.

BUG=666958

[1] https://codereview.chromium.org/2622103004/
==========

[fwang, 2017-02-23 08:25:29]

fwang@igalia.com changed reviewers:
+ kylechar@chromium.org

[fwang, 2017-02-23 08:25:30]

PTAL

This is only the mus-demo specific code. Multiple windows will likely need more
work in aura and ws (Antonio has started to work on it) so for now I'm keeping
kNumberOfWindows = 1. I'll open a separate CL for the mus-demo-unittest and
window-server-test.

[kylechar, 2017-02-23 22:45:30]

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:60: window_tree_data_list_.erase(it, it + 1);
You can just do: erase(it).

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.h:47: void
AppendWindowTreeData(std::unique_ptr<WindowTreeData> window_tree_data);
Can you walk me through the flow of how this is supposed to work?

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.h:50: void
RemoveWindowTreeDataFor(aura::WindowTreeHostMus* window_tree_host);
nit: RemoveWindowTreeData maybe?

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo_external.cc (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:44: const unsigned kNumberOfWindows = 1;
Same comment about type.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:46: static int GetSquareSizeForWindow(int
window_index) {
The static qualifier here isn't necessary due to the anonymous namespace.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:46: static int GetSquareSizeForWindow(int
window_index) {
I'm not sure I actually understand what this does? Is it just to have different
size squares on each window? You probably don't need the name constants then.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:64: //
window_tree_client()->ConnectViaWindowTreeHostFactory();
Delete commented out code.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:82: window_tree_host_factory_.get(),
std::move(tree_client_),
For two windows, you move this value then reuse it. I don't see it getting
reinitialized? That might be a problem, unless I missed something about how it's
used.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo_external.h (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.h:38: 
I would use int or maybe size_t if you really want it to be unsigned.

"Do not use unsigned types to mean “this value should never be < 0”. For that,
use assertions or run-time checks (as appropriate)."

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

[fwang, 2017-02-24 10:08:51]

Thank you for the feedback. More explanations inline. As you see, there are
follow-up work to do when we have the new API of
https://bugs.chromium.org/p/chromium/issues/detail?id=666958#c20. I think it is
worth keeping this separate CL to facilitate review, but I'm not sure whether it
should be taken after or before Antonio's work.

Anyway, I'll upload a new CL trying to fix comments.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:60: window_tree_data_list_.erase(it, it + 1);
On 2017/02/23 22:45:30, kylechar wrote:
> You can just do: erase(it).

Acknowledged.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.h:47: void
AppendWindowTreeData(std::unique_ptr<WindowTreeData> window_tree_data);
On 2017/02/23 22:45:30, kylechar wrote:
> Can you walk me through the flow of how this is supposed to work?

Sure. This is explained in a comment above window_tree_data_list_, in
MusDemoExternal::OnStartImpl and MusDemoExternal::OnEmbed. But to give more
details, we starts with an empty vector of WindowTreeData. OnStartImpl calls
AppendWindowTreeData to append one WindowTreeData, which makes a Mojo call to
CreateWindowTreeHost and gets an associated WindowTreeHostPtr. On the the ws
side, this causes the following execution stack:

ui::ws::WindowTree::Init()
ui::ws::WindowServer::AddTree()
ui::ws::WindowServer::EmbedAtWindow()
ui::ws::DisplayBindingImpl::CreateWindowTree()
ui::ws::Display::InitWindowManagerDisplayRoots()
ui::ws::Display::OnAcceleratedWidgetAvailable()
ui::ws::PlatformDisplayDefault::OnAcceleratedWidgetAvailable()
ui::X11WindowBase::Create()
ui::X11WindowBase::Show()
ui::ws::PlatformDisplayDefault::Init()
ui::ws::Display::Init()
ui::ws::WindowTreeHostFactory::CreateWindowTreeHost()

Then ui::ws::WindowTree::Init makes a Mojo call to OnEmbed back to the
aura::WindowTreeClient, which leads to the following execution stack:

ui::demo::MusDemoExternal::OnEmbed()
aura::WindowTreeClient::OnEmbedImpl()
aura::WindowTreeClient::OnEmbed()

Finally, OnEmbed() makes a call to InitWindowTreeData to associate the
WindowTreeHostMus to the last appended WindowTreeData. It calls
AppendWindowTreeData and the same process starts again with a second window, a
third window and so on until we have the desired number of windows.

Two important comments:

* This mechanism ensures that the WindowTreeData have matching WindowTreeHostPtr
and WindowTreeHostMus. It's not clear to me whether we actually want to keep the
Mojo pointer to WindowTreeHostMus (for example to test SetSize or SetTitle?). If
not, I believe we could just do all Mojo calls to CreateWindowTreeHost in one go
and do append+init (which would then just be one function) WindowTreeData in
OnEmbed. That would make things much simpler!

* As explained by sky, we need a single WindowClientTree/WindowTree for all all
the data otherwise this won't work for multiple windows. Antonio is working on a
new WindowClientTree API for that purpose
(https://bugs.chromium.org/p/chromium/issues/detail?id=666958#c20). When this is
ready, we could just do window_tree_client()->ConnectViaWindowTreeHostFactory()
and window_tree_client()->CreateWindowTreeHost(MakeRequest(&host_)). This will
allow to get rid of the window_tree_host_factory_ and tree_client_ Mojo
pointers.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.h:50: void
RemoveWindowTreeDataFor(aura::WindowTreeHostMus* window_tree_host);
On 2017/02/23 22:45:30, kylechar wrote:
> nit: RemoveWindowTreeData maybe?

Antonio suggested to change to RemoveWindowTreeDataFor, because it's not really
symmetric to AppendWindowTreeData(data). Either option will work for me.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo_external.cc (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:44: const unsigned kNumberOfWindows = 1;
On 2017/02/23 22:45:30, kylechar wrote:
> Same comment about type.

Acknowledged.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:46: static int GetSquareSizeForWindow(int
window_index) {
On 2017/02/23 22:45:30, kylechar wrote:
> I'm not sure I actually understand what this does? Is it just to have
different
> size squares on each window? You probably don't need the name constants then.

Yes, it's just to have different size squares on each window.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:46: static int GetSquareSizeForWindow(int
window_index) {
On 2017/02/23 22:45:30, kylechar wrote:
> I'm not sure I actually understand what this does? Is it just to have
different
> size squares on each window? You probably don't need the name constants then.

Acknowledged.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:64: //
window_tree_client()->ConnectViaWindowTreeHostFactory();
On 2017/02/23 22:45:30, kylechar wrote:
> Delete commented out code.

OK, will do that. As I said, the goal is to replace the BindInterface with
ConnectViaWindowTreeHostFactory when Antonio's API is ready.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:82: window_tree_host_factory_.get(),
std::move(tree_client_),
On 2017/02/23 22:45:30, kylechar wrote:
> For two windows, you move this value then reuse it. I don't see it getting
> reinitialized? That might be a problem, unless I missed something about how
it's
> used.

You are right. As sky said, this won't work with multiple windows, we really
need to change the API to have a single WindowTreeClient for all the windows.
Antonio is working on that. Then the WindowTreeDataExternal constructor would
take an aura::WindowTreeClient* (== window_tree_client()) instead of
mojom::WindowTreeHostFactory* + mojom::WindowTreeClientPtr. The host will then
just be created with WindowTreeClient::CreateHost(MakeRequest(&host_));

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo_external.h (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.h:38: 
On 2017/02/23 22:45:30, kylechar wrote:
> I would use int or maybe size_t if you really want it to be unsigned.
> 
> "Do not use unsigned types to mean “this value should never be < 0”. For that,
> use assertions or run-time checks (as appropriate)."
> 
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

Acknowledged.

[fwang, 2017-02-24 10:40:11]

PTAL

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.cc (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.cc:60: window_tree_data_list_.erase(it, it + 1);
On 2017/02/24 10:08:50, fwang wrote:
> On 2017/02/23 22:45:30, kylechar wrote:
> > You can just do: erase(it).
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.h:50: void
RemoveWindowTreeDataFor(aura::WindowTreeHostMus* window_tree_host);
On 2017/02/24 10:08:50, fwang wrote:
> On 2017/02/23 22:45:30, kylechar wrote:
> > nit: RemoveWindowTreeData maybe?
> 
> Antonio suggested to change to RemoveWindowTreeDataFor, because it's not
really
> symmetric to AppendWindowTreeData(data). Either option will work for me.

Done.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo_external.cc (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:44: const unsigned kNumberOfWindows = 1;
On 2017/02/24 10:08:51, fwang wrote:
> On 2017/02/23 22:45:30, kylechar wrote:
> > Same comment about type.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:46: static int GetSquareSizeForWindow(int
window_index) {
On 2017/02/24 10:08:51, fwang wrote:
> On 2017/02/23 22:45:30, kylechar wrote:
> > I'm not sure I actually understand what this does? Is it just to have
> different
> > size squares on each window? You probably don't need the name constants
then.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:64: //
window_tree_client()->ConnectViaWindowTreeHostFactory();
On 2017/02/24 10:08:51, fwang wrote:
> On 2017/02/23 22:45:30, kylechar wrote:
> > Delete commented out code.
> 
> OK, will do that. As I said, the goal is to replace the BindInterface with
> ConnectViaWindowTreeHostFactory when Antonio's API is ready.

Done.

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.cc:82: window_tree_host_factory_.get(),
std::move(tree_client_),
On 2017/02/24 10:08:51, fwang wrote:
> On 2017/02/23 22:45:30, kylechar wrote:
> > For two windows, you move this value then reuse it. I don't see it getting
> > reinitialized? That might be a problem, unless I missed something about how
> it's
> > used.
> 
> You are right. As sky said, this won't work with multiple windows, we really
> need to change the API to have a single WindowTreeClient for all the windows.
> Antonio is working on that. Then the WindowTreeDataExternal constructor would
> take an aura::WindowTreeClient* (== window_tree_client()) instead of
> mojom::WindowTreeHostFactory* + mojom::WindowTreeClientPtr. The host will then
> just be created with WindowTreeClient::CreateHost(MakeRequest(&host_));
> 
> 
> 

Done (added a comment to clarify).

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo_external.h (right):

https://codereview.chromium.org/2700493005/diff/100001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.h:38: 
On 2017/02/23 22:45:30, kylechar wrote:
> I would use int or maybe size_t if you really want it to be unsigned.
> 
> "Do not use unsigned types to mean “this value should never be < 0”. For that,
> use assertions or run-time checks (as appropriate)."
> 
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

Done.

[kylechar, 2017-02-24 21:14:04]

https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.h:47: void
AppendWindowTreeData(std::unique_ptr<WindowTreeData> window_tree_data);
Thanks for the explanation on how it's expected to work. Can you add some short
comments on the functions for when they are expected to be called? Like, it's
append, wait for embed, init right?

https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo_external.h (right):

https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.h:36: mojom::WindowTreeClientPtr
tree_client_;
Maybe change this name so it's obvious it's a mojo interface pointer vs the
other window_tree_client_ in MusDemo.

window_tree_client_mojo_?

[fwang, 2017-02-24 21:53:45]

https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo.h (right):

https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_d...
services/ui/demo/mus_demo.h:47: void
AppendWindowTreeData(std::unique_ptr<WindowTreeData> window_tree_data);
On 2017/02/24 21:14:04, kylechar wrote:
> Thanks for the explanation on how it's expected to work. Can you add some
short
> comments on the functions for when they are expected to be called? Like, it's
> append, wait for embed, init right?

Done. (actually my previous comment only described the MusExternal case, the
MusInternal is simpler since you only have one window and the append/init can be
done at the same time)

https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_d...
File services/ui/demo/mus_demo_external.h (right):

https://codereview.chromium.org/2700493005/diff/120001/services/ui/demo/mus_d...
services/ui/demo/mus_demo_external.h:36: mojom::WindowTreeClientPtr
tree_client_;
On 2017/02/24 21:14:04, kylechar wrote:
> Maybe change this name so it's obvious it's a mojo interface pointer vs the
> other window_tree_client_ in MusDemo.
> 
> window_tree_client_mojo_?

Done.

[kylechar, 2017-02-24 21:55:46]

lgtm

[fwang, 2017-02-24 22:01:01]

The CQ bit was checked by fwang@igalia.com

[commit-bot: I haz the power, 2017-02-24 22:02:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 22:51:41]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487973661047130,
"parent_rev": "31cc5805fa24ca2c9161ec1b93cc7fe3662d9739", "commit_rev":
"84358c00a81eab2e2ccc5485fbfe44ffc50307dc"}

[commit-bot: I haz the power, 2017-02-24 22:52:23]

Description was changed from

==========
Mus Demo: Refactor code to prepare multiple windows in external mode

Support for external mode in Mus Demo was added in [1]. However, the
Mus Demo code is currently designed to work with a single
WindowTreeData. This CL refactors the code to be able to handle a
list of WindowTreeDatas instead. Actual support for multiple windows
will be handled in follow-up CLs.

BUG=666958

[1] https://codereview.chromium.org/2622103004/
==========

to

==========
Mus Demo: Refactor code to prepare multiple windows in external mode

Support for external mode in Mus Demo was added in [1]. However, the
Mus Demo code is currently designed to work with a single
WindowTreeData. This CL refactors the code to be able to handle a
list of WindowTreeDatas instead. Actual support for multiple windows
will be handled in follow-up CLs.

BUG=666958

[1] https://codereview.chromium.org/2622103004/

Review-Url: https://codereview.chromium.org/2700493005
Cr-Commit-Position: refs/heads/master@{#452971}
Committed:
https://chromium.googlesource.com/chromium/src/+/84358c00a81eab2e2ccc5485fbfe...
==========

[commit-bot: I haz the power, 2017-02-24 22:52:25]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/84358c00a81eab2e2ccc5485fbfe...