marq@ PTAL

subhashinik@ I don't see this in the latest canary (70.0.3502.0), can you verify?

Tested on iPhone 5S, iOS 10.3.3

This happens on latest canary as well.

https://drive.google.com/file/d/172vo05PMrKZVzVD9R3iASM6NOlThTrB0/view

@marq this is confirmed in latest canary, ptal

Load-shedding to kkhorimoto@

After spending some time digging into this, it looks like this is occurring because of overlapping animations.  The new tab button goes through TabGridMediator and inserts a WebState into the WebStateList.  The resulting WSLO callback kicks off the fade-in and scale animations for the new tab being added in GridViewController's |-insertItem:atIndex:selectedItemID:|.  Then, before this animation can finish, GridTransitionAnimation's expansion animation starts.  The flicker is the result of transition animation occurring before the insertion animation can finish.

Some possible solutions:
- Wait until the insertion animation finishes before presenting the tab.  This would likely be easiest to implement, but may result in an overly-long new tab animation.
- Prevent the insertion animation from occurring, and handle animation entirely in the animator.  This would likely require special casing and the assumption that WebStates are only inserted into the list for new tabs that are immediately activated.

I tried waiting until after the insertion animation finishes to present the view controller, but even after speeding up the insertion there's still a pretty unpleasant lag.  I'm going to attempt the second approach described above, where we forgo the insertion animation altogether if we know that we're about to start the presentation transition animation.

Video of first attempt:
https://drive.google.com/open?id=1oRzsowGhINzK8TNUwgDEyYmYhc93aYaF

By disabling the UICollectionView updates while the grid item is being inserted, I was able to produce a new tab animation that is non-jittery, but originates from the center of the screen instead of the tab's ultimate location in the grid.  This seems a reasonable solution, WDYT Mark & Pete?  I'm trying to get it to work such that we get a non-jittery animation that begins at the ultimate location within the tab grid, but it's proving somewhat difficult, as it doesn't seem possible to disable the implicit UICollectionView animation that insert the tab (I tried wrapping the calls in UIView |+performWithoutAnimations:| call).  I'll keep working at this to try to avoid updating the UICollectionView, but handle the insertion from within the GridViewController's |-transitionLayout| calculation.

Video:
https://drive.google.com/open?id=1pJH2WvAgEfMRjItc5pVyVJzZ1BpTvLrk

I'd want to see how that looks on iPad as well, but I'm (reluctantly) OK with that as an interim fix given where we are in the release cycle.

Another approach to disable the animation might be to wrap the collection view update in a CATransaction that sets the animation duration to zero.

WRT handling insertion from within -transitionLayout -- that makes me nervous; -transitionLayout should not mutate the the collection view or collection view model state at all.

Okay, I did a quick experiment and having GridLayout's -initialLayoutAttributesForAppearingItemAtIndexPath: method return [[super layoutAttributesForItemAtIndexPath:itemIndexPath] copy] gets rid of the new tab appearance animation. 

(We do only want that to happen for the case where a new tab is created; tabs that are inserted by restoring a session or undoing close-all should keep the animation).

Hmm interesting.  I tried your suggestions (both disabling animations in the layer and updating the GridLayout), but there was still a flicker when the presentation started.  I was looking at a |-transitionLayout| based approach, which would be easy for the pre-existing tabs, but since we need the actual cell object to construct the GridTranstionCell, it seems a bit hacky to try to create a cell specifically for the animation.  Here are some videos of approaches I've tried so far; I'll upload a CL for the solution that doesn't update the UICollectionView.

No Fix (for comparison):https://drive.google.com/open?id=1BSWrYSMG_zubKfsBeKfkvfMXVnQrpxDL

Don't Update Collection View:
iPhone : https://drive.google.com/open?id=1pJH2WvAgEfMRjItc5pVyVJzZ1BpTvLrk
iPad : https://drive.google.com/open?id=1qPpwo2UVo0TLplACix_CXj1ahOnMZg2f

Present after insertion (too slow):
https://drive.google.com/open?id=1oRzsowGhINzK8TNUwgDEyYmYhc93aYaF

Attempt to Disable Animations (GridLayout + CATransaction approaches combined; flicker still there):
https://drive.google.com/open?id=1oRzsowGhINzK8TNUwgDEyYmYhc93aYaF

Here's the CL that prevents UICollectionView updates from occurring: crrev.com/c/1161505.  I'll continue looking at this today to try to find a way to effectively prevent the flicker while keeping the animation start location.

After more experimentation, I tried NSLogging the animation keys of all layers in subviews of self.collectionView after the insertion.  It turns out that even if we disable animations, they are still added if the layouts returned by GridLayout aren't exactly the same as the final layout.  They are probably using their own CATransaction to ensure the animation occurs.  I added GridLayout.animatesItemUpdates in crrev.com/c/1161560, which returns the tab's final location in the collection view if the property is disabled.  When i NSLog the CALayer animations after this fix, there are indeed zero animations added.  However, it looks like there is still a small delay between when the insertion occurs and when the presentation begins.  I've inserted a video of this below.  Curiously, it seems that the duration of that pause is the same regardless of whether slow animations are enabled on the simulator, suggesting that this pause is unrelated to animations.  It may be some latency created by the UICollectionView batch update implementation, but I'm not sure how to verify that or fix it.

