Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1841563003:
ARC Toast: Prevent onClosed event from being called multiple times (Closed)
DescriptionARC Toast: Prevent onClosed event from being called multiple times
BUG=b/25797993
Committed: https://crrev.com/8079b4d3bda9c25b7f20c80bd206e3bf11176612
Cr-Commit-Position: refs/heads/master@{#387499}
Patch Set 1 #Patch Set 2 #
Total comments: 6
Patch Set 3 : addressed the comment #Patch Set 4 : Using inflight animation set instead of the flag #Patch Set 5 : Removed is_visible flag #
Total comments: 2
Patch Set 6 : . #
Total comments: 2
Patch Set 7 : . #
Messages
Total messages: 25 (7 generated)
Description was changed from ========== wip BUG= ========== to ========== ARC Toast: Prevent onClosed event from being called multiple times BUG=b/25797993 ==========
yoshiki@chromium.org changed reviewers: + oshima@chromium.org
Oshima-san, PTAL. Thanks.
https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... ash/system/toast/toast_overlay.cc:229: close_event_fired_ = false; (if you still need this) move this before Hide because Hide may trigger the ended if ZERO_DURATION is used. https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/scop... https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... ash/system/toast/toast_overlay.cc:249: if (!is_visible_ && !close_event_fired_) { can't you just check overlay_widget_'s visibility?
Oshima-san, PTAL. https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... ash/system/toast/toast_overlay.cc:229: close_event_fired_ = false; On 2016/03/30 19:11:50, oshima wrote: > (if you still need this) > move this before Hide because Hide may trigger the ended if ZERO_DURATION is > used. > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/scop... Done. https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... ash/system/toast/toast_overlay.cc:249: if (!is_visible_ && !close_event_fired_) { On 2016/03/30 19:11:50, oshima wrote: > can't you just check overlay_widget_'s visibility? Here overlay_widget_'s visibility is synced with |is_visible_|.
https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... ash/system/toast/toast_overlay.cc:249: if (!is_visible_ && !close_event_fired_) { On 2016/04/01 18:32:35, yoshiki wrote: > On 2016/03/30 19:11:50, oshima wrote: > > can't you just check overlay_widget_'s visibility? > > Here overlay_widget_'s visibility is synced with |is_visible_|. My question was do you even need is_visible_. Can't we jsut use overlay_widget_->IsVisible() ?
I updated the patch with set. That is because I found multiple animations are happened when toast is closing and showing. In previous way, the onClosed-delegate is called on the first OnLayerAnimationEnded-handler. But it was wrong. PTAL again? Thanks. https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... ash/system/toast/toast_overlay.cc:249: if (!is_visible_ && !close_event_fired_) { On 2016/04/01 19:15:10, oshima wrote: > On 2016/04/01 18:32:35, yoshiki wrote: > > On 2016/03/30 19:11:50, oshima wrote: > > > can't you just check overlay_widget_'s visibility? > > > > Here overlay_widget_'s visibility is synced with |is_visible_|. > > My question was do you even need is_visible_. Can't we jsut use > overlay_widget_->IsVisible() ? Sorry I was wrong. When this method is called, widget is still visible. So we need |is_invisible_| to know that this is hiding (or showing) animation.
On 2016/04/06 17:34:07, yoshiki wrote: > I updated the patch with set. That is because I found multiple animations are > happened when toast is closing and showing. In previous way, the > onClosed-delegate is called on the first OnLayerAnimationEnded-handler. But it > was wrong. > > PTAL again? Thanks. > > https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... > File ash/system/toast/toast_overlay.cc (right): > > https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... > ash/system/toast/toast_overlay.cc:249: if (!is_visible_ && !close_event_fired_) > { > On 2016/04/01 19:15:10, oshima wrote: > > On 2016/04/01 18:32:35, yoshiki wrote: > > > On 2016/03/30 19:11:50, oshima wrote: > > > > can't you just check overlay_widget_'s visibility? > > > > > > Here overlay_widget_'s visibility is synced with |is_visible_|. > > > > My question was do you even need is_visible_. Can't we jsut use > > overlay_widget_->IsVisible() ? > > Sorry I was wrong. When this method is called, widget is still visible. So we > need |is_invisible_| to know that this is hiding (or showing) animation. In that case, can you use the target visibility? (aura::Window::TargetVisibility()
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
On 2016/04/06 21:36:37, oshima wrote: > On 2016/04/06 17:34:07, yoshiki wrote: > > I updated the patch with set. That is because I found multiple animations are > > happened when toast is closing and showing. In previous way, the > > onClosed-delegate is called on the first OnLayerAnimationEnded-handler. But it > > was wrong. > > > > PTAL again? Thanks. > > > > > https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... > > File ash/system/toast/toast_overlay.cc (right): > > > > > https://codereview.chromium.org/1841563003/diff/20001/ash/system/toast/toast_... > > ash/system/toast/toast_overlay.cc:249: if (!is_visible_ && > !close_event_fired_) > > { > > On 2016/04/01 19:15:10, oshima wrote: > > > On 2016/04/01 18:32:35, yoshiki wrote: > > > > On 2016/03/30 19:11:50, oshima wrote: > > > > > can't you just check overlay_widget_'s visibility? > > > > > > > > Here overlay_widget_'s visibility is synced with |is_visible_|. > > > > > > My question was do you even need is_visible_. Can't we jsut use > > > overlay_widget_->IsVisible() ? > > > > Sorry I was wrong. When this method is called, widget is still visible. So we > > need |is_invisible_| to know that this is hiding (or showing) animation. > > In that case, can you use the target visibility? > (aura::Window::TargetVisibility() I can use LayerAnimator::GetTargetVisiblity(). PTAL.
https://codereview.chromium.org/1841563003/diff/120001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1841563003/diff/120001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:258: if (inflight_animations_.size() == 0) { Can you use animator->is_animating() instead of managing the queued animation yourself?
https://codereview.chromium.org/1841563003/diff/120001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1841563003/diff/120001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:258: if (inflight_animations_.size() == 0) { On 2016/04/07 17:52:02, oshima wrote: > Can you use animator->is_animating() instead of managing the queued animation > yourself? I want to run the code only once. Checking by animator->is_animating() can't avoid it from running multiple times, since multiple animations run simultaneously.
Oshima, ping?
On 2016/04/11 06:22:58, yoshiki wrote: > Oshima, ping? I'll review this soon. I want to double check if extra set is really necessary.
Thank you for investigation. Could you approve this if it's ok? Thanks.
https://codereview.chromium.org/1841563003/diff/140001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1841563003/diff/140001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:224: You can use ImplicitAnimationObserver to implement an action to take upon completion. ui::ScopedAnimationSettings settings(animator); settings.AddObserver(your_observer); here. Then you should be able to check if it's still animating in Completed to make sure that there is another animation going.
Oshima-san, PTAL. https://codereview.chromium.org/1841563003/diff/140001/ash/system/toast/toast... File ash/system/toast/toast_overlay.cc (right): https://codereview.chromium.org/1841563003/diff/140001/ash/system/toast/toast... ash/system/toast/toast_overlay.cc:224: On 2016/04/13 10:03:18, oshima wrote: > You can use ImplicitAnimationObserver to implement an action to take upon > completion. > > ui::ScopedAnimationSettings settings(animator); > settings.AddObserver(your_observer); > > > here. Then you should be able to check if it's still animating in Completed to > make sure that there is another animation going. Done.
lgtm thanks!
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1841563003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1841563003/160001
Message was sent while issue was closed.
Description was changed from ========== ARC Toast: Prevent onClosed event from being called multiple times BUG=b/25797993 ========== to ========== ARC Toast: Prevent onClosed event from being called multiple times BUG=b/25797993 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== ARC Toast: Prevent onClosed event from being called multiple times BUG=b/25797993 ========== to ========== ARC Toast: Prevent onClosed event from being called multiple times BUG=b/25797993 Committed: https://crrev.com/8079b4d3bda9c25b7f20c80bd206e3bf11176612 Cr-Commit-Position: refs/heads/master@{#387499} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8079b4d3bda9c25b7f20c80bd206e3bf11176612 Cr-Commit-Position: refs/heads/master@{#387499} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
