[Notes to self] Here are the observed behaviors with #slim-navigation-manager enabled:

Case 1: "No Internet" (ERR_INTERNET_DISCONNECTED) is shown instead of offline view.
Case 2: "This site can't be reached" (ERR_FAILED) is shown instead of offline view.
Video:

The only thing I am able to reproduce for the first case is to load the online version but with an offline indicator. I think the issue for the first case is that the ReadingListWebStateObserver::LoadOfflineReadingListEntry() doesn't find a pendingItem https://cs.chromium.org/chromium/src/ios/chrome/browser/reading_list/reading_list_web_state_observer.mm?sq=package:chromium&dr=CSs&g=0&l=262 . I am not really sure to understand what is triggering this.

Also, as the native content is presented twice, the -restoreOnlineURL method of the OfflinePageNativeContent is called (when the first is destroyed). It might introduce an issue as the URL of the item will be restored to the real URL.

Also, there is an issue where the committed item has a URL of the form "about:blank?for=https%3A%2F%2Fwww.nytimes.com..." and not virtual URL but the url being loaded (from CRWWebController webView:didFinishNavigation:) has the correct chrome://offline URL and correct virtual URL.

For the second case, there is a DCHECK for user agent type NONE when reloading.

After investigating, on the second case there is a DCHECK issue when the pending item is created based on the last committed item as the last committed item is the offline item with a UserAgent::NONE. So it is also created with the same user agent (which creates a DCHECK). I think a possible fix would be to create the offline item with a user agent.
This is not DCHECKing with the old navigation manager as no pending item is created during reload. https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?sq=package:chromium&dr=C&g=0&l=1444 (See issue 676129).

The second issue, which is actually the cause of the error page is that when the navigation is over (in CRWWebController -webView:didFinishNavigation:), the webViewURL is the placeholder URL. So, when the ErrorRetryStateMachine is checked https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?sq=package:chromium&dr=C&g=0&l=4904 ; the URL which is loaded (the non-placeholder one) is checked against the placeholder, making the StateMachine return an error https://cs.chromium.org/chromium/src/ios/web/navigation/error_retry_state_machine.mm?dr=C&sq=package:chromium&g=0&l=101
I am not exactly sure about the correct fix for this one. I am not sure to understand which problem the state machine is supposed to handle.

For the reload I guess it has to do with your comment #4 on issue 840782, or did you already implemented that?

I think there's a race condition between -handleLoadError in CRWWebController and LoadOfflineReadingListEntry(). If the former finishes first, there will be no pending item.

Agreed that native content being presented twice is a bug. I'll fix that for crbug.com/865422.

The committed item has an about:blank?for=... URL is expected. It is loaded into WKWebView to create a history entry for the failed navigation: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?q=loadPlaceholderInWebViewForURL&l=1880
The fact you see chrome://offline URL in webView:didFinishNavigation: is most likely due to issue 840782. I haven't implemented the state machine idea mentioned in that bug because the error retry state machine is complicated, and I'm not eager to replicate it.

It seems to me that a root cause here (other than presenting native content twice) is that reading list code is initiating the offline reading list load by modifying URL of navigation item directly. This doesn't work with slim navigation manager. What would be a good alternative? Should CRWWebController trigger the loading of reading list items directly?

I don't know how you would want CRWWebController to trigger the loading of the reading list item directly, but I am afraid that it would increase its complexity quite a lot.

Do you think it is possible to let the navigation manager/CRWWebController knows that the URL of the item has changed and that it needs to reload the item?

Also, I am wondering if there isn't another problem as I am hitting quite a lot the situation where there is no pending item for the offline page.

I'll investigate the missing pending item issue.

About directly trigger the loading of reading list, my understanding is that reading list is a native content that covers the underlying web view, and we only want to trigger reading list item if the online load fails.

Right now when online load fails, three things happen:

1) WKWebView triggers CRWWebController's |-webView:didFailProvisionalNavigation| or |-webView:didFailNavigation| callbacks, depending on when the load fails. Both call |-handleLoadError|, which then loads the native content returned by CRWNativeContentProvider |-controllerForUnhandledContentAtURL:webState|: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?q=controllerForUnhandledContentAtURL&l=3054
This presents the typical error page.

2) Offline reading list kicks in after the error page is presented. It modifies the URL of the item to chrome://offline and reload.

3) With LegacyNavigationManager, navigation layer will correctly present the native content associated with this URL here: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?q=controllerForURL&l=1814

The bug here is WKBasedNavigationManager delegates reload to WKWebView directly, so the reload triggered in (2) reloads whatever is in the web view, which may be the placeholder item if the load fails in provisional navigation.

