mus: Add debug key to print the server window hierarchy

mus: Add debug key to print the server window hierarchy

This is similar to what we do in ash to dump the aura::Window and ui::Layer
hierarchies.

BUG=none
TEST=none

Committed: https://crrev.com/2e44082546a584c2d82b97fbeb16a0f2632d7f51
Cr-Commit-Position: refs/heads/master@{#392283}

[James Cook, 2016-05-05 22:02:47]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2016-05-05 22:02:47]

msw, please take a look

[msw, 2016-05-05 22:12:08]

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
File components/mus/ws/server_window.cc (right):

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
components/mus/ws/server_window.cc:353: return std::string(it->second.begin(),
it->second.end());
nit: use the property_type_converter?

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
components/mus/ws/server_window.cc:413: "%sid=%d,%d visible=%s bounds=%d,%d
%dx%d %s\n", depth.c_str(),
nit: maybe name='%s'?

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
File components/mus/ws/server_window.h (right):

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
components/mus/ws/server_window.h:146: std::string GetName() const;
nit: const std::string&?

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/window_ma...
File components/mus/ws/window_manager_state.cc (right):

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/window_ma...
components/mus/ws/window_manager_state.cc:341: void
WindowManagerState::RunDebugShortcuts(const ui::Event& event) {
Should this instead use accelerators, like ash/accelerators/debug_commands.h?
(even if it differs a bit, using accelerators seem more fitting here)

[James Cook, 2016-05-05 22:53:18]

msw, please take another look

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
File components/mus/ws/server_window.cc (right):

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
components/mus/ws/server_window.cc:353: return std::string(it->second.begin(),
it->second.end());
On 2016/05/05 22:12:07, msw wrote:
> nit: use the property_type_converter?

I can't, property_type_converters.cc is in components/mus/public/cpp and
components/mus/ws cannot depend on that, by design.

//components/mus/public/cpp has a data_dep on //components/mus, so you get a
circular dependency. I've talked to Scott about this in the past and he wants to
keep it, the theory being that anyone who depends on //components/mus/public/cpp
needs all of mus to be built/run, and also the /ws stuff is an implementation
detail of mus and hence shouldn't rely on code in /public.

If you really want the property converter I could try moving it to
components/mus/common.

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
components/mus/ws/server_window.cc:413: "%sid=%d,%d visible=%s bounds=%d,%d
%dx%d %s\n", depth.c_str(),
On 2016/05/05 22:12:08, msw wrote:
> nit: maybe name='%s'?

Done.

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
File components/mus/ws/server_window.h (right):

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
components/mus/ws/server_window.h:146: std::string GetName() const;
On 2016/05/05 22:12:08, msw wrote:
> nit: const std::string&?

I can't, the string is a stack variable inside the function, created by parsing
the window property key.

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/window_ma...
File components/mus/ws/window_manager_state.cc (right):

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/window_ma...
components/mus/ws/window_manager_state.cc:341: void
WindowManagerState::RunDebugShortcuts(const ui::Event& event) {
On 2016/05/05 22:12:08, msw wrote:
> Should this instead use accelerators, like ash/accelerators/debug_commands.h?
> (even if it differs a bit, using accelerators seem more fitting here)

It seems like overkill. Ash already has an accelerator registry, accelerator
dispatch code, etc. In mash, only the window manager has any of that, not the
window server. I doubt the window server itself will handle any shortcuts in
production. It seemed like overkill to build it just for debug stuff.

[msw, 2016-05-05 23:10:06]

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
File components/mus/ws/server_window.cc (right):

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
components/mus/ws/server_window.cc:353: return std::string(it->second.begin(),
it->second.end());
On 2016/05/05 22:53:18, James Cook wrote:
> On 2016/05/05 22:12:07, msw wrote:
> > nit: use the property_type_converter?
> 
> I can't, property_type_converters.cc is in components/mus/public/cpp and
> components/mus/ws cannot depend on that, by design.
> 
> //components/mus/public/cpp has a data_dep on //components/mus, so you get a
> circular dependency. I've talked to Scott about this in the past and he wants
to
> keep it, the theory being that anyone who depends on
//components/mus/public/cpp
> needs all of mus to be built/run, and also the /ws stuff is an implementation
> detail of mus and hence shouldn't rely on code in /public.
> 
> If you really want the property converter I could try moving it to
> components/mus/common.

Nah, I guess this is fine as-is.

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
File components/mus/ws/server_window.h (right):

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/server_wi...
components/mus/ws/server_window.h:146: std::string GetName() const;
On 2016/05/05 22:53:18, James Cook wrote:
> On 2016/05/05 22:12:08, msw wrote:
> > nit: const std::string&?
> 
> I can't, the string is a stack variable inside the function, created by
parsing
> the window property key.

Acknowledged.

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/window_ma...
File components/mus/ws/window_manager_state.cc (right):

https://codereview.chromium.org/1953193002/diff/1/components/mus/ws/window_ma...
components/mus/ws/window_manager_state.cc:341: void
WindowManagerState::RunDebugShortcuts(const ui::Event& event) {
On 2016/05/05 22:53:18, James Cook wrote:
> On 2016/05/05 22:12:08, msw wrote:
> > Should this instead use accelerators, like
ash/accelerators/debug_commands.h?
> > (even if it differs a bit, using accelerators seem more fitting here)
> 
> It seems like overkill. Ash already has an accelerator registry, accelerator
> dispatch code, etc. In mash, only the window manager has any of that, not the
> window server. I doubt the window server itself will handle any shortcuts in
> production. It seemed like overkill to build it just for debug stuff.

See if it's easy to add an accelerator to the EventDispatcher for which this is
a delegate, and then handle it in OnAccelerator. If it's tough, this is fine.

[James Cook, 2016-05-06 21:59:10]

msw, please take another look

I switched to use an Accelerator. This has two side effects:
1. I have to consume the event (because event dispatch stops when an accelerator
is matched).
2. I need to reserve some accelerator ids (because the uint32 id space is
shared).

Because of (2) I always set up the accelerators, even in debug builds, so that
client apps that try to register IDs that are already reserved will fail in both
release and debug.

[msw, 2016-05-06 22:34:31]

lgtm with a nit and a number change.

https://codereview.chromium.org/1953193002/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/1953193002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/window_manager.mojom:84: // Accelerator ID 1 <<
24 and above are reserved for internal use.
The int32 space is 4,294,967,295, this would reserve a disproportionately large
block of that for debug accelerators (a total of 4,293,290,080 numbers are
greater than 16,777,216). Maybe use numbers 1 << 31 and above for debug, that
reserves half the available space, which still seems unnecessarily large (for
now, time will tell...), but also supports an equally unnecessarily large space
of numbers for actual accelerators.

https://codereview.chromium.org/1953193002/diff/40001/components/mus/ws/windo...
File components/mus/ws/window_manager_state.h (right):

https://codereview.chromium.org/1953193002/diff/40001/components/mus/ws/windo...
components/mus/ws/window_manager_state.h:153: void AddDebugAccelerators();
nit: please add a simple comment

[James Cook, 2016-05-07 00:03:10]

https://codereview.chromium.org/1953193002/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/1953193002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/window_manager.mojom:84: // Accelerator ID 1 <<
24 and above are reserved for internal use.
On 2016/05/06 22:34:31, msw wrote:
> The int32 space is 4,294,967,295, this would reserve a disproportionately
large
> block of that for debug accelerators (a total of 4,293,290,080 numbers are
> greater than 16,777,216). Maybe use numbers 1 << 31 and above for debug, that
> reserves half the available space, which still seems unnecessarily large (for
> now, time will tell...), but also supports an equally unnecessarily large
space
> of numbers for actual accelerators.

Done.

https://codereview.chromium.org/1953193002/diff/40001/components/mus/ws/windo...
File components/mus/ws/window_manager_state.h (right):

https://codereview.chromium.org/1953193002/diff/40001/components/mus/ws/windo...
components/mus/ws/window_manager_state.h:153: void AddDebugAccelerators();
On 2016/05/06 22:34:31, msw wrote:
> nit: please add a simple comment

Done.

[James Cook, 2016-05-08 22:49:56]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-05-08 22:49:57]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1953193002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-08 22:50:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1953193002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1953193002/100001

[commit-bot: I haz the power, 2016-05-08 23:25:52]

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

[commit-bot: I haz the power, 2016-05-08 23:25:54]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2016-05-09 01:54:08]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-05-09 01:54:08]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1953193002/#ps120001
(title: "uint32 for win")

[commit-bot: I haz the power, 2016-05-09 01:54:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1953193002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1953193002/120001

[commit-bot: I haz the power, 2016-05-09 02:45:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-09 02:46:59]

Description was changed from

==========
mus: Add debug key to print the server window hierarchy

This is similar to what we do in ash to dump the aura::Window and ui::Layer
hierarchies.

BUG=none
TEST=none
==========

to

==========
mus: Add debug key to print the server window hierarchy

This is similar to what we do in ash to dump the aura::Window and ui::Layer
hierarchies.

BUG=none
TEST=none

Committed: https://crrev.com/2e44082546a584c2d82b97fbeb16a0f2632d7f51
Cr-Commit-Position: refs/heads/master@{#392283}
==========

[commit-bot: I haz the power, 2016-05-09 02:46:59]

Patchset 7 (id:??) landed as
https://crrev.com/2e44082546a584c2d82b97fbeb16a0f2632d7f51
Cr-Commit-Position: refs/heads/master@{#392283}