marking RBB. 

Rohit can you take a look?

Assigning to Kurt for now, but Kurt, Justin, and I should chat about fixes on Monday.


This is a result of the switch from currentEntry to GetVisibleItem() in https://codereview.chromium.org/2722983003/diff/40001/ios/chrome/browser/ui/browser_view_controller.mm?context=&column_width=80&tab_spaces=8

As soon as we start a forward navigation, GetVisibleItem() still returns the old item, saying that we're on the NTP, so we keep the toolbar hidden.  But we've already removed the NTP view, so there's no toolbar to display, resulting in grey.

The NTP show/hide code needs to be synced with the toolbar show/hide code.  Justin and Kurt, do you want to chat and sort this out?

One solution is to change the logic in BVC to read:
    web::NavigationItem* item = [tab navigationManager]->GetPendingItem();
    if (!item)
      item = [tab navigationManager]->GetVisibleItem();

Another option is to change the NTP show/hide logic to use GetVisibleItem() instead of whatever it is using now (presumably currentEntry).

I'm not sure which of the two options is better from a security standpoint.  Should we continue to show the NTP until the page has responded to the original request, or should we hide the NTP and show the real toolbar as quickly as possible?

Line 1920 in that file, btw.

Any update here? We are branching today!

I also saw this on iPad when going back, not forward. 

Unfortunately, Rohit's suggested change to BVC logic also results in a poor user experience.  Before we fixed the visible URL logic for back/forward navigations, the omnibox was already updated to the destination NavigationItem's URL even before the navigation was committed, so showing the toolbar immediately worked fine.  However, now that we only update the visible URL once the back/forward navigation is committed, showing the toolbar immediately would result in a toolbar with the "Enter URL or Search..." placeholder text until the navigation is committed.  I'm working on a CL now that will delay removing the NTP until the forward navigation is committed, so we can show the toolbar and the web page at the same time.

https://codereview.chromium.org/2820013003/

Why did the old |currentEntry| code work?  What specifically does currentEntry do that sidesteps these issues?

So there are two patches of importance here:
1) Eugene's fixing of the visible URL that's displayed during back/forward history navigations.  This introduced the pending item index, which delayed updating the URL in the omnibox until the navigation is committed, thereby helping to avoid spoofing bugs.
2) My change that switches from currentItem => visibleItem.

Before (1) landed, this had the behavior of swapping out the content view with an empty web view and swapping the NTP toolbar with the web toolbar showing the URL of the page to which we were doing the forward navigation.  After (1) but before (2) landed, the behavior was that the toolbar and content were switched at the same time, but the omnibox would show "Enter URL or Search..." until the navigation was committed.  After (2) landed, we were left with the current bug where we show the gray toolbar.  My CL in comment 12 changes the behavior such that the content area and the toolbar are swapped out when the pending history navigation is committed so that the omnibox is only displayed once the visible URL is updated.

Aha, thanks for the helpful summary.

