Refactor GestureNavigation to eliminate code redundancy

Refactor GestureNavigation to eliminate code redundancy

Right now, Gesture Navigation has two very distinct phases: the initial
one, where the web contents window receives events and is moved, and the
follow up navigation, where the page still hasn't loaded and the user
scrolls on the overlay. The meat of this patch is to unify as much of
the logic as possible without sacrificing features. A new class is
introduced, OverscrollWindowAnimation, and the responsibility of the
existing OverscrollNavigationOverlay is steered into performing the
actual navigation.

Reland of: https://codereview.chromium.org/895543005/.

TEST=Overscroll*
BUG=467692, 464532, 420121
Committed: https://crrev.com/5384f002f839439ef666ed9688069e42ca5ccdca
Cr-Commit-Position: refs/heads/master@{#324275}

Committed: https://crrev.com/c0b2fa5d0ad1a266234240d9fac20be93b5ca0fc
Cr-Commit-Position: refs/heads/master@{#324526}

[mfomitchev, 2015-04-09 17:47:06]

mfomitchev@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[mfomitchev, 2015-04-09 17:47:06]

LGTM with nits. I think not checking for null in other places is okay.. others
may disagree.

https://codereview.chromium.org/1072123002/diff/20001/content/browser/web_con...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right):

https://codereview.chromium.org/1072123002/diff/20001/content/browser/web_con...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:217: // If
the web contents are being destroyed, GetContentNativeView() will return
Are you sure this comment is always true? Maybe just get rid of it - it's pretty
clear what we are doing anyhow.

[Nina, 2015-04-09 18:24:01]

nsatragno@chromium.org changed reviewers:
+ avi@chromium.org, sadrul@chromium.org

[Nina, 2015-04-09 18:24:02]

nsatragno@chromium.org changed required reviewers:
+ avi@chromium.org, mfomitchev@chromium.org, sadrul@chromium.org

[Nina, 2015-04-09 18:24:02]

sadrul@, avi@, can you please review the reland of the GestureNav refactoring
patch? I had to fix a bug that occurred in one of the browsertests. ONO was
being destroyed during WebContents destruction, which invalidated a pointer that
we were referencing.

Cheers!

[sadrul, 2015-04-09 19:41:24]

The fix lgtm, but you should address the ownership issue:

https://codereview.chromium.org/1072123002/diff/40001/content/browser/web_con...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right):

https://codereview.chromium.org/1072123002/diff/40001/content/browser/web_con...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:180:
window->Show();
By default, aura Windows are owned by their parents (and so when the parent
dies, it takes all its children with it to the grave). Since in this case, you
are explicitly passing ownership of the window to OWA, you should set
Window::set_owned_by_parent to false here.

[Nina, 2015-04-09 20:18:54]

Thanks for the ownership fix, sadrul@!

https://codereview.chromium.org/1072123002/diff/40001/content/browser/web_con...
File content/browser/web_contents/aura/overscroll_navigation_overlay.cc (right):

https://codereview.chromium.org/1072123002/diff/40001/content/browser/web_con...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:180:
window->Show();
On 2015/04/09 19:41:24, sadrul wrote:
> By default, aura Windows are owned by their parents (and so when the parent
> dies, it takes all its children with it to the grave). Since in this case, you
> are explicitly passing ownership of the window to OWA, you should set
> Window::set_owned_by_parent to false here.

Done.

[Avi (use Gerrit), 2015-04-09 21:16:29]

lgtm

stampity stamp

[Nina, 2015-04-09 21:16:49]

The CQ bit was checked by nsatragno@chromium.org

[Nina, 2015-04-09 21:16:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
mfomitchev@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1072123002/#ps60001
(title: "Fixed window ownership")

[commit-bot: I haz the power, 2015-04-09 21:18:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1072123002/60001

[commit-bot: I haz the power, 2015-04-09 22:31:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-09 22:33:13]

Patchset 4 (id:??) landed as
https://crrev.com/c0b2fa5d0ad1a266234240d9fac20be93b5ca0fc
Cr-Commit-Position: refs/heads/master@{#324526}

[sadrul, 2015-04-13 22:38:05]

https://codereview.chromium.org/1072123002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (left):

https://codereview.chromium.org/1072123002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1191: else if
(!navigation_overlay_)
I believe the 'else' removal was accidental? Because otherwise, we are always
creating the ONO, even when OC is disabled. Can you fix this up?

[mfomitchev, 2015-04-14 19:07:27]

https://codereview.chromium.org/1072123002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (left):

https://codereview.chromium.org/1072123002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:1191: else if
(!navigation_overlay_)
On 2015/04/13 22:38:05, sadrul wrote:
> I believe the 'else' removal was accidental? Because otherwise, we are always
> creating the ONO, even when OC is disabled. Can you fix this up?

Good catch, thanks!
https://codereview.chromium.org/1082313002