Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2596723002:
mus: Unset the cursor client during teardown. (Closed)
Descriptionmus: Unset the cursor client during teardown.
DesktopWindowTreeHostMus owns the cursor client. So it is destroyed before
the ~WindowTreeHostMus is called. But the WindowTreeHostMus teardown can
try to access the cursor client. So it is necessary to unset the cursor
client on the root-window.
BUG=676184
Committed: https://crrev.com/d67bfeade6999acc5160588e8b755a6374bb6a4f
Cr-Commit-Position: refs/heads/master@{#440283}
Patch Set 1 #
Total comments: 2
Patch Set 2 : test #
Total comments: 2
Patch Set 3 : - #
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sadrul@chromium.org changed reviewers: + jamescook@chromium.org
[ working on adding a test for this too ]
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nit. A test would be nice. Feel free to do that in another CL -- I'm OOO starting tomorrow so I won't be around to review it. https://codereview.chromium.org/2596723002/diff/1/ui/views/mus/desktop_window... File ui/views/mus/desktop_window_tree_host_mus.cc (right): https://codereview.chromium.org/2596723002/diff/1/ui/views/mus/desktop_window... ui/views/mus/desktop_window_tree_host_mus.cc:199: aura::client::SetCursorClient(window(), nullptr); nit: Should this be done before line 193 so that things are torn down in the opposite order to their initialization?
sadrul@chromium.org changed reviewers: + msw@chromium.org
+msw@ for the test This CL (specifically, the test) requires https://codereview.chromium.org/2596903002/ to land first. I can split out the test into a separate CL if we want to land the fix first to unblock something. https://codereview.chromium.org/2596723002/diff/1/ui/views/mus/desktop_window... File ui/views/mus/desktop_window_tree_host_mus.cc (right): https://codereview.chromium.org/2596723002/diff/1/ui/views/mus/desktop_window... ui/views/mus/desktop_window_tree_host_mus.cc:199: aura::client::SetCursorClient(window(), nullptr); On 2016/12/21 04:02:31, James Cook (OOO until Jan 4) wrote: > nit: Should this be done before line 193 so that things are torn down in the > opposite order to their initialization? Done.
rubber stamp lgtm https://codereview.chromium.org/2596723002/diff/20001/ui/views/mus/desktop_wi... File ui/views/mus/desktop_window_tree_host_mus.cc (right): https://codereview.chromium.org/2596723002/diff/20001/ui/views/mus/desktop_wi... ui/views/mus/desktop_window_tree_host_mus.cc:194: // the cursor client needs to be unset on the root-window before nit: be consistent with hyphen use (or not) for 'cursor client'.
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2596723002/diff/20001/ui/views/mus/desktop_wi... File ui/views/mus/desktop_window_tree_host_mus.cc (right): https://codereview.chromium.org/2596723002/diff/20001/ui/views/mus/desktop_wi... ui/views/mus/desktop_window_tree_host_mus.cc:194: // the cursor client needs to be unset on the root-window before On 2016/12/21 18:06:52, msw wrote: > nit: be consistent with hyphen use (or not) for 'cursor client'. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sadrul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2596723002/#ps40001 (title: "-")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482366870076770,
"parent_rev": "345a6b1e9f9e7112ed9162e69785e2c97fb38e00", "commit_rev":
"03d52b265914061bc298337f160b51b493e1f62c"}
Message was sent while issue was closed.
Description was changed from ========== mus: Unset the cursor client during teardown. DesktopWindowTreeHostMus owns the cursor client. So it is destroyed before the ~WindowTreeHostMus is called. But the WindowTreeHostMus teardown can try to access the cursor client. So it is necessary to unset the cursor client on the root-window. BUG=676184 ========== to ========== mus: Unset the cursor client during teardown. DesktopWindowTreeHostMus owns the cursor client. So it is destroyed before the ~WindowTreeHostMus is called. But the WindowTreeHostMus teardown can try to access the cursor client. So it is necessary to unset the cursor client on the root-window. BUG=676184 Review-Url: https://codereview.chromium.org/2596723002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== mus: Unset the cursor client during teardown. DesktopWindowTreeHostMus owns the cursor client. So it is destroyed before the ~WindowTreeHostMus is called. But the WindowTreeHostMus teardown can try to access the cursor client. So it is necessary to unset the cursor client on the root-window. BUG=676184 Review-Url: https://codereview.chromium.org/2596723002 ========== to ========== mus: Unset the cursor client during teardown. DesktopWindowTreeHostMus owns the cursor client. So it is destroyed before the ~WindowTreeHostMus is called. But the WindowTreeHostMus teardown can try to access the cursor client. So it is necessary to unset the cursor client on the root-window. BUG=676184 Committed: https://crrev.com/d67bfeade6999acc5160588e8b755a6374bb6a4f Cr-Commit-Position: refs/heads/master@{#440283} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d67bfeade6999acc5160588e8b755a6374bb6a4f Cr-Commit-Position: refs/heads/master@{#440283} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
