Fixing race conditions in ui::content::WindowSlider which could cause the overscroll overlay to nev…

Fixing race conditions in ui::content::WindowSlider which could cause the overscroll overlay to never go away.

The race condition occurred because new naviagtion could happen before the callback from the animation associated with the old navigation was executed. This could get the OverscrollNavigationOverlay into a bad state.

The fix is to instantly complete the animation associated with a previous user action when new interaction occurs. Also, we now perform page navigation at the beginning of the transition animation, not in the end, which saves us 200ms in load time.

BUG=348717, 305448
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259766

[sadrul, 2014-03-19 20:21:18]

> Also, we now perform page navigation at the beginning of
> the transition animation, not in the end, which saves us
> 200ms in load time.

This is only for the repeated gesture-nav case, right? For the single
gesture-nav, you need to make changes in
WebContentsViewAura::OnOverscrollComplete()

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:241: if
(slide_direction_ == SLIDE_UNKNOWN) {
You can remove the braces

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:295: // if we
need to dismiss it.
I am not sure I understand this comment.

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/window_slider.cc (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.cc:92: ((fabs(delta_x_) >=
active_start_threshold_ && slider_.get()) ||
This seems wrong? What if the slide starts, and then ends with delta_x_ <
active_start_threshold_?

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/window_slider.h (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.h:36: virtual ui::Layer*
OnSlideBackStartedCreateLayer() = 0;
I want the window-slider to work in terms of back/front, and not necessarily in
terms of back/forward.

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.h:49: // This callback is always
preceeded by one of the OnSlide***Started()
What are the OnSlide***Started() callbacks?

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.h:57: virtual void
OnWindowFlingCompleted() = 0;
What if the gesture ends without a fling?

Can we call these OnWindowSlideWillCompleteSoon() and OnWindowSlideComplete()
instead? The delegate doesn't need to know if the slide completed because of a
fling or not. It just needs to know when the slide-complete animation starts,
and ends (so something like OnWindowSlideCompleteAnimtionStart()
OnWindowSlideCompleteAnimationEnd() also work).

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/window_slider_unittest.cc (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider_unittest.cc:428:
animator->OnThreadedAnimationStarted(cc::AnimationEvent(
I don't think we are supposed to use cc::AnimationEvents directly. Can you look
into using StartThreadedAnimationsIfNeeded() instead?

[mfomitchev, 2014-03-20 15:10:06]

Yes, the optimization only affects the case of repeated GestureNav when
WindowSlider is involved. I have a sticky note on my monitor to do this for the
main case as well, just want to sort this one out first.

I've addressed most of the feedback, just one question below.

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:241: if
(slide_direction_ == SLIDE_UNKNOWN) {
On 2014/03/19 20:21:18, sadrul wrote:
> You can remove the braces

Done.

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:295: // if we
need to dismiss it.
Yeah, it's poorly worded. Basically I was just trying to say that we need to
check if we can remove the overlay here (because we may have loaded and painted
the page, but kept the overlay because IsSlideInProgress() returned true). I'll
just remove the comment, I think it should be pretty self explanatory.

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/window_slider.cc (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.cc:92: ((fabs(delta_x_) >=
active_start_threshold_ && slider_.get()) ||
Good point! I think we can just get rid of the delta_x comparison. You can have
delta_x > active_start_threshold and slider == NULL if you are trying to
navigate past the last page, so || was wrong. I don't see any reason to look at
delta_x here...

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/window_slider.h (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.h:36: virtual ui::Layer*
OnSlideBackStartedCreateLayer() = 0;
Ah, do you mean back/front in a sense of below/above?
ONO sets slide_direction_ based on these callbacks, which is why I chose to make
it explicit in the callback names. If you don't like this, should we also rename
slide_direction_ to something else?

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.h:49: // This callback is always
preceeded by one of the OnSlide***Started()
On 2014/03/19 20:21:18, sadrul wrote:
> What are the OnSlide***Started() callbacks?

Done.

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.h:57: virtual void
OnWindowFlingCompleted() = 0;
On 2014/03/19 20:21:18, sadrul wrote:
> What if the gesture ends without a fling?
> 
> Can we call these OnWindowSlideWillCompleteSoon() and OnWindowSlideComplete()
> instead? The delegate doesn't need to know if the slide completed because of a
> fling or not. It just needs to know when the slide-complete animation starts,
> and ends (so something like OnWindowSlideCompleteAnimtionStart()
> OnWindowSlideCompleteAnimationEnd() also work).

Done.

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/window_slider_unittest.cc (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider_unittest.cc:428:
animator->OnThreadedAnimationStarted(cc::AnimationEvent(
On 2014/03/19 20:21:18, sadrul wrote:
> I don't think we are supposed to use cc::AnimationEvents directly. Can you
look
> into using StartThreadedAnimationsIfNeeded() instead?

Done.

[sadrul, 2014-03-24 20:20:46]

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/window_slider.cc (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.cc:92: ((fabs(delta_x_) >=
active_start_threshold_ && slider_.get()) ||
On 2014/03/20 15:10:06, mfomitchev wrote:
> Good point! I think we can just get rid of the delta_x comparison. You can
have
> delta_x > active_start_threshold and slider == NULL if you are trying to
> navigate past the last page, so || was wrong. I don't see any reason to look
at
> delta_x here...

sounds good

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/window_slider.h (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.h:36: virtual ui::Layer*
OnSlideBackStartedCreateLayer() = 0;
On 2014/03/20 15:10:06, mfomitchev wrote:
> Ah, do you mean back/front in a sense of below/above?

Yes.

> ONO sets slide_direction_ based on these callbacks, which is why I chose to
make
> it explicit in the callback names. If you don't like this, should we also
rename
> slide_direction_ to something else?

ONO is aware of back/forward navigation. So it should maintain state that
corresponds to back/forward navigation. WindowSlider doesn't deal with
navigation. Its responsibility is to manage the action of sliding a window above
another one. For this slide operation, it needs to know about the top/bottom (or
below/above, front/back etc.) layers. So it should maintain state that
corresponds to this. That's how I am thinking.

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/window_slider.cc (right):

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/window_slider.cc:181:
base::Unretained(shadow_.release()))));
Use base::Passed for these two

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/window_slider.cc:219:
base::Unretained(shadow_.release()))));
ditto

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/window_slider.h (right):

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/window_slider.h:99: void
FlingAnimationCompleted(ui::Layer* layer, ShadowLayerDelegate* shadow);
A window-slide can complete without a fling. We should call this something else.

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/window_slider_unittest.cc (right):

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/window_slider_unittest.cc:95: virtual
~WindowSliderDelegateTest() {}
Can you add a CHECK(!slide_completing_ || slide_completed_) to make sure
slide-completed gets called if slide-completing is called?

[mfomitchev, 2014-03-24 22:36:47]

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/window_slider.h (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.h:36: virtual ui::Layer*
OnSlideBackStartedCreateLayer() = 0;
Ok, that makes sense. I am still not a big fan of the old names, because these
callbacks are not just about the layer creation, they are also about updating
the 'target'. E.g. looking at the old names, you can imagine that you are
allowed to save front/back layers in WindowSlider rather than request a new
layer every time the slide direction changes. Perhaps call them
CreateFrontLayerAndSetAsTarget(), CreateBackLayerAndSetAsTarget()? (Or
CreateXxxLayerAndSetAsSlideTarget()?)

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/window_slider.h (right):

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/window_slider.h:99: void
FlingAnimationCompleted(ui::Layer* layer, ShadowLayerDelegate* shadow);
True, but this callback is used (only) by the UpdateForFling() method. It just
so happens that UpdateForFling() (and thus the fling animation associated with
it) is used not only when you do a fling, but also when you scroll far enough
and release. How about we rename the callback to SlideAnimationCompleted(), and
also rename UpdateForFling() to CompleteOrResetSlide() or FinishSlide()?

[mfomitchev, 2014-03-25 15:27:32]

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/window_slider.cc (right):

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/window_slider.cc:181:
base::Unretained(shadow_.release()))));
On 2014/03/24 20:20:47, sadrul wrote:
> Use base::Passed for these two

Done.

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/window_slider.cc:219:
base::Unretained(shadow_.release()))));
On 2014/03/24 20:20:47, sadrul wrote:
> ditto

Done.

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/aura/window_slider_unittest.cc (right):

https://codereview.chromium.org/202183003/diff/20001/content/browser/web_cont...
content/browser/web_contents/aura/window_slider_unittest.cc:95: virtual
~WindowSliderDelegateTest() {}
On 2014/03/24 20:20:47, sadrul wrote:
> Can you add a CHECK(!slide_completing_ || slide_completed_) to make sure
> slide-completed gets called if slide-completing is called?

Done.

[sadrul, 2014-03-25 23:01:56]

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
File content/browser/web_contents/aura/window_slider.h (right):

https://codereview.chromium.org/202183003/diff/1/content/browser/web_contents...
content/browser/web_contents/aura/window_slider.h:36: virtual ui::Layer*
OnSlideBackStartedCreateLayer() = 0;
On 2014/03/24 22:36:47, mfomitchev wrote:
> Ok, that makes sense. I am still not a big fan of the old names, because these
> callbacks are not just about the layer creation, they are also about updating
> the 'target'. E.g. looking at the old names, you can imagine that you are
> allowed to save front/back layers in WindowSlider rather than request a new
> layer every time the slide direction changes. Perhaps call them
> CreateFrontLayerAndSetAsTarget(), CreateBackLayerAndSetAsTarget()? (Or
> CreateXxxLayerAndSetAsSlideTarget()?) 

I don't understand what you mean by 'updating the target'. The ONO
implementation maintains internal state to decide what to do when the
window-slide completes. If we don't want that, then we can include that
information in the OnWindowSlideComplet*() callbacks. But I still don't see any
value in changing that.

I am not convinced there is an issue with the name as they are. I think in the
context of the WindowSlider, the names make good sense. You can update the doc
to explain that it gets called every time the direction changes if you think
that'd be useful.

[sadrul, 2014-03-26 20:40:15]

LGTM

[mfomitchev, 2014-03-26 20:41:20]

sky@: Can you please review? Thanks!

[sky, 2014-03-26 20:45:56]

LGTM

[mfomitchev, 2014-03-26 21:00:18]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2014-03-26 21:00:28]

The CQ bit was unchecked by mfomitchev@chromium.org

[mfomitchev, 2014-03-26 21:02:37]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-03-26 21:03:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/202183003/80001

[commit-bot: I haz the power, 2014-03-27 00:36:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 00:36:42]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[mfomitchev, 2014-03-27 00:39:02]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-03-27 00:39:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/202183003/80001

[commit-bot: I haz the power, 2014-03-27 02:43:18]

Change committed as 259766