New issue
Advanced search Search tips

Issue 776486 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Task

Blocking:
issue 734150



Sign in to add a comment

HistoryStateOperationsTests fail with WKBasedNavigationManager

Project Member Reported by ajuma@chromium.org, Oct 19 2017

Issue description

ReplaceStateNoHashChangeEvent, ReplaceStatePostRequest, and StateReplacement all fail because they're expecting the NavigationItemImpl corresponding to the last committed item to change, but WKBasedNavigationManagerImpl::GetNavigationItemImplAtIndex returns an existing NavigationItemImpl as-is when one already exists for a particular wk_item. So any changes in the wk_item (like its url) aren't reflected in the returned NavigationItemImpl.
 
This is due to WKBasedNavigationManager flow not intercepting pushState and replaceState messages and not calling [CRWSessionController updateCurrentItemWithURL] in [CRWWebController replaceStateWithURL].

I need to add the push/replaceState interceptors back for  crbug.com/776373 . So the best short term solution is add updateCurrentItemWithURL functionality to the new navigation manager as well.
Owner: danyao@chromium.org
Status: Started (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 10 2017

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

Comment 4 by danyao@chromium.org, Nov 10 2017

Status: Fixed (was: Started)

Sign in to add a comment