Does my suggestion (comments #5 and #10) approximate the behavior that existed after (1) landed but before (2) landed?  If yes, would that be a reasonable temporary solution to lower the risk to the M59 branch?  (Is it actually lower risk that the CL in comment #12?)

My worry with delaying the swap is that we don't show a progress bar on the NTP, so for slow-loading pages, there won't be any visible indication that we acted on a tap until the navigation is committed.

Yes, your suggestion in comment #5 of basically rewriting the currentItem logic would create the behavior between (1) and (2).  We could add this with pretty minimal risk for M59, if that's the behavior we want for this release.  It'd be a less risky change than my CL in comment #12, but I'm not sure it's a better user experience.  Would a partial progress bar for an omnibox with "Enter URL or Search..." really be better than  just showing the NTP and an activity indicator in the toolbar?  This delay before showing the toolbar would only be until the navigation is committed; once we receive an initial response from the server but before the page has finished loading, we'll still see the omnibox with the correct URL and a progress bar.

cc mardini because I'm not sure which option would be better here.  I'm partial to my solution in comment #12 because it seems less broken than showing a progress bar without an accompanying visible URL update.

Thanks, Kurt. I agree with you that it's best to delay removing the NTP and show an activity indicator until the navigation is committed so we can show the toolbar and the webpage at the same time. 

I don't think showing "Search or Enter URL" in between navigation should ever happen. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd2e9b497d8623d1c2844b6df874e42f865c2631

commit cd2e9b497d8623d1c2844b6df874e42f865c2631
Author: kkhorimoto <kkhorimoto@chromium.org>
Date: Tue Apr 18 20:55:30 2017

Display the web view after commit for non-user-initiated loads.

The toolbar's visibility and URL is dependent on the visible URL.
However, the previous implementation of |-ensureWebViewCreated| would
immediately add web view to the hierarchy, even for non-user-initiated
loads.  This CL updates the web view display logic so that it is
displayed at the same time as the toolbar updating.

BUG= 703222 

Review-Url: https://codereview.chromium.org/2820013003
Cr-Commit-Position: refs/heads/master@{#465365}

[modify] https://crrev.com/cd2e9b497d8623d1c2844b6df874e42f865c2631/ios/web/web_state/ui/crw_web_controller.mm

Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Toolbar is now displayed correctly when navigating back and forward.
Verified in iPhone 6 iOS 10.2.1, iPhone 6 plus iOS 9.3.2   Build- 60.0.3081.0 Canary

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Does this still need a merge?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f10bd0a68a24c6c58a7f4d67e5dc2d090ddce05c

commit f10bd0a68a24c6c58a7f4d67e5dc2d090ddce05c
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Mon May 08 22:03:47 2017

Display the web view after commit for non-user-initiated loads.

The toolbar's visibility and URL is dependent on the visible URL.
However, the previous implementation of |-ensureWebViewCreated| would
immediately add web view to the hierarchy, even for non-user-initiated
loads.  This CL updates the web view display logic so that it is
displayed at the same time as the toolbar updating.

BUG= 703222 

Review-Url: https://codereview.chromium.org/2820013003
Cr-Commit-Position: refs/heads/master@{#465365}
(cherry picked from commit cd2e9b497d8623d1c2844b6df874e42f865c2631)

Review-Url: https://codereview.chromium.org/2872823002 .
Cr-Commit-Position: refs/branch-heads/3071@{#470}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/f10bd0a68a24c6c58a7f4d67e5dc2d090ddce05c/ios/web/web_state/ui/crw_web_controller.mm

Verified in 59.0.3071.50 beta, iPhone 6 plus  - iOS 9.3.2, iPhone 6 -10.2.1, the issue "Toolbar flashing in grey when forwarding from NTP to any webpage" is resolved

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7c2ee628552ec6a9d624dcd1b78442991615715

commit a7c2ee628552ec6a9d624dcd1b78442991615715
Author: Eugene But <eugenebut@google.com>
Date: Thu Jul 13 19:51:05 2017

Always add WKWebView to the view hierarchy on iOS 11.

On iOS 11 WKWebView loads much slower if it is not a part of the view
hierarchy. This is more-likely a system issue, however it is important
to have a workaround until Apple fixes the bug.

BUG:  739390 ,  703222 , 734669
Change-Id: I09d5c3ffadee44d3a5a6d834d4b5f0839b20f50b
Reviewed-on: https://chromium-review.googlesource.com/566183
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486460}
[modify] https://crrev.com/a7c2ee628552ec6a9d624dcd1b78442991615715/ios/web/web_state/ui/crw_web_controller.mm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ecb05dc3f6ee2a4615eeb4b3708b32b2b537518c

commit ecb05dc3f6ee2a4615eeb4b3708b32b2b537518c
Author: Justin Cohen <justincohen@chromium.org>
Date: Wed Jul 26 18:25:21 2017

Revert "Always add WKWebView to the view hierarchy on iOS 11."

This reverts commit a7c2ee628552ec6a9d624dcd1b78442991615715.

Reason for revert: This appears fixed in Xcode 9 beta 4.

Original change's description:
> Always add WKWebView to the view hierarchy on iOS 11.
> 
> On iOS 11 WKWebView loads much slower if it is not a part of the view
> hierarchy. This is more-likely a system issue, however it is important
> to have a workaround until Apple fixes the bug.
> 
> BUG:  739390 ,  703222 , 734669
> Change-Id: I09d5c3ffadee44d3a5a6d834d4b5f0839b20f50b
> Reviewed-on: https://chromium-review.googlesource.com/566183
> Reviewed-by: Justin Cohen <justincohen@chromium.org>
> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
> Commit-Queue: Eugene But <eugenebut@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#486460}

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: Ifc1aa7a26ab6ea4a4347380e451bcf8da03dc1bb
Reviewed-on: https://chromium-review.googlesource.com/586727
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489702}
[modify] https://crrev.com/ecb05dc3f6ee2a4615eeb4b3708b32b2b537518c/ios/web/web_state/ui/crw_web_controller.mm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/540592130e1c03ce01b57f5a18d5de79c8bc7122

commit 540592130e1c03ce01b57f5a18d5de79c8bc7122
Author: Justin Cohen <justincohen@google.com>
Date: Mon Jul 31 20:24:04 2017

Revert "Always add WKWebView to the view hierarchy on iOS 11."

This reverts commit a7c2ee628552ec6a9d624dcd1b78442991615715.

Reason for revert: This appears fixed in Xcode 9 beta 4.

Original change's description:
> Always add WKWebView to the view hierarchy on iOS 11.
>
> On iOS 11 WKWebView loads much slower if it is not a part of the view
> hierarchy. This is more-likely a system issue, however it is important
> to have a workaround until Apple fixes the bug.
>
> BUG:  739390 ,  703222 , 734669
> Change-Id: I09d5c3ffadee44d3a5a6d834d4b5f0839b20f50b
> Reviewed-on: https://chromium-review.googlesource.com/566183
> Reviewed-by: Justin Cohen <justincohen@chromium.org>
> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
> Commit-Queue: Eugene But <eugenebut@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#486460}


TBR=justincohen@chromium.org

(cherry picked from commit ecb05dc3f6ee2a4615eeb4b3708b32b2b537518c)

Change-Id: Ifc1aa7a26ab6ea4a4347380e451bcf8da03dc1bb
Reviewed-on: https://chromium-review.googlesource.com/586727
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#489702}
Bug: 
Reviewed-on: https://chromium-review.googlesource.com/594948
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#176}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/540592130e1c03ce01b57f5a18d5de79c8bc7122/ios/web/web_state/ui/crw_web_controller.mm

Verified on chrome beta version 61.0.3163.25 on iPhone 7 plus/10.3.3 and iPhone 6 plus/11 beta 4, following the steps mentioned in comment #0.  Toolbar do not flash grey color on forwarding from NTP.  Looks good.