Issue 880263  has been merged into this issue.

Over to marq@ for the animation changes, but in scrub we weren't able to reproduce this.   pmadalla@ can you please confirm this is still reproducible and on which devices (iPhone X vs SE, etc) and if it reproduces in 70 or just 71.

Issue can be reproduced on first launch upon fresh install 
Tested on the build version 71.0.3544.0 Canary tested on iPhone7+(iOS 11.4.1),iPhone6(iOS 10.3.3) & iPhone8(11.4.1).

Works fine in M70

Video :
https://drive.google.com/file/d/190lIpRF9qxKS3oDM3e8IGAcjPBzD0MEg/view?usp=sharing

So this is a regression between M70 and M71?

pmadalla@ can you confirm that this ONLY repros on a first launch of a fresh install?

I just reproduced this on M71 latest canary. This happens all the times not just on the first launch. You need to disable #slim-navigation-manager from about://flags.

Reply comment#5: Doesn't repro on M70.0.3538.0 #a20f097 Repros on M71.0.3539.0 #110acb1

Issue can be reproduced in M70 as well tested on the build 70.0.3538.11 beta tested on iPhone8+(11.4.1)

Video:
https://drive.google.com/file/d/1WoDztZxVHiGaRTbxCMWdDIHDAPk8oSKi/view?usp=sharing

Issue cannot be reproduced in 70.0.3538.10 beta tested on iPhone8+(11.4.1).
Tested by disable slim-navigation flag and relaunching chrome.

M70.0.3538.10 video :
https://drive.google.com/file/d/137hjxYbwtsmdyy_YGIKmut_a-vbvquBO/view?usp=sharing

issue can be reproduced in 70.0.3538.11 beta tested on iPhone8+(11.4.1)

M70.0.3538.11 video :
https://drive.google.com/file/d/1DI3gaVm2Y6VoV_RtnxB8n3fM9RJLEdCT/view?usp=sharing

This is blocking my NTP post-refresh cleanup, FYI.

Offending CL: https://chromium-review.googlesource.com/c/chromium/src/+/1148596

What's broken:

- (void)animateNewTab:(Tab*)tab
    inForegroundWithCompletion:(ProceduralBlock)completion {

animates differently for a web page vs the NTP.  It checks the NTP like so:

  GURL tabURL = tab.webState->GetLastCommittedURL();
  // Vislble URL should be more correct here than last committed, but for
  // safety, limiting the scope only to WKBasedNavigationManager, which needs
  // it to correctly animate NTP. See https://crbug.com/819606.
  if (web::GetWebClient()->IsSlimNavigationManagerEnabled())
    tabURL = tab.webState->GetVisibleURL();

It turns out with this change 'tab.webState->GetLastCommittedURL()' is empty, which is a regression.

However, |tab.webState->GetVisibleURL()| is correct.

It wouldn't be unreasonable to switch to GetVisibleURL() to fix this bug, but it seems like a larger issue that crrev.com/c/1148596 introduced a regression in GetLastCommittedURL?  Or perhaps there's something else going on?

Since this is in M70, changing milestone.

Over to kkhorimoto@ for crrev.com/c/1148596 

Issue 887679 has been merged into this issue.

This is happening because crrev.com/c/1148596 moves the LoadIfNecessary() call to the KeyedService that enabled web usage for WebStateLists.  This service is built upon the WebStateListObserver API, much like the TabModelWebUsageEnabledObserver used to in the past.  However, the TabModel version of that observer was instantiated upon creation of the TabModel, so it was always first in the observer list, meaning that the LoadIfNecessary() call happened before the animation frame is calculated, and the NTP URL was already committed at that time.  With the KeyedService, however, the observer is not added until the first time webUsageEnabled is updated for the WebStateList, so the observer is added later.  I think the fix here is to just use GetVisibleURL() instead of GetLastCommittedURL().  For the sake of mitigating risk for merges, I'm going to fix this in two CLs: 1) will update to the visible URL only for this scenario (i.e. web usage is enabled and we're not disabling the LoadIfNecessary() behavior), and 2) will update to use the visible URL for all scenarios.

CL 1 (for cherry-picking): crrev.com/c/1241555
CL 2: crrev.com/c/1241557

+ Kariah for a heads up about upcoming merge request.

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

commit e5b36c21e875ad87c5b14eff127676246e08f13a
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Wed Sep 26 00:12:45 2018

[iOS] Use visible URL for BVC animtation frame calculations.

