Wires up modality for aura-mus

Wires up modality for aura-mus

BUG=660073, 659155
TEST=covered by tests

Committed: https://crrev.com/d435b56642e79cbcc6ace91568a62c692cc9958a
Cr-Commit-Position: refs/heads/master@{#428171}

[sky, 2016-10-27 17:21:02]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[sky, 2016-10-27 17:21:07]

Description was changed from

==========
Wires up modality for aura-mus

BUG=660073, 659155
TEST=covered by tests
==========

to

==========
Wires up modality for aura-mus

BUG=660073, 659155
TEST=covered by tests
==========

[sky, 2016-10-27 17:21:08]

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

[commit-bot: I haz the power, 2016-10-27 17:21:23]

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, 2016-10-27 18:21:56]

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

[commit-bot: I haz the power, 2016-10-27 18:22:02]

Dry run: This issue passed the CQ dry run.

[msw, 2016-10-27 18:37:37]

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/in_flight_c...
File ui/aura/mus/in_flight_change.cc (right):

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/in_flight_c...
ui/aura/mus/in_flight_change.cc:235:
window()->GetWindow()->SetProperty(client::kModalKey, ui::MODAL_TYPE_NONE);
Sorry, I'm not sure how InFlightChange typically works. Should this apply some
previous value instead of hard-coding ui::MODAL_TYPE_NONE?

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:508: if (key != client::kModalKey)
q: check !IsInternalProperty(key) for extensibility, then put the other code in
a if (key == client::kModalKey) check? Feel free to ignore for now and just
address this when we add other internal properties.

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:513: return true;
Should this be clearing modality if the window was previously modal?

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:747: if (HandleInternalPropertyChanged(window,
key))
optional nit: combine with the !data early return below.

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client_unittest.cc:1051:
TEST_F(WindowTreeClientClientTest, Modal) {
nit: test a successful change, sequential changes, pass-then-fail?

[sky, 2016-10-27 19:29:24]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[sky, 2016-10-27 19:30:06]

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/in_flight_c...
File ui/aura/mus/in_flight_change.cc (right):

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/in_flight_c...
ui/aura/mus/in_flight_change.cc:235:
window()->GetWindow()->SetProperty(client::kModalKey, ui::MODAL_TYPE_NONE);
On 2016/10/27 18:37:37, msw wrote:
> Sorry, I'm not sure how InFlightChange typically works. Should this apply some
> previous value instead of hard-coding ui::MODAL_TYPE_NONE?

Currently mus modality is a an on/off (see the TODO). Once we expand that this
will need to be updated. I added a TODO here.

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:508: if (key != client::kModalKey)
On 2016/10/27 18:37:37, msw wrote:
> q: check !IsInternalProperty(key) for extensibility, then put the other code
in
> a if (key == client::kModalKey) check? Feel free to ignore for now and just
> address this when we add other internal properties.

Ya, I figured once we have more than one property we'll do that.

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:513: return true;
On 2016/10/27 18:37:37, msw wrote:
> Should this be clearing modality if the window was previously modal?

Yes, but again because the mojom API is basically only turn on, it can't work.
Added TODO.

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:747: if (HandleInternalPropertyChanged(window,
key))
On 2016/10/27 18:37:37, msw wrote:
> optional nit: combine with the !data early return below.

Done.

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client_unittest.cc (right):

https://codereview.chromium.org/2454973003/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client_unittest.cc:1051:
TEST_F(WindowTreeClientClientTest, Modal) {
On 2016/10/27 18:37:37, msw wrote:
> nit: test a successful change, sequential changes, pass-then-fail?

I added a successful test. Sequential doesn't really work, nor does
pass-then-fail.

[commit-bot: I haz the power, 2016-10-27 19:30:11]

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, 2016-10-27 20:27:34]

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

[commit-bot: I haz the power, 2016-10-27 20:27:35]

Dry run: This issue passed the CQ dry run.

[msw, 2016-10-27 21:50:58]

lgtm

[sky, 2016-10-27 22:16:19]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-10-27 22:17:38]

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, 2016-10-27 22:46:10]

Description was changed from

==========
Wires up modality for aura-mus

BUG=660073, 659155
TEST=covered by tests
==========

to

==========
Wires up modality for aura-mus

BUG=660073, 659155
TEST=covered by tests
==========

[commit-bot: I haz the power, 2016-10-27 22:46:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-27 22:48:04]

Description was changed from

==========
Wires up modality for aura-mus

BUG=660073, 659155
TEST=covered by tests
==========

to

==========
Wires up modality for aura-mus

BUG=660073, 659155
TEST=covered by tests

Committed: https://crrev.com/d435b56642e79cbcc6ace91568a62c692cc9958a
Cr-Commit-Position: refs/heads/master@{#428171}
==========

[commit-bot: I haz the power, 2016-10-27 22:48:05]

Patchset 3 (id:??) landed as
https://crrev.com/d435b56642e79cbcc6ace91568a62c692cc9958a
Cr-Commit-Position: refs/heads/master@{#428171}