NavigationCallbacksTest.UserInitiatedHashChangeNavigation fails for the same underlying reason (not detecting same-document navigation): the page tries to perform a same-document navigation, and the call to LoadUrl waits for WebStateObserver::PageLoaded, which never happens (and when using LegacyNavigationManagerImpl, the call happens in URLDidChangeWithoutDocumentChange).

[CRWWebController webViewURLDidChange] was used to capture hash changes in the webview so we can create the correct navigation entry. I'm not sure if it has other purposes that need preserving. It sounds very similar to the dilemma in crbug/776373.

Wrong link. I was thinking  crbug.com/776392 

Eugene -- do you think we need to continue supporting these callbacks after switching to the new navigation manager? I think we have to make a decision about what events are observable at ios/web layer. The current semantics encoded in this test is that same-document navigation will be visible to WebStateObservers. WKWebView doesn't provide this guarantee. Do you know what features depend on this?

In terms of code / implementation complexity, it'll be the simplest if we match our interface to what WKWebView exposes.

It probably wouldn't be too hard to support DidStartLoading() and DidStopLoading() using KVO, but don't provide DidStartNavigation() and DidStopNavigation().

I don't think we'll be able to fully get DidStartNavigation() and DidStopNavigation() working for navigation events that are not exposed by WKNavigationDelegate because that's the problem we have today.

 Issue 776392  has been merged into this issue.

I believe we have to support these callbacks. If you do a project search for IsSameDocument() you will find all instances which care about same document-navigations. If you search for is_in_page you will also find a few cases where clients depend on hash change info (though those clients should probably rely on IsSameDocument() instead). DidStartNavigation() for same-document navigation can probably be supported by observing WKWebView.URL property.

The story with DidStartLoading() is .... complicated. For some reason Chrome for iOS uses DidStartLoading callback to count page loads and CPM. CMP has to be stable across the releases so we can't just change the behavior of DidStartLoading callback without loosing the ability to compare CMP with the previous release. One way to migrate page load count off DidStartLoading would be running 50% experiment, where Page Loads are counted using a different callback (e.g. DidStartNavigation).

Thanks Eugene!

Based on code search, I found 2 users of IsSameDocument() outside of the navigation stack itself:
- LanguageDetectionController: uses IsSameDocument() in its DidFinishNavigation() callback to kick off language detection code in JavaScript. Usually this functionality is triggered in the PageLoaded() callback.
- WebFaviconDriver: IsSameDocument() is used to cache favicons for same-document navigation.

Just thinking out loud: if we don't send Did{Start|Finish}Navigation callbacks for same-document navigation, the WebFaviconDriver use case will continue to work because they don't want to refetch favicons anyways on same-document navigation. The LanguageDetectionController case should be OK too, because the content of the page would not have changed.

There are also 2 users of is_in_page:
- InfoBarManager: is_in_page is used to calculate InfoBarDelegate::NavigationDetails::is_navigation_to_different_page, which doesn't seem to be used by any infobar managers.
- IOSTranslateDriver: Uses !is_in_page to detect when navigation happened as a proxy of when content changed (in desktop they use WebContentObserver). It seems to me that not notifying on same-document change will actually be better for them.

I can start a conversation with the owners of these use cases. If they come back with they'd rather not see same-document navigation exposed through DidStartNavigation() and DidFinishNavigation(), would you be open to changing the semantics?

==

I see that keeping DidStartLoading() semantics constant is important. Let me document the current semantic of DidStartLoading() so we can figure out a path without breaking CPM stat.

If DidStartNavigation is not called for same-document navigations then bunch of code will not execute, which will break things. Also I'm pretty sure that DidStartNavigation will be a part of navigation service and it will be important to iOS to support this callback for same-document navigation. Having said that I believe we should try to keep the existing DidStartNavigation/DidFinishNavigation behavior. Can we rely on WKWebView.URL KVO to support same-document navigation callbacks? 

Why do you think changing DidStartNavigation semantic is preferable? It requires changes in the code which do not have tests, creates potential problems for servicification, and carries risks of breaking things. The benefits is of course simpler code, but is it hard to support same-document navigations?

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

commit bdbae340b2e139f0b098cc4a0c8335739568d2c2
Author: Danyao Wang <danyao@chromium.org>
Date: Tue Nov 07 14:27:27 2017

[Nav] Reduce scope of [CRWWebController didStartLoading].

Moved NavigationManager::CommitPendingItem() to the callers explicitly
so |didStartLoading| is only concerned with updating web controller
states. This is a decouples navigation item commits from
WebStateObserver::DidFinishNavigation() callback so we can invoke the
latter when NavigationItem is created lazily on access (e.g.
same-document navigations).

Also removed unused URL argument.

Bug:  776373 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Id8ea1568a489864899efa3e0039f5127878d4542
Reviewed-on: https://chromium-review.googlesource.com/756283
Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514465}
[modify] https://crrev.com/bdbae340b2e139f0b098cc4a0c8335739568d2c2/ios/web/web_state/ui/crw_web_controller.mm

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

commit 23a351aeb879eb30e02ca06afb721f9bfbb4df25
Author: Danyao Wang <danyao@chromium.org>
Date: Fri Nov 10 19:49:59 2017

[Nav Experiment] Re-enable same-document handling for new nav manager.

Same-document handling (URL, loading KVO and pushState/replaceState
override) was originally disabled when using WKBasedNavigationManager
because I thought their primary purpose was to update the navigation
history, which is no longer needed. I overlooked that this code path
also triggers WebStateObserver callbacks, and current contract requires
that they be triggered for same-document navigations.

Although we have plans to update the contract (e.g. crbug.com/767092),
it would be done after switching to new navigation manager.

Summary of the change:
- Split overrides that are not needed and incompatible with new nav
  manager from navigation.js  to legacy_navigation.js.
- Added navigation.js back to web_bundle so it is injected even when
  new nav manager is used.
- Removed condition gating URL and loading KVO
- Moved CRWSessionController methods used by CRWWebController's state
  navigation handlers to NavigationManager (i.e. |updateCurrentItem|
  and |pushNewItemWithURL|.

This change fixes the following tests that were failing with new nav
manager:
NavigationCallbacksTest
HistoryStateOperationsTest

Bug:  776373 ,  776486 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ife803390b42ca30098deeea9ce35487bc33b1ed7
Reviewed-on: https://chromium-review.googlesource.com/762176
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515643}
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/BUILD.gn
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/navigation/crw_session_controller.h
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/navigation/crw_session_controller.mm
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/navigation/crw_session_controller_unittest.mm
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/navigation/legacy_navigation_manager_impl.h
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/navigation/legacy_navigation_manager_impl.mm
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/navigation/navigation_manager_impl.h
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/navigation/navigation_manager_impl.mm
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/navigation/navigation_manager_impl_unittest.mm
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/navigation/wk_based_navigation_manager_impl.h
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/navigation/wk_based_navigation_manager_impl.mm
[add] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/web_state/js/resources/legacy_navigation.js
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/web_state/js/resources/navigation.js
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/web_state/js/resources/web_bundle.js
[modify] https://crrev.com/23a351aeb879eb30e02ca06afb721f9bfbb4df25/ios/web/web_state/ui/crw_web_controller.mm

 Issue 775044  has been merged into this issue.

 Issue 775598  has been merged into this issue.