Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2626013005:
Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. (Closed)
DescriptionChange CaptureSynchronizer and PointerWatcherEventRouter to support
multiple CaptureClients.
DesktopNativeWidgetAura was using DesktopCaptureClient but
CaptureSynchronizer was only supporting one CaptureClient, which is
a CaptureController. Therefore made the following changes:
1. We have one CaptureController for ash and multiple
DesktopCaptureClients for all mus clients. This CL changes
CaptureSynchronizer and PointerWatcherEventRouter to support multiple
CaptureClients. When a root window's capture client is set, MusClient
will be notified by DesktopWindowTreeHostMus if it's a
DesktopCaptureClient, and WindowManager will take care of
CaptureController. We then add CaptureSynchronizer and
PointerWatcherEventRouter to be the observer for that CaptureClient.
2. Update DesktopCaptureClient to notify its observers when capture
has changed.
BUG=676002, 678057
TEST=aura_unittests views_mus_unittests
Review-Url: https://codereview.chromium.org/2626013005
Cr-Original-Commit-Position: refs/heads/master@{#447621}
Committed: https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a9417582f14ebc3f161
Review-Url: https://codereview.chromium.org/2626013005
Cr-Commit-Position: refs/heads/master@{#447790}
Committed: https://chromium.googlesource.com/chromium/src/+/f6a5c39ae29d703bb4ecc6663477de3f9d3284a6
Patch Set 1 #
Total comments: 2
Patch Set 2 : another approach to attach capture client #
Total comments: 14
Patch Set 3 : another approach + PointerWatcherEventRouter + ImmersiveContextMus #Patch Set 4 : separate changes to ImmersiveContextMus into another CL #Patch Set 5 : separate out changes for removing GetCaptureClient() #
Total comments: 5
Patch Set 6 : comments; sequence; test #
Total comments: 7
Patch Set 7 : comment #Patch Set 8 : rebase #Patch Set 9 : test #Patch Set 10 : fix leak in test #Messages
Total messages: 48 (27 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== . BUG= ========== to ========== Change CaptureSynchronizer to support multiple CaptureClients. 1. Change CaptureSynchronizer to support multiple CaptureClients. When a root window's capture client is set, we attach the capture client (i.e. add CaptureSynchronizer as its observer). 2. Update DesktopCaptureClient to notify its observers when capture has changed. This is the client that DesktopNativeWidgetAura uses. BUG=676002, 678057 TEST=aura_unittests ==========
riajiang@chromium.org changed reviewers: + sadrul@chromium.org
This CL fixes the bug (for most of the times) where DesktopNativeWidgetAura (e.g. context menu, app menu) can't disappear. Sometimes it still doesn't disappear, and it seems like it's the same reason for this bug crbug.com/676015 where when chrome (ash) should have the capture window but instead ash (chrome) have the capture window. sadrul@, could you take a look at this part first? I'll try to fix the other bug later. Thank you!
https://codereview.chromium.org/2626013005/diff/20001/ui/aura/mus/capture_syn... File ui/aura/mus/capture_synchronizer.cc (right): https://codereview.chromium.org/2626013005/diff/20001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.cc:57: Attach(client::GetCaptureClient(window)); So, this sort of suck a little bit. I know this is what we suggested when talking offline. But can you think of something better? like, maybe the CaptureSynchronizer can be created *after* we know that the window has a capture-client set?
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2626013005/diff/20001/ui/aura/mus/capture_syn... File ui/aura/mus/capture_synchronizer.cc (right): https://codereview.chromium.org/2626013005/diff/20001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.cc:57: Attach(client::GetCaptureClient(window)); On 2017/01/18 22:14:44, sadrul wrote: > So, this sort of suck a little bit. I know this is what we suggested when > talking offline. But can you think of something better? like, maybe the > CaptureSynchronizer can be created *after* we know that the window has a > capture-client set? As discussed offline, CaptureSynchronizer has to be created before windows and their capture clients get created so updated the approach to be (still similar to previous approach): handling capture client setting as a special case in WindowTreeClient::HandleInternalPropertyChanged and then attach CaptureSynchronizer as the observer for that capture client.
https://codereview.chromium.org/2626013005/diff/60001/ui/aura/client/capture_... File ui/aura/client/capture_client.h (right): https://codereview.chromium.org/2626013005/diff/60001/ui/aura/client/capture_... ui/aura/client/capture_client.h:55: kRootWindowCaptureClientKey; Can you move this into aura_constants.h? since the other exported properties are also in there. https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... File ui/aura/mus/capture_synchronizer.cc (right): https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.cc:35: capture_window_->GetWindow()->GetRootWindow()); If capture-client for |window| is different from capture-client for |capture_window_|, then should you call SetCapture() on both of them? (although it looks like DesktopCaptureClient::SetCapture() takes care of notifying other capture-clients, so what you have is probably OK). https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.cc:40: if (capture_client) When can this be null? https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.cc:41: capture_client->AddObserver(this); Document why we never need to RemoveObserver() from any clients. https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.cc:48: if (capture_window_ && !capture_window_->GetWindow()->HasObserver(this)) Should not need this change anymore? https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.cc:58: } ditto https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... File ui/aura/mus/capture_synchronizer.h (right): https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.h:43: void Attach(client::CaptureClient* capture_client); Document. https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:229: } Remove this (and WindowTreeClientDelegate::GetCaptureClient()).
Description was changed from ========== Change CaptureSynchronizer to support multiple CaptureClients. 1. Change CaptureSynchronizer to support multiple CaptureClients. When a root window's capture client is set, we attach the capture client (i.e. add CaptureSynchronizer as its observer). 2. Update DesktopCaptureClient to notify its observers when capture has changed. This is the client that DesktopNativeWidgetAura uses. BUG=676002, 678057 TEST=aura_unittests ========== to ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. Add a static function GetCaptureWindowGlobal() to DesktopCaptureClient so that ImmersiveContextMus can use it. 3. Update ImmersiveContextMus to use DesktopCaptureClient to get global capture window. 4. Removed WindowTreeClient::GetCaptureClient() and WindowTreeClientDelegate::GetCaptureClient() (virtual function). BUG=676002, 678057 TEST=aura_unittests ==========
Hi sadrul@, changed to use another approach and updated PointerWatcherEventRouter etc. CL description has detailed changes. Could you take another look? Thanks! https://codereview.chromium.org/2626013005/diff/60001/ui/aura/client/capture_... File ui/aura/client/capture_client.h (right): https://codereview.chromium.org/2626013005/diff/60001/ui/aura/client/capture_... ui/aura/client/capture_client.h:55: kRootWindowCaptureClientKey; On 2017/01/21 03:23:37, sadrul wrote: > Can you move this into aura_constants.h? since the other exported properties are > also in there. The key is not needed anymore so deleted here. https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... File ui/aura/mus/capture_synchronizer.cc (right): https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.cc:35: capture_window_->GetWindow()->GetRootWindow()); On 2017/01/21 03:23:37, sadrul wrote: > If capture-client for |window| is different from capture-client for > |capture_window_|, then should you call SetCapture() on both of them? (although > it looks like DesktopCaptureClient::SetCapture() takes care of notifying other > capture-clients, so what you have is probably OK). I thought if a window is going to lose capture, ReleaseCapture would be called for this window and then SetCapture would be called on the new window? Therefore capture-client for |window| is never going to be different from capture-client for |capture_window_|? https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.cc:48: if (capture_window_ && !capture_window_->GetWindow()->HasObserver(this)) On 2017/01/21 03:23:37, sadrul wrote: > Should not need this change anymore? Done. https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.cc:58: } On 2017/01/21 03:23:37, sadrul wrote: > ditto Don't need this anymore. Deleted. https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... File ui/aura/mus/capture_synchronizer.h (right): https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/capture_syn... ui/aura/mus/capture_synchronizer.h:43: void Attach(client::CaptureClient* capture_client); On 2017/01/21 03:23:37, sadrul wrote: > Document. Moved this functionality into WindowTreeClient. https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/window_tree... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2626013005/diff/60001/ui/aura/mus/window_tree... ui/aura/mus/window_tree_client.cc:229: } On 2017/01/21 03:23:37, sadrul wrote: > Remove this (and WindowTreeClientDelegate::GetCaptureClient()). Removed these two and updated use cases.
Description was changed from ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. Add a static function GetCaptureWindowGlobal() to DesktopCaptureClient so that ImmersiveContextMus can use it. 3. Update ImmersiveContextMus to use DesktopCaptureClient to get global capture window. 4. Removed WindowTreeClient::GetCaptureClient() and WindowTreeClientDelegate::GetCaptureClient() (virtual function). BUG=676002, 678057 TEST=aura_unittests ========== to ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. 3. Remove WindowTreeClient::GetCaptureClient() and WindowTreeClientDelegate::GetCaptureClient() (virtual function). 4. Add a static function GetCaptureWindowGlobal() to DesktopCaptureClient so that ImmersiveContextMus can use it to get the global capture window. This is separated out to CL https://codereview.chromium.org/2652033003. BUG=676002, 678057 TEST=aura_unittests ==========
Description was changed from ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. 3. Remove WindowTreeClient::GetCaptureClient() and WindowTreeClientDelegate::GetCaptureClient() (virtual function). 4. Add a static function GetCaptureWindowGlobal() to DesktopCaptureClient so that ImmersiveContextMus can use it to get the global capture window. This is separated out to CL https://codereview.chromium.org/2652033003. BUG=676002, 678057 TEST=aura_unittests ========== to ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. 3. Remove WindowTreeClient::GetCaptureClient() and WindowTreeClientDelegate::GetCaptureClient() (virtual function). This is separated out to CL https://codereview.chromium.org/2657873006. 4. Add a static function GetCaptureWindowGlobal() to DesktopCaptureClient so that ImmersiveContextMus can use it to get the global capture window. This is separated out to CL https://codereview.chromium.org/2652033003. BUG=676002, 678057 TEST=aura_unittests ==========
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Can you add some tests for the views side of the changes? https://codereview.chromium.org/2626013005/diff/160001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2626013005/diff/160001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:840: capture_client->RemoveObserver(capture_synchronizer_.get()); Can this be replaced by adding functions on CaptureSynchronizer instead? e.g. in OnCaptureClientSet() above: capture_synchronizer_->AttachToCaptureClient(capture_client); and here: capture_synchronizer_->DetachFromCaptureClient(capture_client); The CaptureSynchronizer can take care of updating its internal state, instead of having to do that from here (the call to SetCaptureWindow(nullptr)) https://codereview.chromium.org/2626013005/diff/160001/ui/views/mus/mus_clien... File ui/views/mus/mus_client.cc (right): https://codereview.chromium.org/2626013005/diff/160001/ui/views/mus/mus_clien... ui/views/mus/mus_client.cc:227: window_tree_client_->OnCaptureClientSet(capture_client); Maybe do: pointer_watcher_event_router_->AttachToClient(capture_client); window_tree_client_->capture_synchronizer()->AttachToClient(capture_client); I feel like that would make this simpler. https://codereview.chromium.org/2626013005/diff/160001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_capture_client.cc (right): https://codereview.chromium.org/2626013005/diff/160001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_capture_client.cc:90: for (aura::client::CaptureClientObserver& observer : observers_) for (auto& observer : observers_)
Also, thanks a lot for splitting up the CL into smaller chunks! Can you update the CL desc. to mention only the remaining changes in this CL?
Description was changed from ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. 3. Remove WindowTreeClient::GetCaptureClient() and WindowTreeClientDelegate::GetCaptureClient() (virtual function). This is separated out to CL https://codereview.chromium.org/2657873006. 4. Add a static function GetCaptureWindowGlobal() to DesktopCaptureClient so that ImmersiveContextMus can use it to get the global capture window. This is separated out to CL https://codereview.chromium.org/2652033003. BUG=676002, 678057 TEST=aura_unittests ========== to ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests ==========
Also added tests and updated descriptions. PTAL, thanks! https://codereview.chromium.org/2626013005/diff/160001/ui/views/mus/mus_clien... File ui/views/mus/mus_client.cc (right): https://codereview.chromium.org/2626013005/diff/160001/ui/views/mus/mus_clien... ui/views/mus/mus_client.cc:227: window_tree_client_->OnCaptureClientSet(capture_client); On 2017/01/27 01:06:54, sadrul wrote: > Maybe do: > > pointer_watcher_event_router_->AttachToClient(capture_client); > window_tree_client_->capture_synchronizer()->AttachToClient(capture_client); > > I feel like that would make this simpler. Sure that makes sense. Changed to expose capture_synchronizer from WindowTreeClient and call that directly. But had to make CaptureSynchronizer AURA_EXPORT. https://codereview.chromium.org/2626013005/diff/160001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_capture_client.cc (right): https://codereview.chromium.org/2626013005/diff/160001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_capture_client.cc:90: for (aura::client::CaptureClientObserver& observer : observers_) On 2017/01/27 01:06:54, sadrul wrote: > for (auto& observer : observers_) Done.
Description was changed from ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests ========== to ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests views_mus_unittests ==========
lgtm https://codereview.chromium.org/2626013005/diff/180001/ui/aura/mus/capture_sy... File ui/aura/mus/capture_synchronizer.cc (right): https://codereview.chromium.org/2626013005/diff/180001/ui/aura/mus/capture_sy... ui/aura/mus/capture_synchronizer.cc:27: // Don't immediately set capture client. It's possible the change will be We do not set the capture client anymore (since we don't track a single capture client). Maybe something like: """Don't immediately set |capture_window_|. It's possible the change will be rejected. |capture_window_| is set in OnCaptureChanged() if capture succeeds.""" https://codereview.chromium.org/2626013005/diff/180001/ui/aura/mus/capture_sy... ui/aura/mus/capture_synchronizer.cc:74: // accident. This probably needs some farther explanation, e.g. mention how this can happen when multiple capture-clients are involved. https://codereview.chromium.org/2626013005/diff/180001/ui/views/mus/pointer_w... File ui/views/mus/pointer_watcher_event_router.cc (right): https://codereview.chromium.org/2626013005/diff/180001/ui/views/mus/pointer_w... ui/views/mus/pointer_watcher_event_router.cc:172: window_tree_client_ = nullptr; It would make sense to continue to RemoveObserver from |client|.
riajiang@chromium.org changed reviewers: + sky@chromium.org
Also +sky@, could you take a look? Thanks! https://codereview.chromium.org/2626013005/diff/180001/ui/aura/mus/capture_sy... File ui/aura/mus/capture_synchronizer.cc (right): https://codereview.chromium.org/2626013005/diff/180001/ui/aura/mus/capture_sy... ui/aura/mus/capture_synchronizer.cc:27: // Don't immediately set capture client. It's possible the change will be On 2017/01/30 19:37:58, sadrul wrote: > We do not set the capture client anymore (since we don't track a single capture > client). Maybe something like: > > """Don't immediately set |capture_window_|. It's possible the change > will be rejected. |capture_window_| is set in OnCaptureChanged() if > capture succeeds.""" Done. https://codereview.chromium.org/2626013005/diff/180001/ui/aura/mus/capture_sy... ui/aura/mus/capture_synchronizer.cc:74: // accident. On 2017/01/30 19:37:58, sadrul wrote: > This probably needs some farther explanation, e.g. mention how this can happen > when multiple capture-clients are involved. Updated the comment. WDYT? https://codereview.chromium.org/2626013005/diff/180001/ui/views/mus/pointer_w... File ui/views/mus/pointer_watcher_event_router.cc (right): https://codereview.chromium.org/2626013005/diff/180001/ui/views/mus/pointer_w... ui/views/mus/pointer_watcher_event_router.cc:172: window_tree_client_ = nullptr; On 2017/01/30 19:37:58, sadrul wrote: > It would make sense to continue to RemoveObserver from |client|. I thought that by the time the WindowTreeClient is being destroyed, the PointerWatcherEventRouter should have already removed itself as an observer for all the CaptureClients (when DesktopWindowTreeHostMus gets closed and when WindowManager gets shut down)? And now that we don't just have one capture client anymore, we can't get the one capture client from WindowTreeClient?
https://codereview.chromium.org/2626013005/diff/180001/ui/views/mus/pointer_w... File ui/views/mus/pointer_watcher_event_router.cc (right): https://codereview.chromium.org/2626013005/diff/180001/ui/views/mus/pointer_w... ui/views/mus/pointer_watcher_event_router.cc:172: window_tree_client_ = nullptr; On 2017/01/30 20:24:09, riajiang wrote: > On 2017/01/30 19:37:58, sadrul wrote: > > It would make sense to continue to RemoveObserver from |client|. > > I thought that by the time the WindowTreeClient is being destroyed, the > PointerWatcherEventRouter should have already removed itself as an observer for > all the CaptureClients (when DesktopWindowTreeHostMus gets closed and when > WindowManager gets shut down)? And now that we don't just have one capture > client anymore, we can't get the one capture client from WindowTreeClient? Oh yeah, of course. You are right.
As there can only be a single CaptureClient with capture at once is there a reason you didn't want to go with a pattern like I did for the FocusClient in Env? That is, Env gets a SetActiveCaptureClient() that can be used to observe and CaptureSynchronizer then observes changes on Env? I like that better in that it means no direct dependencies on mus code.
On 2017/01/31 00:52:27, sky wrote: > As there can only be a single CaptureClient with capture at once is there a > reason you didn't want to go with a pattern like I did for the FocusClient in > Env? That is, Env gets a SetActiveCaptureClient() that can be used to observe > and CaptureSynchronizer then observes changes on Env? I like that better in that > it means no direct dependencies on mus code. My worry is that aura::Env will end up being the place where we stuff everything, and it would be difficult to stop these essentially internal states from elsewhere in chrome. I think the approach for the capture-synchronizer avoids that (and it would make sense to make focus-controller states be maintained in a similar fashion).
Fair enough, LGTM
The CQ bit was checked by riajiang@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2626013005/#ps240001 (title: "test")
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": 240001, "attempt_start_ts": 1485982796554390,
"parent_rev": "662f25082427e46ee802aa95ed883a0132e682e5", "commit_rev":
"e75f053a801b6a6322a57a9417582f14ebc3f161"}
Message was sent while issue was closed.
Description was changed from ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests views_mus_unittests ========== to ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests views_mus_unittests Review-Url: https://codereview.chromium.org/2626013005 Cr-Commit-Position: refs/heads/master@{#447621} Committed: https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a941758... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a941758...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:240001) has been created in https://codereview.chromium.org/2674683002/ by johnme@chromium.org. The reason for reverting is: Caused aura_unittests WindowTreeClientClientTest.TwoWindowTreesRequestCapture to fail consistently on https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... since LSan detects a leak: Direct leak of 64 byte(s) in 1 object(s) allocated from: #0 0x52fcfb in operator new(unsigned long) (/b/s/w/irdxqiGi/out/Release/aura_unittests+0x52fcfb) #1 0x729983 in aura::WindowTreeClientClientTest_TwoWindowTreesRequestCapture_Test::TestBody() ui/aura/mus/window_tree_client_unittest.cc:1376:7.
Message was sent while issue was closed.
On 2017/02/02 12:59:19, johnme wrote: > A revert of this CL (patchset #9 id:240001) has been created in > https://codereview.chromium.org/2674683002/ by mailto:johnme@chromium.org. > > The reason for reverting is: Caused aura_unittests > WindowTreeClientClientTest.TwoWindowTreesRequestCapture > to fail consistently on > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... > > since LSan detects a leak: > > Direct leak of 64 byte(s) in 1 object(s) allocated from: > #0 0x52fcfb in operator new(unsigned long) > (/b/s/w/irdxqiGi/out/Release/aura_unittests+0x52fcfb) > #1 0x729983 in > aura::WindowTreeClientClientTest_TwoWindowTreesRequestCapture_Test::TestBody() > ui/aura/mus/window_tree_client_unittest.cc:1376:7. This looks like a leak in the test code. If it's not breaking the tree, maybe we can leave the cls in tree, and fix instead?
Message was sent while issue was closed.
Description was changed from ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests views_mus_unittests Review-Url: https://codereview.chromium.org/2626013005 Cr-Commit-Position: refs/heads/master@{#447621} Committed: https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a941758... ========== to ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests views_mus_unittests Review-Url: https://codereview.chromium.org/2626013005 Cr-Commit-Position: refs/heads/master@{#447621} Committed: https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a941758... ==========
On 2017/02/02 13:52:32, sadrul wrote: > On 2017/02/02 12:59:19, johnme wrote: > > A revert of this CL (patchset #9 id:240001) has been created in > > https://codereview.chromium.org/2674683002/ by mailto:johnme@chromium.org. > > > > The reason for reverting is: Caused aura_unittests > > WindowTreeClientClientTest.TwoWindowTreesRequestCapture > > to fail consistently on > > > > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... > > > > since LSan detects a leak: > > > > Direct leak of 64 byte(s) in 1 object(s) allocated from: > > #0 0x52fcfb in operator new(unsigned long) > > (/b/s/w/irdxqiGi/out/Release/aura_unittests+0x52fcfb) > > #1 0x729983 in > > aura::WindowTreeClientClientTest_TwoWindowTreesRequestCapture_Test::TestBody() > > ui/aura/mus/window_tree_client_unittest.cc:1376:7. > > This looks like a leak in the test code. If it's not breaking the tree, maybe we > can leave the cls in tree, and fix instead? Changed to use std::unique_ptr and reset to fix the leak in the test. Confirmed locally.
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2626013005/#ps260001 (title: "fix leak in test")
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": 260001, "attempt_start_ts": 1486053336743090,
"parent_rev": "6398f107d8df0d71f9c989ea1b73630136b580f3", "commit_rev":
"f6a5c39ae29d703bb4ecc6663477de3f9d3284a6"}
Message was sent while issue was closed.
Description was changed from ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests views_mus_unittests Review-Url: https://codereview.chromium.org/2626013005 Cr-Commit-Position: refs/heads/master@{#447621} Committed: https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a941758... ========== to ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests views_mus_unittests Review-Url: https://codereview.chromium.org/2626013005 Cr-Original-Commit-Position: refs/heads/master@{#447621} Committed: https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a941758... Review-Url: https://codereview.chromium.org/2626013005 Cr-Commit-Position: refs/heads/master@{#447790} Committed: https://chromium.googlesource.com/chromium/src/+/f6a5c39ae29d703bb4ecc6663477... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001) as https://chromium.googlesource.com/chromium/src/+/f6a5c39ae29d703bb4ecc6663477...
Message was sent while issue was closed.
Description was changed from ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests views_mus_unittests Review-Url: https://codereview.chromium.org/2626013005 Cr-Original-Commit-Position: refs/heads/master@{#447621} Committed: https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a941758... Review-Url: https://codereview.chromium.org/2626013005 Cr-Commit-Position: refs/heads/master@{#447790} Committed: https://chromium.googlesource.com/chromium/src/+/f6a5c39ae29d703bb4ecc6663477... ========== to ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057, 687869 TEST=aura_unittests views_mus_unittests Review-Url: https://codereview.chromium.org/2626013005 Cr-Original-Commit-Position: refs/heads/master@{#447621} Committed: https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a941758... Review-Url: https://codereview.chromium.org/2626013005 Cr-Commit-Position: refs/heads/master@{#447790} Committed: https://chromium.googlesource.com/chromium/src/+/f6a5c39ae29d703bb4ecc6663477... ==========
Message was sent while issue was closed.
Description was changed from ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057, 687869 TEST=aura_unittests views_mus_unittests Review-Url: https://codereview.chromium.org/2626013005 Cr-Original-Commit-Position: refs/heads/master@{#447621} Committed: https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a941758... Review-Url: https://codereview.chromium.org/2626013005 Cr-Commit-Position: refs/heads/master@{#447790} Committed: https://chromium.googlesource.com/chromium/src/+/f6a5c39ae29d703bb4ecc6663477... ========== to ========== Change CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. DesktopNativeWidgetAura was using DesktopCaptureClient but CaptureSynchronizer was only supporting one CaptureClient, which is a CaptureController. Therefore made the following changes: 1. We have one CaptureController for ash and multiple DesktopCaptureClients for all mus clients. This CL changes CaptureSynchronizer and PointerWatcherEventRouter to support multiple CaptureClients. When a root window's capture client is set, MusClient will be notified by DesktopWindowTreeHostMus if it's a DesktopCaptureClient, and WindowManager will take care of CaptureController. We then add CaptureSynchronizer and PointerWatcherEventRouter to be the observer for that CaptureClient. 2. Update DesktopCaptureClient to notify its observers when capture has changed. BUG=676002, 678057 TEST=aura_unittests views_mus_unittests Review-Url: https://codereview.chromium.org/2626013005 Cr-Original-Commit-Position: refs/heads/master@{#447621} Committed: https://chromium.googlesource.com/chromium/src/+/e75f053a801b6a6322a57a941758... Review-Url: https://codereview.chromium.org/2626013005 Cr-Commit-Position: refs/heads/master@{#447790} Committed: https://chromium.googlesource.com/chromium/src/+/f6a5c39ae29d703bb4ecc6663477... ========== |
