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

commit 10b5b9dd09b0e853a8ee78eb53599c0e81f14a45
Author: gambard <gambard@chromium.org>
Date: Thu Aug 10 15:15:18 2017

Going back to NTP scroll to the previous position

The scroll position on the NTP should be kept through navigation.
This CL treats the NativeContent the same way as the WebViews.

Bug:  751161 
Change-Id: Ic95c5775dff8b469b603f94fd42256a8a49142e0
Reviewed-on: https://chromium-review.googlesource.com/599811
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493392}
[modify] https://crrev.com/10b5b9dd09b0e853a8ee78eb53599c0e81f14a45/ios/chrome/browser/content_suggestions/BUILD.gn
[modify] https://crrev.com/10b5b9dd09b0e853a8ee78eb53599c0e81f14a45/ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h
[modify] https://crrev.com/10b5b9dd09b0e853a8ee78eb53599c0e81f14a45/ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
[modify] https://crrev.com/10b5b9dd09b0e853a8ee78eb53599c0e81f14a45/ios/chrome/browser/ui/ntp/new_tab_page_controller.mm
[modify] https://crrev.com/10b5b9dd09b0e853a8ee78eb53599c0e81f14a45/ios/web/public/web_state/ui/crw_native_content.h
[modify] https://crrev.com/10b5b9dd09b0e853a8ee78eb53599c0e81f14a45/ios/web/web_state/ui/crw_web_controller.mm

gambard: Is this bug fixed or more cls to come?
I still see an issue following the steps to reproduce from above.

Still working on it.

+Eugene, Kurt: The NTP does not keep its position if the user interacts with the WebView.
Because when going back to the previous page, CRWWebController record the state of the outgoing page (why?) https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?sq=package:chromium&l=1410 , overwriting the previously saved value written when leaving the NTP.
Any clue on how I can solve that?

Calling recordStateInHistory from registerLoadRequestForURL: seems reasonable to me, because registerLoadRequestForURL is called before requesting a navigation. 

Just to make sure we are on the same page. Do you observe the following?:
1.) Open the NTP with ContentSuggestions
2.) Open an article
3.) Navigate back to the NTP (tool menu -> back arrow)
4.) registerLoadRequestForURL is called 
5.) recordStateInHistory is called
6.) recordStateInHistory overrides NTP navigation item

Step 6 does not seem correct to me and recordStateInHistory should update Article's navigation item

Sorry, this fell out of my radar.
Yes, this is the behavior observed.
Note that 6 override the previous state only if the current web page has been interacted with, which is even more disturbing.
I think that 5 is the wrong step, as I don't see why the outgoing state should be recorded.

I believe step 5 is correct, because after registerLoadRequestForURL the load will start and it will be too late to save current state. Step should update Article's navigation item, not NTP navigation item. Kurt, WDYT?

There is a comment right above step 5: // Record state of outgoing page.
So it really seems related to the page we are trying to reach more than the page we are leaving. Which seems weird to me.

I think the steps in c#10 is the intended behavior.

Clarification for c#9: Yes, we only store display states for user-initiated scrolling.  We want to load a page in the "default" state, and if the page does programmatic scrolling by default when it's loaded, then it's expected that it will do it again without us restoring its display state.  In terms of actually applying the display state, that only occurs if the user hasn't interacted with the page.  This is because pages that take a long time to load could have been scrolled by the user before the DidFinishLoading() callback occurs and the recorded display state can be applied.  It would be a jarring experience to restore scroll state after the user already explicitly scrolled.

I haven't worked with the PageDisplayState stuff in a while, and won't have cycles for this soon; sending to Eugene since he's worked on this more recently than I have.

Gutier, how important is this bug? For the new architecture we getting rid of NativeContent, and I don't really have cycles to fix the code we trying to get rid of. 

I think it might be a blocker for the release of ContentSuggestions (M62). So it is important.
Just to be sure that I did not misunderstood the term "outgoing" in step 5. For me this refers to the page which will be displayed (in that case, the NTP as it is the first page displayed, the one which will be displayed once the back arrow is tapped). And at this point the _webView is nil.
If my understanding is correct, I don't understand why we want to save the state of the page which is not yet displayed.

