Chromium Code Reviews
Help | Chromium Project | Sign in
(4)

Issue 2745143004: Inform window manager about modal windows in mus+ash. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 2 days ago by Hadi
Modified:
1 week, 3 days ago
Reviewers:
Tom Sepez, sadrul, sky
CC:
chromium-reviews, rjkroege, sadrul, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, kalyank, darin (slow to review)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Inform window manager about modal windows in mus+ash. We need to inform window manager about modal windows. This is necessary because for example SystemModalContainerLayoutManager needs to know about modal types so it can create the fullscreen grey background for system modals. After this change we have some of the WM effects for modal windows (e.g. animating system modal windows when clicked outside of it), but for the rest we need more wiring (e.g. no grey background yet) which will be done in subsequent CLs. BUG=645996 Review-Url: https://codereview.chromium.org/2745143004 Cr-Commit-Position: refs/heads/master@{#458065} Committed: https://chromium.googlesource.com/chromium/src/+/edab7eb543505b23c3618767d40e2aa4f4ddeb19

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Total comments: 6

Patch Set 4 : Addressed feedback. #

Patch Set 5 : Added a test. #

Total comments: 1

Patch Set 6 : Addressed feedback. #

Patch Set 7 : rebased. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -2 lines) Patch
M services/ui/public/interfaces/window_manager.mojom View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
M services/ui/ws/test_utils.h View 1 2 3 4 5 6 3 chunks +3 lines, -0 lines 0 comments Download
M services/ui/ws/test_utils.cc View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M services/ui/ws/window_tree.cc View 1 2 3 4 5 6 2 chunks +24 lines, -2 lines 0 comments Download
M services/ui/ws/window_tree_client_unittest.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M services/ui/ws/window_tree_unittest.cc View 1 2 3 4 5 6 1 chunk +33 lines, -0 lines 0 comments Download
M ui/aura/mus/window_tree_client.h View 1 2 3 4 5 6 2 chunks +2 lines, -0 lines 0 comments Download
M ui/aura/mus/window_tree_client.cc View 1 2 3 4 5 6 1 chunk +6 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 47 (34 generated)
Hadi
PTAL. How do we write tests for changes like this? I couldn't find tests for ...
2 weeks, 1 day ago (2017-03-14 19:38:46 UTC) #8
sky
On 2017/03/14 19:38:46, Hadi wrote: > PTAL. > > How do we write tests for ...
2 weeks, 1 day ago (2017-03-14 21:15:26 UTC) #14
sky
https://codereview.chromium.org/2745143004/diff/40001/services/ui/ws/window_tree.cc File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2745143004/diff/40001/services/ui/ws/window_tree.cc#newcode1298 services/ui/ws/window_tree.cc:1298: WindowTree* wm_tree = GetWindowManagerDisplayRoot(window) As ash always allows the ...
2 weeks, 1 day ago (2017-03-14 21:16:23 UTC) #15
Hadi
Addressed feedback and added a test to window_tree_unittest.cc to check that it forwards the change ...
1 week, 6 days ago (2017-03-17 16:16:32 UTC) #22
Hadi
tsepez@ can you please review window_manager.mojom? Thanks.
1 week, 6 days ago (2017-03-17 16:18:43 UTC) #24
Tom Sepez
lgtm
1 week, 5 days ago (2017-03-17 17:03:12 UTC) #27
sky
LGTM https://codereview.chromium.org/2745143004/diff/80001/services/ui/ws/window_tree_unittest.cc File services/ui/ws/window_tree_unittest.cc (right): https://codereview.chromium.org/2745143004/diff/80001/services/ui/ws/window_tree_unittest.cc#newcode1400 services/ui/ws/window_tree_unittest.cc:1400: TestWindowTreeBinding* child_binding; = nullptr
1 week, 5 days ago (2017-03-17 19:07:06 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2745143004/100001
1 week, 5 days ago (2017-03-17 19:10:52 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/386109)
1 week, 5 days ago (2017-03-17 20:31:42 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2745143004/100001
1 week, 5 days ago (2017-03-18 05:14:21 UTC) #39
commit-bot: I haz the power
Failed to apply patch for ui/aura/mus/window_tree_client.h: While running git apply --index -p1; error: patch failed: ...
1 week, 5 days ago (2017-03-18 07:30:04 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2745143004/120001
1 week, 3 days ago (2017-03-20 13:17:25 UTC) #44
commit-bot: I haz the power
1 week, 3 days ago (2017-03-20 15:16:25 UTC) #47
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/edab7eb543505b23c3618767d40e...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46