Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2434573004:
Close app list widget when users click outside of the widget (Closed)
DescriptionMash: Close app list widget when users click outside of the widget
Use PointerWatcherEventRouter/ui::PointerEvent to detect when a user clicks outside the visible app-list widget,
and close it. Make sure the app list is dismissed if the presenter is destroyed while it is visible.
BUG=611447
Committed: https://crrev.com/e1490a565be9fb332dd94b7f65518ea36c8251bf
Cr-Commit-Position: refs/heads/master@{#426909}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Check if view_ is not null before using it #
Total comments: 1
Patch Set 3 : Move pointer watcher for the app-list widget to functions OnShown and OnDismissed from constructor/… #Patch Set 4 : Remove pointer watcher in the destructor in case OnDismissed doesn't get called when delegate gets … #
Total comments: 12
Patch Set 5 : Adding a blank line back. #Patch Set 6 : Call AppListPresenterImpl::Dismiss() in AppListPresenterImpl::~AppListPresenterImpl() to ensure de… #
Total comments: 6
Patch Set 7 : Remove class name on Dismiss. #Patch Set 8 : Combine 2 if statements and add DCHECK(!is_visible_) before its use. #
Total comments: 2
Patch Set 9 : Make Dismiss override final #
Total comments: 2
Patch Set 10 : Use presenter_->GetTargetVisibility() to check visibility of the app-list widget instead of the var… #
Total comments: 2
Patch Set 11 : Add comments in destructor for clarification. #
Messages
Total messages: 70 (25 generated)
thanhph@chromium.org changed reviewers: + mfomitchev@chromium.org
LGTM
thanhph@chromium.org changed reviewers: + msw@chromium.org
msw@chromium.org: Please review changes in
https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (left): https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:35: nit: leave this blank line here; it matches the start of the namespace. https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:44: views::WindowManagerConnection::Get() The presenter doesn't need to watch for pointer events over its entire lifetime. It should add itself in |OnShown| and remove itself in |OnDismissed| to only watch while the app list is open; that'll avoid incurring any performance costs outside the existence of the app list. https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:115: if (!target || target != view_->GetWidget()) It seems like |view_| could be null when this is called; check before accessing. https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:116: presenter_->Dismiss(); nit: we could probably just call view_->GetWidget()->Close() to avoid caching the presenter pointer. WDYT? https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right): https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:23: public views::PointerWatcher { optional nit: It'd would be nice to use composition instead of inheritance here; this class could own a separate PointerWatcher subclass, defined in the .cc file, that's just dedicated to that bit of functionality. Perhaps that could even be generalized and consolidated for other similar transient UI surfaces, like the status area bubble. WDYT?
https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:44: views::WindowManagerConnection::Get() On 2016/10/19 21:18:46, msw wrote: > The presenter doesn't need to watch for pointer events over its entire lifetime. > It should add itself in |OnShown| and remove itself in |OnDismissed| to only > watch while the app list is open; that'll avoid incurring any performance costs > outside the existence of the app list. Even though it's in the constructor, the presenter only starts to watch pointer events when users open the app-list widget. I checked by the put a LOG(ERROR) in function AppListPresenterDelegateMus::OnPointerEventObserved. And as soon as the widget closes, the presenter removes the watching pointer event. This approach doesn't affect performance in this manner. https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:115: if (!target || target != view_->GetWidget()) On 2016/10/19 21:18:46, msw wrote: > It seems like |view_| could be null when this is called; check before accessing. Acknowledged. https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:116: presenter_->Dismiss(); On 2016/10/19 21:18:46, msw wrote: > nit: we could probably just call view_->GetWidget()->Close() to avoid caching > the presenter pointer. WDYT? Using view_->GetWidget()->Close() instead of presenter_->Dismiss(); will provide no animation when closing the app-list widget. I tested your suggestion using view_->GetWidget()->Close() on classic ash by replacing this line https://cs.chromium.org/chromium/src/ash/app_list/app_list_presenter_delegate... and it produces no animation (mfomitchev@ gave me input here) We probably want to support animation.
https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:44: views::WindowManagerConnection::Get() On 2016/10/19 22:21:45, thanhph wrote: > On 2016/10/19 21:18:46, msw wrote: > > The presenter doesn't need to watch for pointer events over its entire > lifetime. > > It should add itself in |OnShown| and remove itself in |OnDismissed| to only > > watch while the app list is open; that'll avoid incurring any performance > costs > > outside the existence of the app list. > > Even though it's in the constructor, the presenter only starts to watch pointer > events when users open the app-list widget. I checked by the put a LOG(ERROR) in > function AppListPresenterDelegateMus::OnPointerEventObserved. And as soon as the > widget closes, the presenter removes the watching pointer event. This approach > doesn't affect performance in this manner. I'm not sure I understand. Are you saying that the delegate is only constructed when the app list is shown and destroyed when the app list is hidden, or is there some other mechanism adding removing this pointer watcher ability during the lifetime of this object? I would find it very odd if the pointer watcher notifications aren't sent to this object during its lifetime while the app list isn't visible. Could there have been something else confounding your log printing investigation? https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:116: presenter_->Dismiss(); On 2016/10/19 22:21:45, thanhph wrote: > On 2016/10/19 21:18:46, msw wrote: > > nit: we could probably just call view_->GetWidget()->Close() to avoid caching > > the presenter pointer. WDYT? > > Using view_->GetWidget()->Close() instead of presenter_->Dismiss(); will provide > no animation when closing the app-list widget. > > I tested your suggestion using view_->GetWidget()->Close() on classic ash by > replacing this line > https://cs.chromium.org/chromium/src/ash/app_list/app_list_presenter_delegate... > and it produces no animation (mfomitchev@ gave me input here) > > We probably want to support animation. > Acknowledged. https://chromiumcodereview.appspot.com/2434573004/diff/20001/chrome/browser/u... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/20001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:37: } repeated nit: restore the blank line here to match the block start.
https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:44: views::WindowManagerConnection::Get() On 2016/10/19 22:43:38, msw wrote: > On 2016/10/19 22:21:45, thanhph wrote: > > On 2016/10/19 21:18:46, msw wrote: > > > The presenter doesn't need to watch for pointer events over its entire > > lifetime. > > > It should add itself in |OnShown| and remove itself in |OnDismissed| to only > > > watch while the app list is open; that'll avoid incurring any performance > > costs > > > outside the existence of the app list. > > > > Even though it's in the constructor, the presenter only starts to watch > pointer > > events when users open the app-list widget. I checked by the put a LOG(ERROR) > in > > function AppListPresenterDelegateMus::OnPointerEventObserved. And as soon as > the > > widget closes, the presenter removes the watching pointer event. This approach > > doesn't affect performance in this manner. > > I'm not sure I understand. Are you saying that the delegate is only constructed > when the app list is shown and destroyed when the app list is hidden, or is > there some other mechanism adding removing this pointer watcher ability during > the lifetime of this object? > > I would find it very odd if the pointer watcher notifications aren't sent to > this object during its lifetime while the app list isn't visible. Could there > have been something else confounding your log printing investigation? Regardless, it seems entirely reasonable to move these Add/Remove function calls to OnShown and OnDismissed, right?
https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:44: views::WindowManagerConnection::Get() On 2016/10/19 22:45:12, msw wrote: > On 2016/10/19 22:43:38, msw wrote: > > On 2016/10/19 22:21:45, thanhph wrote: > > > On 2016/10/19 21:18:46, msw wrote: > > > > The presenter doesn't need to watch for pointer events over its entire > > > lifetime. > > > > It should add itself in |OnShown| and remove itself in |OnDismissed| to > only > > > > watch while the app list is open; that'll avoid incurring any performance > > > costs > > > > outside the existence of the app list. > > > > > > Even though it's in the constructor, the presenter only starts to watch > > pointer > > > events when users open the app-list widget. I checked by the put a > LOG(ERROR) > > in > > > function AppListPresenterDelegateMus::OnPointerEventObserved. And as soon as > > the > > > widget closes, the presenter removes the watching pointer event. This > approach > > > doesn't affect performance in this manner. > > > > I'm not sure I understand. Are you saying that the delegate is only > constructed > > when the app list is shown and destroyed when the app list is hidden, or is > > there some other mechanism adding removing this pointer watcher ability during > > the lifetime of this object? > > > > I would find it very odd if the pointer watcher notifications aren't sent to > > this object during its lifetime while the app list isn't visible. Could there > > have been something else confounding your log printing investigation? > > Regardless, it seems entirely reasonable to move these Add/Remove function calls > to OnShown and OnDismissed, right? Acknowledged. https://chromiumcodereview.appspot.com/2434573004/diff/1/chrome/browser/ui/as... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:44: views::WindowManagerConnection::Get() On 2016/10/19 22:43:38, msw wrote: > On 2016/10/19 22:21:45, thanhph wrote: > > On 2016/10/19 21:18:46, msw wrote: > > > The presenter doesn't need to watch for pointer events over its entire > > lifetime. > > > It should add itself in |OnShown| and remove itself in |OnDismissed| to only > > > watch while the app list is open; that'll avoid incurring any performance > > costs > > > outside the existence of the app list. > > > > Even though it's in the constructor, the presenter only starts to watch > pointer > > events when users open the app-list widget. I checked by the put a LOG(ERROR) > in > > function AppListPresenterDelegateMus::OnPointerEventObserved. And as soon as > the > > widget closes, the presenter removes the watching pointer event. This approach > > doesn't affect performance in this manner. > > I'm not sure I understand. Are you saying that the delegate is only constructed > when the app list is shown and destroyed when the app list is hidden, or is > there some other mechanism adding removing this pointer watcher ability during > the lifetime of this object? > > I would find it very odd if the pointer watcher notifications aren't sent to > this object during its lifetime while the app list isn't visible. Could there > have been something else confounding your log printing investigation? I put the log in constructor and destructor, also on the AppListPresenterDelegateMus::OnPointerEventObserved. The logs only spit out when users start clicking while app-list widget is being shown. Thus I don't believe this code affects performance at least at this moment. However I just moved the pointer watcher codes to OnShown and OnDismissed since they seem semantically make more sense. Thanks!
https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:38: } // namespace 2x repeated nit: restore the blank line before this line. Why does it make sense to remove that blank line, but keep the one at the top of the anon namespace? Please reply to code review comments, even nits. https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46: if (is_visible_) { Replace this with DCHECK(!is_visible_); it's a bug if OnDismissed isn't called.
https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46: if (is_visible_) { Drive-by: What if we destroy AppListPresenterImpl while it is showing the app list? Right now the first thing the destructor does is it destroys the delegate..
It would be nice if this wasn't a mus/mash-specific change. ShelfTooltipManager is a decent example of using PointerWatcher in both classic ash and mash. https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46: if (is_visible_) { On 2016/10/20 17:55:24, mfomitchev wrote: > Drive-by: What if we destroy AppListPresenterImpl while it is showing the app > list? Right now the first thing the destructor does is it destroys the > delegate.. OnDismissed should be called before this object is destroyed. This can be left as-is and that can be fixed later, but it's not that complex of a change and it'd be nice to see it happen now to avoid this odd pattern and duplication. https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:87: is_visible_ = true; nit: DCHECK(!is_visible_); some reasonable assumptions may not hold here.
https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:38: } // namespace On 2016/10/20 17:32:15, msw wrote: > 2x repeated nit: restore the blank line before this line. Why does it make sense > to remove that blank line, but keep the one at the top of the anon namespace? > Please reply to code review comments, even nits. Adding a blank line back is ok, to conform with the convention. https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46: if (is_visible_) { On 2016/10/20 18:09:03, msw wrote: > On 2016/10/20 17:55:24, mfomitchev wrote: > > Drive-by: What if we destroy AppListPresenterImpl while it is showing the app > > list? Right now the first thing the destructor does is it destroys the > > delegate.. > > OnDismissed should be called before this object is destroyed. This can be left > as-is and that can be fixed later, but it's not that complex of a change and > it'd be nice to see it happen now to avoid this odd pattern and duplication. I'll just leave it as it's for now in case AppListPresenterImpl got destroyed for some reasons. In the future we can create a unit test for this.
https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46: if (is_visible_) { On 2016/10/20 18:09:03, msw wrote: > On 2016/10/20 17:55:24, mfomitchev wrote: > > Drive-by: What if we destroy AppListPresenterImpl while it is showing the app > > list? Right now the first thing the destructor does is it destroys the > > delegate.. > > OnDismissed should be called before this object is destroyed. This can be left > as-is and that can be fixed later, but it's not that complex of a change and > it'd be nice to see it happen now to avoid this odd pattern and duplication. That would mean that AppListPresenterImpl should call Dismiss() on itself in its destructor, right before doing anything else, right?
https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46: if (is_visible_) { On 2016/10/20 18:23:07, mfomitchev wrote: > On 2016/10/20 18:09:03, msw wrote: > > On 2016/10/20 17:55:24, mfomitchev wrote: > > > Drive-by: What if we destroy AppListPresenterImpl while it is showing the > app > > > list? Right now the first thing the destructor does is it destroys the > > > delegate.. > > > > OnDismissed should be called before this object is destroyed. This can be left > > as-is and that can be fixed later, but it's not that complex of a change and > > it'd be nice to see it happen now to avoid this odd pattern and duplication. > > That would mean that AppListPresenterImpl should call Dismiss() on itself in its > destructor, right before doing anything else, right? That's reasonable, or it could just call OnDismissed on the delegate. https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46: if (is_visible_) { On 2016/10/20 18:16:48, thanhph wrote: > On 2016/10/20 18:09:03, msw wrote: > > On 2016/10/20 17:55:24, mfomitchev wrote: > > > Drive-by: What if we destroy AppListPresenterImpl while it is showing the > app > > > list? Right now the first thing the destructor does is it destroys the > > > delegate.. > > > > OnDismissed should be called before this object is destroyed. This can be left > > as-is and that can be fixed later, but it's not that complex of a change and > > it'd be nice to see it happen now to avoid this odd pattern and duplication. > > I'll just leave it as it's for now in case AppListPresenterImpl got destroyed > for some reasons. In the future we can create a unit test for this. If you call Dismiss/OnDismissed in the AppListPresenterImpl dtor, then I would encourage you to replace this unnecessarily duplicated code with a DCHECK.
https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46: if (is_visible_) { On 2016/10/20 18:46:53, msw wrote: > On 2016/10/20 18:23:07, mfomitchev wrote: > > On 2016/10/20 18:09:03, msw wrote: > > > On 2016/10/20 17:55:24, mfomitchev wrote: > > > > Drive-by: What if we destroy AppListPresenterImpl while it is showing the > > app > > > > list? Right now the first thing the destructor does is it destroys the > > > > delegate.. > > > > > > OnDismissed should be called before this object is destroyed. This can be > left > > > as-is and that can be fixed later, but it's not that complex of a change and > > > it'd be nice to see it happen now to avoid this odd pattern and duplication. > > > > That would mean that AppListPresenterImpl should call Dismiss() on itself in > its > > destructor, right before doing anything else, right? > > That's reasonable, or it could just call OnDismissed on the delegate. Simply calling OnDismissed feels a bit icky, because this means that the state of is_visible_ in the presenter and the delegate get out of sync. Granted, the presenter is getting destroyed, but still.. You could imagine a perfectly reasonable DCHECK(!presenter_->IsVisible()) in OnDismissed(), which would fail if we did this.
https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46: if (is_visible_) { On 2016/10/20 19:29:35, mfomitchev wrote: > On 2016/10/20 18:46:53, msw wrote: > > On 2016/10/20 18:23:07, mfomitchev wrote: > > > On 2016/10/20 18:09:03, msw wrote: > > > > On 2016/10/20 17:55:24, mfomitchev wrote: > > > > > Drive-by: What if we destroy AppListPresenterImpl while it is showing > the > > > app > > > > > list? Right now the first thing the destructor does is it destroys the > > > > > delegate.. > > > > > > > > OnDismissed should be called before this object is destroyed. This can be > > left > > > > as-is and that can be fixed later, but it's not that complex of a change > and > > > > it'd be nice to see it happen now to avoid this odd pattern and > duplication. > > > > > > That would mean that AppListPresenterImpl should call Dismiss() on itself in > > its > > > destructor, right before doing anything else, right? > > > > That's reasonable, or it could just call OnDismissed on the delegate. > > Simply calling OnDismissed feels a bit icky, because this means that the state > of is_visible_ in the presenter and the delegate get out of sync. Granted, the > presenter is getting destroyed, but still.. You could imagine a perfectly > reasonable DCHECK(!presenter_->IsVisible()) in OnDismissed(), which would fail > if we did this. Good point; calling Dismiss seems more correct.
On 2016/10/20 19:34:50, msw wrote: > https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... > File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): > > https://chromiumcodereview.appspot.com/2434573004/diff/60001/chrome/browser/u... > chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:46: if > (is_visible_) { > On 2016/10/20 19:29:35, mfomitchev wrote: > > On 2016/10/20 18:46:53, msw wrote: > > > On 2016/10/20 18:23:07, mfomitchev wrote: > > > > On 2016/10/20 18:09:03, msw wrote: > > > > > On 2016/10/20 17:55:24, mfomitchev wrote: > > > > > > Drive-by: What if we destroy AppListPresenterImpl while it is showing > > the > > > > app > > > > > > list? Right now the first thing the destructor does is it destroys the > > > > > > delegate.. > > > > > > > > > > OnDismissed should be called before this object is destroyed. This can > be > > > left > > > > > as-is and that can be fixed later, but it's not that complex of a change > > and > > > > > it'd be nice to see it happen now to avoid this odd pattern and > > duplication. > > > > > > > > That would mean that AppListPresenterImpl should call Dismiss() on itself > in > > > its > > > > destructor, right before doing anything else, right? > > > > > > That's reasonable, or it could just call OnDismissed on the delegate. > > > > Simply calling OnDismissed feels a bit icky, because this means that the state > > of is_visible_ in the presenter and the delegate get out of sync. Granted, the > > presenter is getting destroyed, but still.. You could imagine a perfectly > > reasonable DCHECK(!presenter_->IsVisible()) in OnDismissed(), which would fail > > if we did this. > > Good point; calling Dismiss seems more correct. Thanks! I call AppListPresenterImpl::Dismiss() on ~AppListPresenterImpl and put DCHECK(!is_visible_) on ~AppListPresenterDelegateMus
The CQ bit was checked by thanhph@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 thanhph@chromium.org
https://chromiumcodereview.appspot.com/2434573004/diff/100001/ui/app_list/pre... File ui/app_list/presenter/app_list_presenter_impl.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/100001/ui/app_list/pre... ui/app_list/presenter/app_list_presenter_impl.cc:40: AppListPresenterImpl::Dismiss(); remove "AppListPresenterImpl::"
On 2016/10/20 19:52:24, mfomitchev wrote: > https://chromiumcodereview.appspot.com/2434573004/diff/100001/ui/app_list/pre... > File ui/app_list/presenter/app_list_presenter_impl.cc (right): > > https://chromiumcodereview.appspot.com/2434573004/diff/100001/ui/app_list/pre... > ui/app_list/presenter/app_list_presenter_impl.cc:40: > AppListPresenterImpl::Dismiss(); > remove "AppListPresenterImpl::" Ok!
https://chromiumcodereview.appspot.com/2434573004/diff/100001/ui/app_list/pre... File ui/app_list/presenter/app_list_presenter_impl.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/100001/ui/app_list/pre... ui/app_list/presenter/app_list_presenter_impl.cc:40: AppListPresenterImpl::Dismiss(); On 2016/10/20 19:52:23, mfomitchev wrote: > remove "AppListPresenterImpl::" Ok!
Mostly lg with nits; I'll stamp after you address/reply to my comments and the last one from Mikhail. https://chromiumcodereview.appspot.com/2434573004/diff/100001/chrome/browser/... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/100001/chrome/browser/... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:84: is_visible_ = true; repeated nit: DCHECK(!is_visible_); before this. https://chromiumcodereview.appspot.com/2434573004/diff/100001/chrome/browser/... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:116: if (!target || (view_ && (target != view_->GetWidget()))) nit: combine this conditional with the one nesting it; there's no real reason to split the conditional terms into two if statements.
https://chromiumcodereview.appspot.com/2434573004/diff/100001/chrome/browser/... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://chromiumcodereview.appspot.com/2434573004/diff/100001/chrome/browser/... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:84: is_visible_ = true; On 2016/10/20 20:03:52, msw wrote: > repeated nit: DCHECK(!is_visible_); before this. Acknowledged. https://chromiumcodereview.appspot.com/2434573004/diff/100001/chrome/browser/... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:116: if (!target || (view_ && (target != view_->GetWidget()))) On 2016/10/20 20:03:52, msw wrote: > nit: combine this conditional with the one nesting it; there's no real reason to > split the conditional terms into two if statements. Acknowledged.
lgtm
The CQ bit was checked by thanhph@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...
thanhph@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@chromium.org: Please review changes in app_list_presenter_impl.cc
https://codereview.chromium.org/2434573004/diff/140001/ui/app_list/presenter/... File ui/app_list/presenter/app_list_presenter_impl.cc (right): https://codereview.chromium.org/2434573004/diff/140001/ui/app_list/presenter/... ui/app_list/presenter/app_list_presenter_impl.cc:40: Dismiss(); Looks like this is a virtual function, and calling this from the dtor feels icky/risky. Perhaps AppListPresenterImpl should be a final class? Or just make the Dismiss() override final?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2434573004/diff/140001/ui/app_list/presenter/... File ui/app_list/presenter/app_list_presenter_impl.cc (right): https://codereview.chromium.org/2434573004/diff/140001/ui/app_list/presenter/... ui/app_list/presenter/app_list_presenter_impl.cc:40: Dismiss(); On 2016/10/21 15:12:48, sadrul wrote: > Looks like this is a virtual function, and calling this from the dtor feels > icky/risky. Perhaps AppListPresenterImpl should be a final class? Or just make > the Dismiss() override final? Thank Sadrul! I make Dismiss override final by changing .h file.
Please update the CL description to mention the changes, and that this is a fix for mus. With that, lgtm https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right): https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:48: bool is_visible_ = false; Unrelated to this CL I guess: why is |is_visible_| being tracked here? Does it ever get out of sync with |presenter_->IsVisible()|?
https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right): https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:48: bool is_visible_ = false; On 2016/10/21 17:12:28, sadrul wrote: > Unrelated to this CL I guess: why is |is_visible_| being tracked here? Does it > ever get out of sync with |presenter_->IsVisible()|? Good point. Now that we have a pointer to the presenter, I don't think there's any reason to track is_visible_.
The CQ bit was checked by thanhph@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...
Description was changed from ========== Close app list widget when users click outside of the widget BUG=611447 ========== to ========== Close app list widget when users click outside of the widget BUG=611447 Solution: Starting to monitor mouse/click events when app-list goes through OnShown. Stopping to monitor mouse/click events when app-list goes through OnDismissed. Calling function Dimiss on ~AppListPresenterImpl to ensure presenter cleanup. Makeing function Dismiss override final to prevent nasty inheritance on Dismiss. ==========
On 2016/10/21 17:12:29, sadrul wrote: > Please update the CL description to mention the changes, and that this is a fix > for mus. With that, lgtm Description is updated! > > https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right): > > https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:48: bool > is_visible_ = false; > Unrelated to this CL I guess: why is |is_visible_| being tracked here? Does it > ever get out of sync with |presenter_->IsVisible()|?
On 2016/10/21 17:55:21, thanhph wrote: > On 2016/10/21 17:12:29, sadrul wrote: > > Please update the CL description to mention the changes, and that this is a > fix > > for mus. With that, lgtm > > Description is updated! > > > > > > https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... > > File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right): > > > > > https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... > > chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:48: bool > > is_visible_ = false; > > Unrelated to this CL I guess: why is |is_visible_| being tracked here? Does it > > ever get out of sync with |presenter_->IsVisible()|? I'm testing this out.
On 2016/10/21 17:55:21, thanhph wrote: > On 2016/10/21 17:12:29, sadrul wrote: > > Please update the CL description to mention the changes, and that this is a > fix > > for mus. With that, lgtm > > Description is updated! Thanks! The 'BUG' line should come at the end though. Suggestion with some re-wording: 'mash: Close app list widget when users click outside of the widget Use views::PointerWatcher to detect when user clicks outside the widget, and close it. Also, make sure the app list is dismissed if the presenter is destroyed while it is visible. BUG=611447
Description was changed from ========== Close app list widget when users click outside of the widget BUG=611447 Solution: Starting to monitor mouse/click events when app-list goes through OnShown. Stopping to monitor mouse/click events when app-list goes through OnDismissed. Calling function Dimiss on ~AppListPresenterImpl to ensure presenter cleanup. Makeing function Dismiss override final to prevent nasty inheritance on Dismiss. ========== to ========== Mash: Close app list widget when users click outside of the widget Use PointerWatcherEventRouter/ui::PointerEvent to detect when a user clicks outside the visible app-list widget, and close it. Make sure the app list is dismissed if the presenter is destroyed while it is visible. BUG=611447 ==========
On 2016/10/21 17:59:21, sadrul wrote: > On 2016/10/21 17:55:21, thanhph wrote: > > On 2016/10/21 17:12:29, sadrul wrote: > > > Please update the CL description to mention the changes, and that this is a > > fix > > > for mus. With that, lgtm > > > > Description is updated! > > Thanks! The 'BUG' line should come at the end though. Suggestion with some > re-wording: > > 'mash: Close app list widget when users click outside of the widget > > Use views::PointerWatcher to detect when user clicks outside the widget, > and close it. Also, make sure the app list is dismissed if the presenter > is destroyed while it is visible. > > BUG=611447 Thanks! I updated the description to: Mash: Close visible app list widget when users click outside of the widget. Use views::PointerWatcher to detect when user clicks outside the visible app-list widget, and close it. Also, make sure the app list is dismissed if the presenter is destroyed while it is visible. BUG=611447
On 2016/10/21 17:14:19, mfomitchev wrote: > https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right): > > https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:48: bool > is_visible_ = false; > On 2016/10/21 17:12:28, sadrul wrote: > > Unrelated to this CL I guess: why is |is_visible_| being tracked here? Does it > > ever get out of sync with |presenter_->IsVisible()|? > > Good point. Now that we have a pointer to the presenter, I don't think there's > any reason to track is_visible_. When I call presenter_->Dismiss() in void AppListPresenterDelegateMus::OnPointerEventObserved, apparently presenter_->IsVisible() is still true so the 2nd time I clicked on launcher, DCHECK(!presenter_->IsVisible()) at OnShown breaks since presenter_->IsVisible() is supposed to be false instead of true. I think I need to add something in OnDismiss in AppListPresenterImpl regarding to AppListView.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/21 18:54:39, thanhph wrote: > On 2016/10/21 17:14:19, mfomitchev wrote: > > > https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... > > File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h (right): > > > > > https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... > > chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:48: bool > > is_visible_ = false; > > On 2016/10/21 17:12:28, sadrul wrote: > > > Unrelated to this CL I guess: why is |is_visible_| being tracked here? Does > it > > > ever get out of sync with |presenter_->IsVisible()|? > > > > Good point. Now that we have a pointer to the presenter, I don't think there's > > any reason to track is_visible_. > > When I call presenter_->Dismiss() in void > AppListPresenterDelegateMus::OnPointerEventObserved, apparently > presenter_->IsVisible() is still true so the 2nd time I clicked on launcher, > DCHECK(!presenter_->IsVisible()) at OnShown breaks since presenter_->IsVisible() > is supposed to be false instead of true. I think I need to add something in > OnDismiss in AppListPresenterImpl regarding to AppListView. presenter_->IsVisible() is true in OnShown because is_visible_ is set to true in AppListPresenterImpl::Show, before presenter_delegate_->OnShown() is called. We can probably just reverse the DCHECKs: DCHECK(presenter->IsVisible()) in OnShown and DCHECK(!presenter->IsVisible()) in OnDismissed.
On 2016/10/21 20:21:54, mfomitchev wrote: > On 2016/10/21 18:54:39, thanhph wrote: > > On 2016/10/21 17:14:19, mfomitchev wrote: > > > > > > https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... > > > File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h > (right): > > > > > > > > > https://codereview.chromium.org/2434573004/diff/160001/chrome/browser/ui/ash/... > > > chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.h:48: bool > > > is_visible_ = false; > > > On 2016/10/21 17:12:28, sadrul wrote: > > > > Unrelated to this CL I guess: why is |is_visible_| being tracked here? > Does > > it > > > > ever get out of sync with |presenter_->IsVisible()|? > > > > > > Good point. Now that we have a pointer to the presenter, I don't think > there's > > > any reason to track is_visible_. > > > > When I call presenter_->Dismiss() in void > > AppListPresenterDelegateMus::OnPointerEventObserved, apparently > > presenter_->IsVisible() is still true so the 2nd time I clicked on launcher, > > DCHECK(!presenter_->IsVisible()) at OnShown breaks since > presenter_->IsVisible() > > is supposed to be false instead of true. I think I need to add something in > > OnDismiss in AppListPresenterImpl regarding to AppListView. > > presenter_->IsVisible() is true in OnShown because is_visible_ is set to true in > AppListPresenterImpl::Show, before presenter_delegate_->OnShown() is called. > We can probably just reverse the DCHECKs: DCHECK(presenter->IsVisible()) in > OnShown and DCHECK(!presenter->IsVisible()) in OnDismissed. I use existing presenter_->GetTargetVisibility() instead of presenter_->IsVisible() and it works. Not sure why presenter_->IsVisible() still true after presenter_->Dismiss() is called. I tempted to update the function AppListPresenterImpl::IsVisible() to counter this but chose simpler solution instead. Let me know if any problem arises with this approach.
> I use existing presenter_->GetTargetVisibility() instead of > presenter_->IsVisible() and it works. Not sure why presenter_->IsVisible() still > true after presenter_->Dismiss() is called. Ah, right, this makes sense. Presenter has a dismiss animation, and so IsVisible() (correctly) returns true while the dismiss animation is in progress and the app list widget is still visible. When the animation ends, the widget will no longer be visible and IsVisible will return false if called. GetTargetVisibility() on the other hand corresponds to the "target" state, so it will return false after Dismiss() is executed even while the animation is in progress. https://codereview.chromium.org/2434573004/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://codereview.chromium.org/2434573004/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:47: DCHECK(!presenter_->GetTargetVisibility()); Nit: Maybe add a comment here to clarify the DCHECK: // Presenter is supposed to be dismissed when the delegate is destroyed. This means we don't have to worry about unregistering the PointerWatcher here.
> I use existing presenter_->GetTargetVisibility() instead of > presenter_->IsVisible() and it works. Not sure why presenter_->IsVisible() still > true after presenter_->Dismiss() is called. Ah, right, this makes sense. Presenter has a dismiss animation, and so IsVisible() (correctly) returns true while the dismiss animation is in progress and the app list widget is still visible. When the animation ends, the widget will no longer be visible and IsVisible will return false if called. GetTargetVisibility() on the other hand corresponds to the "target" state, so it will return false after Dismiss() is executed even while the animation is in progress.
On 2016/10/21 20:41:33, mfomitchev wrote: > > I use existing presenter_->GetTargetVisibility() instead of > > presenter_->IsVisible() and it works. Not sure why presenter_->IsVisible() > still > > true after presenter_->Dismiss() is called. > > Ah, right, this makes sense. Presenter has a dismiss animation, and so > IsVisible() (correctly) returns true while the dismiss animation is in progress > and the app list widget is still visible. When the animation ends, the widget > will no longer be visible and IsVisible will return false if called. > GetTargetVisibility() on the other hand corresponds to the "target" state, so it > will return false after Dismiss() is executed even while the animation is in > progress. Thanks for the clarification. I indeed repeatedly clicked the launcher icon in high frequency, maybe animation is probably unable to complete as a result.
https://codereview.chromium.org/2434573004/diff/180001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc (right): https://codereview.chromium.org/2434573004/diff/180001/chrome/browser/ui/ash/... chrome/browser/ui/ash/app_list/app_list_presenter_delegate_mus.cc:47: DCHECK(!presenter_->GetTargetVisibility()); On 2016/10/21 20:41:29, mfomitchev wrote: > Nit: Maybe add a comment here to clarify the DCHECK: > // Presenter is supposed to be dismissed when the delegate is destroyed. This > means we don't have to worry about unregistering the PointerWatcher here. Acknowledged.
The CQ bit was checked by thanhph@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...
On 2016/10/21 20:50:52, thanhph wrote: > On 2016/10/21 20:41:33, mfomitchev wrote: > > > I use existing presenter_->GetTargetVisibility() instead of > > > presenter_->IsVisible() and it works. Not sure why presenter_->IsVisible() > > still > > > true after presenter_->Dismiss() is called. > > > > Ah, right, this makes sense. Presenter has a dismiss animation, and so > > IsVisible() (correctly) returns true while the dismiss animation is in > progress > > and the app list widget is still visible. When the animation ends, the widget > > will no longer be visible and IsVisible will return false if called. > > GetTargetVisibility() on the other hand corresponds to the "target" state, so > it > > will return false after Dismiss() is executed even while the animation is in > > progress. > > Thanks for the clarification. I indeed repeatedly clicked the launcher icon in > high frequency, maybe animation is probably unable to complete as a result. The animation is JUST started when Show/Dismiss in called (see where it calls ScheduleAnimation()). So it will never be completed when OnShown/OnDismissed is called unless its duration is 0. So you don't have to click quickly at all to run into this.
On 2016/10/21 20:57:57, mfomitchev wrote: > On 2016/10/21 20:50:52, thanhph wrote: > > On 2016/10/21 20:41:33, mfomitchev wrote: > > > > I use existing presenter_->GetTargetVisibility() instead of > > > > presenter_->IsVisible() and it works. Not sure why presenter_->IsVisible() > > > still > > > > true after presenter_->Dismiss() is called. > > > > > > Ah, right, this makes sense. Presenter has a dismiss animation, and so > > > IsVisible() (correctly) returns true while the dismiss animation is in > > progress > > > and the app list widget is still visible. When the animation ends, the > widget > > > will no longer be visible and IsVisible will return false if called. > > > GetTargetVisibility() on the other hand corresponds to the "target" state, > so > > it > > > will return false after Dismiss() is executed even while the animation is in > > > progress. > > > > Thanks for the clarification. I indeed repeatedly clicked the launcher icon in > > high frequency, maybe animation is probably unable to complete as a result. > > The animation is JUST started when Show/Dismiss in called (see where it calls > ScheduleAnimation()). So it will never be completed when OnShown/OnDismissed is > called unless its duration is 0. So you don't have to click quickly at all to > run into this. Ok, sounds good! I started the dry run.
The CQ bit was unchecked by thanhph@chromium.org
The CQ bit was checked by thanhph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfomitchev@chromium.org, msw@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2434573004/#ps200001 (title: "Add comments in destructor for clarification.")
The CQ bit was unchecked by thanhph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by thanhph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Mash: Close app list widget when users click outside of the widget Use PointerWatcherEventRouter/ui::PointerEvent to detect when a user clicks outside the visible app-list widget, and close it. Make sure the app list is dismissed if the presenter is destroyed while it is visible. BUG=611447 ========== to ========== Mash: Close app list widget when users click outside of the widget Use PointerWatcherEventRouter/ui::PointerEvent to detect when a user clicks outside the visible app-list widget, and close it. Make sure the app list is dismissed if the presenter is destroyed while it is visible. BUG=611447 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Mash: Close app list widget when users click outside of the widget Use PointerWatcherEventRouter/ui::PointerEvent to detect when a user clicks outside the visible app-list widget, and close it. Make sure the app list is dismissed if the presenter is destroyed while it is visible. BUG=611447 ========== to ========== Mash: Close app list widget when users click outside of the widget Use PointerWatcherEventRouter/ui::PointerEvent to detect when a user clicks outside the visible app-list widget, and close it. Make sure the app list is dismissed if the presenter is destroyed while it is visible. BUG=611447 Committed: https://crrev.com/e1490a565be9fb332dd94b7f65518ea36c8251bf Cr-Commit-Position: refs/heads/master@{#426909} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/e1490a565be9fb332dd94b7f65518ea36c8251bf Cr-Commit-Position: refs/heads/master@{#426909} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
