Correct location bar behavior on load fail

Correct location bar behavior on load fail

This change has the toolbar manager check if an onDidFailLoad event
is triggered due to a navigation to a different page. If there is a
pending navigation from the NTP, the location bar needs to be showing
to display the URL to the user, otherwise it should disappear.

BUG=527401
Committed: https://crrev.com/86518f41ac286bf5156ef3140a2356d684cf773d
Cr-Commit-Position: refs/heads/master@{#409355}

[mdjones, 2016-08-02 18:45:34]

mdjones@chromium.org changed reviewers:
+ tedchoc@chromium.org

[mdjones, 2016-08-02 18:45:35]

ptal

[Ted C, 2016-08-02 20:55:19]

https://codereview.chromium.org/2207583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2207583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:422:
boolean isFailingToNtp = tab.getWebContents() != null
Hmm...I'm a bit confused what this is checking for.

Is the problem something like:

1.) You're on the NTP
2.) You tap a most visited item <foo.com>, navigation happens to foo.com
3.) You tap another most visited item, navigation in #2 is failed, new
navigation to foo.com is started

Wondering if #3 is right.

I'm wondering if the visible URL is the best indicator.  Or whether we should
look for a pending navigation.

isFailingToNtp also is a bit confusing to me/hard to grok.  My thinking about
pending navigations would be moving towards naming this hasPendingNavigation or
something as that is clearer where we wouldn't reset the state if there was a
pending navigation.  No idea if we can figure this out at this point though.

[mdjones, 2016-08-02 21:39:26]

Description was changed from

==========
Correct location bar behavior on load fail

This change has the toolbar manager check if an onDidFailLoad event
is failing to the NTP or a different web page. Since the location
bar needs to be invisible on the NTP, this change allows it to
correctly know whether or not it should be showing.

BUG=527401
==========

to

==========
Correct location bar behavior on load fail

This change has the toolbar manager check if an onDidFailLoad event
is triggered due to a navigation to a different page. If there is a
pending navigation from the NTP, the location bar needs to be showing
to display the URL to the user, otherwise it should disappear.

BUG=527401
==========

[mdjones, 2016-08-02 21:39:42]

On 2016/08/02 20:55:19, Ted C wrote:
>
https://codereview.chromium.org/2207583002/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
> (right):
> 
>
https://codereview.chromium.org/2207583002/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:422:
> boolean isFailingToNtp = tab.getWebContents() != null
> Hmm...I'm a bit confused what this is checking for.
> 
> Is the problem something like:
> 
> 1.) You're on the NTP
> 2.) You tap a most visited item <http://foo.com>, navigation happens to
http://foo.com
> 3.) You tap another most visited item, navigation in #2 is failed, new
> navigation to http://foo.com is started
> 
> Wondering if #3 is right.
> 
> I'm wondering if the visible URL is the best indicator.  Or whether we should
> look for a pending navigation.
> 
> isFailingToNtp also is a bit confusing to me/hard to grok.  My thinking about
> pending navigations would be moving towards naming this hasPendingNavigation
or
> something as that is clearer where we wouldn't reset the state if there was a
> pending navigation.  No idea if we can figure this out at this point though.

Mostly correct, user actions including clicking on another most-visited link or
the stop button will trigger a failed navigation. I updated the code to check if
there is a pending navigation as it does seem like the better choice. Also
updated description.

[Ted C, 2016-08-02 21:40:43]

lgtm

[mdjones, 2016-08-02 21:50:43]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2016-08-02 21:51:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 22:37:10]

Description was changed from

==========
Correct location bar behavior on load fail

This change has the toolbar manager check if an onDidFailLoad event
is triggered due to a navigation to a different page. If there is a
pending navigation from the NTP, the location bar needs to be showing
to display the URL to the user, otherwise it should disappear.

BUG=527401
==========

to

==========
Correct location bar behavior on load fail

This change has the toolbar manager check if an onDidFailLoad event
is triggered due to a navigation to a different page. If there is a
pending navigation from the NTP, the location bar needs to be showing
to display the URL to the user, otherwise it should disappear.

BUG=527401
==========

[commit-bot: I haz the power, 2016-08-02 22:37:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-02 22:38:56]

Description was changed from

==========
Correct location bar behavior on load fail

This change has the toolbar manager check if an onDidFailLoad event
is triggered due to a navigation to a different page. If there is a
pending navigation from the NTP, the location bar needs to be showing
to display the URL to the user, otherwise it should disappear.

BUG=527401
==========

to

==========
Correct location bar behavior on load fail

This change has the toolbar manager check if an onDidFailLoad event
is triggered due to a navigation to a different page. If there is a
pending navigation from the NTP, the location bar needs to be showing
to display the URL to the user, otherwise it should disappear.

BUG=527401

Committed: https://crrev.com/86518f41ac286bf5156ef3140a2356d684cf773d
Cr-Commit-Position: refs/heads/master@{#409355}
==========

[commit-bot: I haz the power, 2016-08-02 22:38:57]

Patchset 3 (id:??) landed as
https://crrev.com/86518f41ac286bf5156ef3140a2356d684cf773d
Cr-Commit-Position: refs/heads/master@{#409355}