I wonder if it's possible to present the reading list native content in (1) instead of (3). But I can see that this introduces tight coupling between reading list UI code and navigation code that may not be desirable.

Maybe it is possible to let navigation manager / CRWWebController know that the URL of the item has changed. This would interact with the ErrorRetryStateMachine, which is a beast: https://docs.google.com/document/d/1gpmJnHnChm5xWeUhie8wW6Rq-0PHw0vU2myAJ0kIukA/edit#heading=h.wndz2y2yntcn

Is there a third option of presenting offline native content in step (2)? Navigation manager already has the responsibility of retrying a load error around back/forward navigation. Reading list just needs to make sure that if a navigation finishes in error, and if it has a distilled page, present the distilled page. Distilled page should be dismissed whenever a new navigation starts. This preserves current reload semantics as well.

The offline version of the page is presented when the load fails and when the load takes too much time to complete (i.e. at least 25% in 1.5s, 50% in 3s, 75% in 4.5s).

For the loading when the page takes too much time to load: https://cs.chromium.org/chromium/src/ios/chrome/browser/reading_list/reading_list_web_state_observer.mm?sq=package:chromium&dr=CSs&g=0&l=247

Looking at it, I am not sure sure it really have something to do with the pending items. It is actually because when there is a pending item, the URL is loaded directly, which prevent the problem. It means that the issue (have the online version when loading the offline page) happens only when the offline version triggers after some loading (the item has to be committed). If the offline version is trigger at the beginning (e.g. when you have no network), it is working as expected. 

The problem seems to be that the ReadingListWebStateObserver is changing the URL of the item and then asking the Navigation Manager to navigate to the item. I think that the WKNavigationManger is just asking the WKWebView to reload the item at the index X. However, at this point the items kept by the NavigationManager and the other from the WKWebView are out of sync. The WKWebView didn't change the URL of its item so when it is asked to navigate to it, it is reloading the online page.

I think the ReadingListWebStateObserver should ask the NavigationManager to change the url of the current navigation item. Then the LegacyNavigationManager would do what is currently done, the WKNavigationManager would change the item in the WKWebView stack (I think it is possible to either modify the WKBackForwardListItem, or remove it and add a new one instead?).
Same thing when restoring the item when the offline page is navigated away.

The behavior should be: you navigate to a page on slow network, the offline page is presented (the back/forward navigation stack is kept). If you navigate back/forward, the online version should be loaded again (so the item needs to be restored to its original state).

For the error page:

I think if we are able to solve the problem of the "load slowly", then we would have an interface for letting the NavigationManager change the URL of an item. Then reloading the page in 2) would reload the correct page (only if we are using navigateTo and not reload on the WKWebView). However I am not sure to really understand the state machine of the error pages or how it is working, so I don't know if it would solve the problem.

I don't think it is a good idea to introduce a strong coupling between the CRWebController and anything else.


The current implementation (of Offline pages) grew out of the bugs we found using the feature. If you think there is a better approach for the loading in general, feel free to suggest it.

If I have time today I will send you a CL describing my approach and you can tell me what you think of it.

> I think the ReadingListWebStateObserver should ask the NavigationManager to change the url of the current navigation item. Then the LegacyNavigationManager would do what is currently done, the WKNavigationManager would change the item in the WKWebView stack (I think it is possible to either modify the WKBackForwardListItem, or remove it and add a new one instead?).

This is the tricky bit. Once an entry is added to WKBackForwardList, there is no API to replace or modify it. ErrorRetryStateMachine uses a hack: navigate to the desired entry, call WKWebView |-loadHTMLString:baseURL| to modify the content of the current entry, and WKWebView |-reload| to get the real content of that URL. For reading list, we would set |baseURL| to the placeholder URL of chrome://offline/... and probably won't need |-reload|.

This hack has the issue that it sometimes changes the URL of the WKBackForwardListItem but not other times (WebKit seems to have additional states that manage this, which I don't yet fully understand). If user navigates away and later comes back to this item, WebKit may load either the original URL before our |-loadHTMLString:baseURL| call or the placeholder URL. The first case may be OK for reading list case, as WebView will simply try to load the online version, and trigger the offline version. The second case seems problematic if we want to reload the online version again. Then we'd have to use the |-loadHTMLString:baseURL| hack again, but to restore the online URL.

Perhaps I'm over thinking. I'd be interested in seeing your approach. Thanks for taking a stab!

I have created crrev.com/c/1148563. The idea would be to store the current Navigation stack, go back one item, replace the current navigation with the "offline" one (which would destroy the forward stack), then replay the forward stack using the same code as we do to restore a sessions (I guess the code will need to be different, but my guess was that it is possible as we are doing it for the session restore).