Use entry URLs instead of entry IDs to determine when to dismiss the overscroll overlay.

Use entry URLs instead of entry IDs to determine when to dismiss the overscroll overlay.

This is a band-aid fix for crbug.com/415167, which I'd like to try to get into M38.
Apparently it's possible to have "temporary" entry IDs set as pending and then
thrown away. This confuses OverscrollNavigationOverlay into thinking that the
page was never loaded and the overlay never gets dismissed as a result, which
makes the webpage unusable.
Using URLs is not as reliable avoids this problem. It can introduce false
positives in some corner cases, however a false positive doesn't have severe
consequences - we simply dismiss the overlay too soon, and the use may see an
empty page or a flicker.

BUG=415167
Committed: https://crrev.com/c0f6e41d97c2527b165600f3a9e139a813b35b81
Cr-Commit-Position: refs/heads/master@{#295811}

[mfomitchev, 2014-09-17 21:08:53]

mfomitchev@chromium.org changed reviewers:
+ creis@chromium.org, sadrul@chromium.org

[sadrul, 2014-09-17 21:18:13]

lgtm

[mfomitchev, 2014-09-17 21:21:53]

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

[Charlie Reis, 2014-09-17 23:51:26]

Hmm.  What happens if the URL redirects?  The committed URL might not be the
same as what started the navigation.

Is there a reason that we don't dismiss the overlay on *any* commit?  Even if
the gesture navigation was interrupted by renderer-initiated navigation to a
different page, it seems like the overlay should go away, right?

[mfomitchev, 2014-09-18 04:04:06]

message: Correct me if I am wrong, but I think the URLs that redirect wouldn't
show up in navigation history? I.e. if you go from URL1 to URL2 which redirects
to URL3, then you'd have URL1 and URL3 in your history, but not URL2: going back
from URL3 would go to URL1 and going forward from URl1 would go to URL3. So for
the purpose of gesture-nav redirection should be irrelevant.

The reason we don't dismiss the overlay on any commit is the case where you
swipe through multiple pages quickly without waiting for them to load. In this
case we don't want the overlay to go away until your final page commits and gets
a first paint. If we dismiss the overlay on any commit, there will be flicker.

[Charlie Reis, 2014-09-18 16:38:45]

On 2014/09/18 04:04:06, mfomitchev wrote:
> message: Correct me if I am wrong, but I think the URLs that redirect wouldn't
> show up in navigation history? I.e. if you go from URL1 to URL2 which
redirects
> to URL3, then you'd have URL1 and URL3 in your history, but not URL2: going
back
> from URL3 would go to URL1 and going forward from URl1 would go to URL3. So
for
> the purpose of gesture-nav redirection should be irrelevant.

Unfortunately, that's not always true.  Suppose you're logged into a site and go
from page A to B.  Much later, you go back to A, which now redirects you to the
login page rather than returning to A.

> The reason we don't dismiss the overlay on any commit is the case where you
> swipe through multiple pages quickly without waiting for them to load. In this
> case we don't want the overlay to go away until your final page commits and
gets
> a first paint. If we dismiss the overlay on any commit, there will be flicker.

In that case, what dismisses the overlay if a slow back navigation is
interrupted by a different navigation (e.g., from a script, or from a bookmark)?

[mfomitchev, 2014-09-18 21:01:54]

> Unfortunately, that's not always true.  Suppose you're logged into a site and
go
> from page A to B.  Much later, you go back to A, which now redirects you to
the
> login page rather than returning to A.

Thanks for pointing this out! PTAL at the new version.

> > The reason we don't dismiss the overlay on any commit is the case where you
> > swipe through multiple pages quickly without waiting for them to load. In
this
> > case we don't want the overlay to go away until your final page commits and
> gets
> > a first paint. If we dismiss the overlay on any commit, there will be
flicker.
> 
> In that case, what dismisses the overlay if a slow back navigation is
> interrupted by a different navigation (e.g., from a script, or from a
bookmark)?

If this was a cross-site navigation, WebContentsViewAura would get
CreateViewForWidget callback and call StartObserving on the overlay, which will
make it update the pending entry it is waiting for. However, if this is an
in-site navigation, there will be a problem. So I changed the code to always
dismiss the overlay when it receives DidStopLoading event - this should be a
decent catch-all for all the edge-cases.
Please let me know if you can suggest a better way (e.g. if there is a good way
to be notified of such interrupting navigations, we could call StartObserving()
or just dismiss the overlay).

[Charlie Reis, 2014-09-18 21:46:23]

This seems like it should work, and it makes me feel better to have the
DidStopLoading dismissal so that we don't leave the overlay up forever if the
URLs don't match.

Any chance we can add a test for this to OverscrollNavigationOverlayTest?  You
can initiate a navigation in the WebContents (after the overlay is showing)
without committing it with:

contents()->GetController().LoadURL(
      url, Referrer(), PAGE_TRANSITION_LINK, std::string());

That should cancel the current pending entry and create a new one.

On 2014/09/18 21:01:54, mfomitchev wrote:
> Please let me know if you can suggest a better way (e.g. if there is a good
way
> to be notified of such interrupting navigations, we could call
StartObserving()
> or just dismiss the overlay).

You could listen for DidFailProvisionalLoad, but I don't think that we'll always
hear that (e.g., with the fast back/forward races).

[mfomitchev, 2014-09-18 23:01:35]

On 2014/09/18 21:46:23, Charlie Reis wrote:
> This seems like it should work, and it makes me feel better to have the
> DidStopLoading dismissal so that we don't leave the overlay up forever if the
> URLs don't match.
> 
> Any chance we can add a test for this to OverscrollNavigationOverlayTest?  You
> can initiate a navigation in the WebContents (after the overlay is showing)
> without committing it with:
> 
> contents()->GetController().LoadURL(
>       url, Referrer(), PAGE_TRANSITION_LINK, std::string());
> 
> That should cancel the current pending entry and create a new one.

I think OverscrollNavigationOverlayTest.MultiNavigation_LoadingUpdate (which
actually wasn't passing after this change) should do the trick. I've just
updated it to test that loading_complete_ flag is set and the overlay is
dismissed even if the pending navigation is not committed.

[Charlie Reis, 2014-09-18 23:03:46]

Ok, LGTM.

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

https://codereview.chromium.org/575203002/diff/40001/content/browser/web_cont...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:32: it !=
redirect_chain.end();
nit: One more space on this line and the line below, to line up with the
initializer.

[Charlie Reis, 2014-09-18 23:05:34]

On 2014/09/18 23:03:46, Charlie Reis wrote:
>
https://codereview.chromium.org/575203002/diff/40001/content/browser/web_cont...
> File content/browser/web_contents/aura/overscroll_navigation_overlay.cc
(right):
> 
>
https://codereview.chromium.org/575203002/diff/40001/content/browser/web_cont...
> content/browser/web_contents/aura/overscroll_navigation_overlay.cc:32: it !=
> redirect_chain.end();
> nit: One more space on this line and the line below, to line up with the
> initializer.

Oh, interesting.  I meant for this comment to be sent on my previous reply, but
I guess I used "reply" rather than "Publish Comments" at the time.  Anyway, LGTM
with nit.

[mfomitchev, 2014-09-19 19:40:20]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2014-09-19 19:40:24]

The CQ bit was unchecked by mfomitchev@chromium.org

[mfomitchev, 2014-09-19 20:03:19]

The CQ bit was checked by mfomitchev@chromium.org

[mfomitchev, 2014-09-19 20:03:55]

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

https://codereview.chromium.org/575203002/diff/40001/content/browser/web_cont...
content/browser/web_contents/aura/overscroll_navigation_overlay.cc:32: it !=
redirect_chain.end();
On 2014/09/18 23:03:46, Charlie Reis wrote:
> nit: One more space on this line and the line below, to line up with the
> initializer.

Done.

[commit-bot: I haz the power, 2014-09-19 20:26:19]

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

[commit-bot: I haz the power, 2014-09-19 20:26:21]

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

[commit-bot: I haz the power, 2014-09-19 20:51:26]

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

[commit-bot: I haz the power, 2014-09-19 20:51:27]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[mfomitchev, 2014-09-19 20:58:23]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-09-19 20:59:45]

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

[commit-bot: I haz the power, 2014-09-19 21:53:25]

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

[commit-bot: I haz the power, 2014-09-19 21:53:26]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[mfomitchev, 2014-09-19 21:54:10]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-09-19 21:56:09]

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

[commit-bot: I haz the power, 2014-09-19 21:56:12]

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

[commit-bot: I haz the power, 2014-09-19 23:35:40]

Committed patchset #5 (id:80001) as 8abda5c7bbf012c2ce5696d444cdec5f61a2d13b

[commit-bot: I haz the power, 2014-09-19 23:40:18]

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