video:
https://drive.google.com/open?id=1-CuC_duULj0sOxJhfER_fn4OBiVm7L7J

I've also noticed that tapping on pre-existing tabs also has what seems like the same latency.  This means it may be related to snapshotting rather than UICollectionView batch updates, as no cells are being inserted when opening a preexisting tab.  We're calling |-snapshotViewAfterScreenUpdates:YES| multiple times when constructing the transitionLayout; maybe this is the culprit?

The video in #18 looks like what I had after hacking on GridLayout, including the noticeable delay before the transition starts.

As far as snapshotting goes, we snapshot all of the non-presented grid cells, and then take up to three snapshots of the BVC (content and each toolbar). I'd want to see profiler output for evidence that that's causing latency, although some of those may not require afterUpdates to be YES.

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

commit 2d89d8520da08cea27e3055f005488efcb0116c0
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Fri Aug 03 19:58:08 2018

[iOS] Don't animate grid item insertions before presenting NTP.

This CL adds GridLayout.animatesItemUpdates, which can be used to
disable animations from occurring when items are inserted or removed.
This is done to prevent the interleving of the new tab insertion
animation and the tab presentation animation.

Bug:  864982 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I7867fc475a6d399a8ecbe45df456fd6082f6d8eb
Reviewed-on: https://chromium-review.googlesource.com/1161560
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580631}
[modify] https://crrev.com/2d89d8520da08cea27e3055f005488efcb0116c0/ios/chrome/browser/ui/tab_grid/grid/grid_layout.h
[modify] https://crrev.com/2d89d8520da08cea27e3055f005488efcb0116c0/ios/chrome/browser/ui/tab_grid/grid/grid_layout.mm
[modify] https://crrev.com/2d89d8520da08cea27e3055f005488efcb0116c0/ios/chrome/browser/ui/tab_grid/grid/grid_view_controller.h
[modify] https://crrev.com/2d89d8520da08cea27e3055f005488efcb0116c0/ios/chrome/browser/ui/tab_grid/grid/grid_view_controller.mm
[modify] https://crrev.com/2d89d8520da08cea27e3055f005488efcb0116c0/ios/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm

Fix has landed; adding next action date for canary verification.

The NextAction date has arrived: 2018-08-06

Verified in today's canary; requesting merge.

This bug requires manual review: Less than 25 days to go before AppStore submit on M69
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

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

Hi Kurt, which canary did you verify in because today's canary failed? https://chromedash.googleplex.com/builds?platform=iOS

Bug here: crbug/870577

I upgraded my canary today, but it may have been built over the weekend.  The version number is 70.0.3515.0.

Ok approved.

Issue verified 
Version: Chrome Canary 70.0.3515.0
Device: iPhone 8
iOS: 11.4

Animation is smooth
https://drive.google.com/open?id=1w7uNoJZ9AVzHkjD0r5AuDkiM_uw8itYO
https://screenshot.googleplex.com/RYTBH3QLuKj

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

commit fecd135355a2cbaef246c35d9550c121ef561d7c
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Tue Aug 07 17:43:48 2018

[iOS] Don't animate grid item insertions before presenting NTP.

This CL adds GridLayout.animatesItemUpdates, which can be used to
disable animations from occurring when items are inserted or removed.
This is done to prevent the interleving of the new tab insertion
animation and the tab presentation animation.

Bug:  864982 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I7867fc475a6d399a8ecbe45df456fd6082f6d8eb
Reviewed-on: https://chromium-review.googlesource.com/1161560
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#580631}(cherry picked from commit 2d89d8520da08cea27e3055f005488efcb0116c0)
Reviewed-on: https://chromium-review.googlesource.com/1165702
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#467}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/fecd135355a2cbaef246c35d9550c121ef561d7c/ios/chrome/browser/ui/tab_grid/grid/grid_layout.h
[modify] https://crrev.com/fecd135355a2cbaef246c35d9550c121ef561d7c/ios/chrome/browser/ui/tab_grid/grid/grid_layout.mm
[modify] https://crrev.com/fecd135355a2cbaef246c35d9550c121ef561d7c/ios/chrome/browser/ui/tab_grid/grid/grid_view_controller.h
[modify] https://crrev.com/fecd135355a2cbaef246c35d9550c121ef561d7c/ios/chrome/browser/ui/tab_grid/grid/grid_view_controller.mm
[modify] https://crrev.com/fecd135355a2cbaef246c35d9550c121ef561d7c/ios/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm

Verified in 69.0.3497.31 Beta,  iPhone 6 iOS 10.3.3, iPhone 7 iOS 12, iPad Air  iOS11.4

Verified in 69.0.3497.31 Beta,  iPhone 6 iOS 10.3.3, iPhone 7 iOS 12, iPad Air  iOS11.4