Can you confirm or not the meaning of "outgoing", and if I am correct, can you explain why we want to record it?

I think this is a RBS.

eugenebut: Please take a look at this first thing and coordinate with gambard@ on how to proceed? The current state is a poor user experience.

I believe that "outgoing" means current page which is still displayed (article page), not new page which will be displayed (NTP). I don't think that Recording NTP is the intended behavior.

gambard@, I should have time to take a look at this by by the end of his week. Feel free to grab it, I'm not familiar with that code anyway, so it's not like I know how to fix the bug.

The behavior of this "record state of outgoing page" is really confusing to me.
Here it is:
- Open NTP: currentNavItem.URL = chrome://newtab/, self.currentURL = chrome://newtab/
- Click on a suggestion: currentNavItem.URL = http://URLPage1, self.currentURL = about:blank
- Click on a link on the page: currentNavItem.URL = http://URLPage1, self.currentURL = http://URLPage1
- Go back: currentNavItem.URL = http://URLPage1, self.currentURL = http://URLPage2
- Go forward: currentNavItem.URL = http://URLPage2, self.currentURL = http://URLPage1
- Go back from Page 1 to NTP: currentNavItem.URL = chrome://newtab/, self.currentURL = chrome://newtab/

I am in favor of removing it. Removing it only impacts the transition from one page to another by clicking on it.
I tried to remove it, I did not see any difference in browsing.

gambard@, what does "it" mean in this context? If "it" means "recordStateInHistory call inside registerLoadRequestForURL:", then removing the call will break page state restoration. Have you tried scrolling the page and then tapping reload button? Expected behavior is to fully restore the scroll state.

Yes "it" means recordStateInHistory in registerLoadRequestForURL:referrer:transition:sameDocumentNavigation:
And indeed it is breaking the reload page state, I did not try it. However, back/forward seems to work.
However, if the recording of the state is done only on reloading, everything seems to work. (There is a "if(!redirect)" which might be changed to "if(reload)".)
WDYT? Is there a case I forgot?

Page state restoration will also break for some back-forward navigations if we remove recordStateInHistory call.

Back-forward restoration works for you because WKWebView maintains navigation history and correctly restores page state iff navigation is performed via goToBackForwardListItem. There are various cases when Chrome does not use goToBackForwardListItem:
 - page restored from app relaunch
 - cookies were cleared
 - page used pushState/replaceState API

Reload was one example, where the problem would reproduce consistently.

Assigning to Kurt while Eugene is OOO.
As I mentioned in #18, the state is only recorded when you click a link on a page. Going back/forward does not store the state. So in any case, it is broken in some cases. They are other method calls that are triggering a recordStateInHistory. registerLoadRequestForURL:referrer:transition:sameDocumentNavigation: is doing nothing to save the state when going forward/back. If you prefer, we could just disable it for PAGE_TRANSITION_FORWARD_BACK.

Just chatted with Eugene about this bug.  We agree that the best long-term solution would be to change |-recordStateInHistory| to update the PageDisplayState of the last committed NavigationItem rather than the current NavigationItem.  However, this function is called from several places in the app, so making this change for branch does not seem like a good idea.

For a cherry-pickable fix, it's best to keep the change local to the |-recordStateInHistory| call in |-registerLoadRequest|.  Gauthier's solution in crrev.com/c/663723 seems like a reasonable solution because the state is already recorded for back-forward navigations via a delegate callback in NavigationManagerImpl::GoToIndex().  Eugene also is also working on a similar solution that checks against whether there is a native controller, rather than whether the navigation is back-forward.  I think this might be a better change to cherry-pick, since it's more obvious why the change exists (rather than relying on an implementation detail of NavigationManagerImpl::GoToIndex()).

I think the best solution here would be to land Eugene's or Gauthier's solution for the branch, then land a follow-up CL reverting this and updating |-recordStateInHistory| to use the last committed item.

Workaround CL: https://chromium-review.googlesource.com/663771

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