This CL updates BVC's new tab animation to use the visible URL when it
is expected to be loaded by the WebStateListWebUsageEnabler.

Bug:  880262 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I5d948051d39a016f25987fca8840c40c7c8ae99b
Reviewed-on: https://chromium-review.googlesource.com/1241555
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594153}
[modify] https://crrev.com/e5b36c21e875ad87c5b14eff127676246e08f13a/ios/chrome/browser/ui/browser_view_controller.mm

crrev.com/c/1241555 has landed.  This is the CL that should be merged into M70.  I'll hold off on landing the long-term fix so that we can get canary verification on the CL that will be cherry-picked.  I'll merge the follow-up CL after the first one is merged.

This bug requires manual review: Less than 16 days to go before AppStore submit on M70
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

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

Canary verification please.

Verified in canary 71.0.3562.0

I understand you just want to merge crrev.com/c/1241555 for now. But I am confused on what's happening with crrev.com/c/1241557? Will it be merged to M70 later? Or will it just land on canary at some point in the future? Or something else?

crrev.com/c/1241557 will be landed onto trunk and will be shipped with M71.  It contains similar logic to crrev.com/c/1241555, but applies is unilaterally rather than in the specific case in which the this bug is manifesting.  Landing crrev.com/c/1241555 carries slightly more risk, so it's better to let that bake on trunk for a while rather than cherry-picking it over to M70.  Visually, there is no difference between the two CLs.

Kurt, do you mean landing crrev.com/c/1241557 "carries slightly more risk" or you meant crrev.com/c/1241555?

Yes, I meant crrev.com/c/1241557 carries more risk.  crrev.com/c/1241555 is the cherry-pickable CL that's already landed and only changes code behavior in a specific scenario to prevent this bug.  I'm requesting merge for that one. crrev.com/c/1241557 changes behavior for *all* new tabs, so it is slightly riskier to merge.  crrev.com/c/1241557 is still the correct long-term solution, but it'd be better not to cherry-pick it since there might be unintended consequences.

Ok thanks. Approved!

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

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

Commit: d1e597ff4e167077afaf3e6302449e9dac171c1f
Author: kkhorimoto@chromium.org
Commiter: kkhorimoto@chromium.org
Date: 2018-10-01 18:22:07 +0000 UTC

[iOS] Use visible URL for BVC animtation frame calculations.

This CL updates BVC's new tab animation to use the visible URL when it
is expected to be loaded by the WebStateListWebUsageEnabler.

Bug:  880262 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I5d948051d39a016f25987fca8840c40c7c8ae99b
Reviewed-on: https://chromium-review.googlesource.com/1241555
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#594153}(cherry picked from commit e5b36c21e875ad87c5b14eff127676246e08f13a)
Reviewed-on: https://chromium-review.googlesource.com/1255051
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#780}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}

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

commit d1e597ff4e167077afaf3e6302449e9dac171c1f
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Mon Oct 01 18:22:07 2018

[iOS] Use visible URL for BVC animtation frame calculations.

This CL updates BVC's new tab animation to use the visible URL when it
is expected to be loaded by the WebStateListWebUsageEnabler.

Bug:  880262 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I5d948051d39a016f25987fca8840c40c7c8ae99b
Reviewed-on: https://chromium-review.googlesource.com/1241555
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#594153}(cherry picked from commit e5b36c21e875ad87c5b14eff127676246e08f13a)
Reviewed-on: https://chromium-review.googlesource.com/1255051
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#780}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/d1e597ff4e167077afaf3e6302449e9dac171c1f/ios/chrome/browser/ui/browser_view_controller.mm

Verified on 71.0.3568.0 Canary, iPhone X  iOS 11.4.1, iPhone 7 iOS 12.0 beta#12, iPad Air  iOS11.4
Looks good

Verified the issue on the build 70.0.3538.44 beta tested on iPhone 7+(iOS 12).
No patches or flickering is seen on opening a newtab, looks good

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

commit 54148b4231bfd2491db0fd1cb5c8d272fb3dd603
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Wed Oct 03 23:38:31 2018

[iOS] Always use visible URL for BVC new tab animations.

This CL updates BVC to always use the visible URL when calculating the
final frame of the NTP for animtions.

Bug:  880262 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: Ie896009aff289b505ba3365afdc7ca9827058cd8
Reviewed-on: https://chromium-review.googlesource.com/c/1241557
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596426}
[modify] https://crrev.com/54148b4231bfd2491db0fd1cb5c8d272fb3dd603/ios/chrome/browser/ui/browser_view_controller.mm