commit b011405c6037b3a84fa3cf91a7bb4299525144db
Author: Eugene But <eugenebut@google.com>
Date: Tue Sep 12 23:33:27 2017

Do not call recordStateInHistory when registering request for native controllers.

recordStateInHistory does not work correctly for native controllers
because it reads and stores the state for upcoming page, not for
outgoing page.

This change is a workaround, rather than a fix, which is fine because
native controllers support will be removed from CRWWebController.

Bug:  751161 
Change-Id: I020b5b98e890a9d6f1e3bcde64076fd9c1758d5f
Reviewed-on: https://chromium-review.googlesource.com/663771
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501452}
[modify] https://crrev.com/b011405c6037b3a84fa3cf91a7bb4299525144db/ios/web/web_state/ui/crw_web_controller.mm

Great. Thank you for reaching resolution on this. 

This bug requires manual review: Less than 29 days to go before AppStore submit on M62
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

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

Tested in 63.0.3219.0 Canary, iPhone6 iOS 10.3.3, iPhone7 iOS11, iPad Pro iOS11
and it is fixed only for the first 10 content suggestion. 

But it is not covered with this scenario:
1. Open the NTP with ContentSuggestions
2. Scroll to bottom of list
3. Tap More 
4. Open an article
5. Navigate back to the NTP (tool menu -> back arrow)

Observed:
The NTP is scrolled to the end of the content suggestion list(Showing only the first 10 items).

This seem like NTP's job to remember that more suggestions were revealed.

Please fix and request merge again

Is there a reason for rejecting b011405c6037b3a84fa3cf91a7bb4299525144db merge? b011405c6037b3a84fa3cf91a7bb4299525144db fixes page state restoration iff "More" was not tapped. Steps in Comment #32 will require a separate fix on top of b011405c6037b3a84fa3cf91a7bb4299525144db and I'm not sure if #32 case is even an RBS.

CC cmasso@ for comment #35

Requesting the merge again for b011405c6037b3a84fa3cf91a7bb4299525144db (fixes the problem described in initial steps to reproduce). Fixes for steps described in comment #32 will be landed in separate CL(s).

Sharon, how about verifying this bug according to initial steps to reproduce and filing a separate bug for scenario from comment #32? There is not much value in delaying b011405c6037b3a84fa3cf91a7bb4299525144db merge as we will only get less time for testing.

This bug requires manual review: Less than 23 days to go before AppStore submit on M62
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

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

The NTP is supposed to restore only the first 10 articles. If more is pressed, the articles are not restored. If the user was looking at the articles added with the "more" action, the view is restored at the end of the collection.
It is the same behavior on Android.

So this is WAI.

Verified in 63.0.3220.0 Canary, iPhone 6 plus iOS 10.3.3, iPhone 7 iOS11, iPad Pro iOS11
Going back to Content Suggestions should display the same content as before navigating
is working for the first 10 items on the list.

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

commit 806f219b23eb838b685f259256cee44e63c9a4de
Author: Eugene But <eugenebut@google.com>
Date: Wed Sep 20 22:06:59 2017

Do not call recordStateInHistory when registering request for native controllers.

recordStateInHistory does not work correctly for native controllers
because it reads and stores the state for upcoming page, not for
outgoing page.

This change is a workaround, rather than a fix, which is fine because
native controllers support will be removed from CRWWebController.

TBR=eugenebut@google.com

(cherry picked from commit b011405c6037b3a84fa3cf91a7bb4299525144db)

Bug:  751161 
Change-Id: I020b5b98e890a9d6f1e3bcde64076fd9c1758d5f
Reviewed-on: https://chromium-review.googlesource.com/663771
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#501452}
Reviewed-on: https://chromium-review.googlesource.com/676094
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#365}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/806f219b23eb838b685f259256cee44e63c9a4de/ios/web/web_state/ui/crw_web_controller.mm

Devices: iPhone 6 plus, iPhone 7 plus, iPad Air
iOS: 10.3.3, 11.0
Build:  62.0.3202.35 beta

Verified that The NTP should be scrolled to the same place.  Looks good.