[Nav Experiment] Enable NavigationManagerTest for WK-based nav manager.

This uncovered a bug where GetLastCommittedItemIndex() returned the
incorrect value because it assumes WKBackForwardList's |currentItem|
is always the last committed item. This is not the case if the pending
navigation is a back-forward navigation. It turned out that this can't
be fixed 100%, but implemented a heurstic and documented the edge case.

Bug: 734150
Change-Id: Ie2c026a2c5231cbd4151c106cd0db664dcf6d587
Reviewed-on: https://chromium-review.googlesource.com/596828Reviewed-by: Eugene But <eugenebut@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491749}

ios/web/navigation/wk_based_navigation_manager_impl.mm

@@ -105,8 +105,9 @@ void WKBasedNavigationManagerImpl::AddTransientItem(const GURL& url) {
 
   // Transient item is only supposed to be added for pending non-app-specific
   // navigations.
-  DCHECK(pending_item_->GetUserAgentType() != UserAgentType::NONE);
-  transient_item_->SetUserAgentType(pending_item_->GetUserAgentType());
+  NavigationItem* pending_item = GetPendingItem();
+  DCHECK(pending_item->GetUserAgentType() != UserAgentType::NONE);
+  transient_item_->SetUserAgentType(pending_item->GetUserAgentType());
 }
 
 void WKBasedNavigationManagerImpl::AddPendingItem(
@@ -176,12 +177,11 @@ void WKBasedNavigationManagerImpl::CommitPendingItem() {
   pending_item_index_ = -1;
   previous_item_index_ = last_committed_item_index_;
   last_committed_item_index_ = GetWKCurrentItemIndex();
-
   OnNavigationItemCommitted();
 }
 
 int WKBasedNavigationManagerImpl::GetIndexForOffset(int offset) const {
-  int result = (pending_item_index_ == -1) ? GetLastCommittedItemIndex()
+  int result = (pending_item_index_ == -1) ? GetWKCurrentItemIndex()
                                            : pending_item_index_;
 
   if (offset < 0 && GetTransientItem() && pending_item_index_ == -1) {
@@ -276,6 +276,15 @@ int WKBasedNavigationManagerImpl::GetPendingItemIndex() const {
 }
 
 int WKBasedNavigationManagerImpl::GetLastCommittedItemIndex() const {
+  // WKBackForwardList's |currentItem| is usually the last committed item,
+  // except when the pending navigation is a back-forward navigation, in which
+  // case it is actually the pending item. As a workaround, fall back to
+  // last_committed_item_index_. This is not 100% correct (since
+  // last_committed_item_index_ is only updated for main frame navigations),
+  // but is the best possible answer.
+  if (pending_item_index_ >= 0) {
+    return last_committed_item_index_;
+  }
   return GetWKCurrentItemIndex();
 }
 

ios/web/test/fakes/crw_test_back_forward_list.h

@@ -7,11 +7,16 @@
 
 #import <Foundation/Foundation.h>
 
+NS_ASSUME_NONNULL_BEGIN
+
 @class WKBackForwardListItem;
 
 // A CRWTestBackForwardList can be used to stub out WKBackForwardList in tests.
 @interface CRWTestBackForwardList : NSObject
 
+// Returns an OCMock of WKBackForwardListItem with the given URL.
++ (WKBackForwardListItem*)itemWithURLString:(NSString*)URL;
+
 // WKBackForwardList interface
 @property(nullable, nonatomic, copy) NSArray<WKBackForwardListItem*>* backList;
 @property(nullable, nonatomic, copy)
@@ -27,9 +32,11 @@
 // Resets this instance to simulate a session with the current entry at
 // |currentItemURL|, and back and forward history entries as specified in
 // |backListURLs| and |forwardListURLs|.
-- (void)setCurrentURL:(nonnull NSString*)currentItemURL
+- (void)setCurrentURL:(NSString*)currentItemURL
          backListURLs:(nullable NSArray<NSString*>*)backListURLs
       forwardListURLs:(nullable NSArray<NSString*>*)forwardListURLs;
 @end
 
+NS_ASSUME_NONNULL_END
+
 #endif  // IOS_WEB_NAVIGATION_CRW_TEST_BACK_FORWARD_LIST_H_

ios/web/test/fakes/crw_test_back_forward_list.mm

@@ -13,7 +13,6 @@
 #endif
 
 @interface CRWTestBackForwardList (PrivateMethods)
-- (WKBackForwardListItem*)mockItemWithURLString:(NSString*)URL;
 - (NSArray*)mockSublistWithURLArray:(NSArray<NSString*>*)URLs;
 @end
 
@@ -23,6 +22,12 @@
 @synthesize forwardList;
 @synthesize currentItem;
 
++ (WKBackForwardListItem*)itemWithURLString:(NSString*)URL {
+  id mock = OCMClassMock([WKBackForwardListItem class]);
+  OCMStub([mock URL]).andReturn([NSURL URLWithString:URL]);
+  return mock;
+}
+
 - (WKBackForwardListItem*)itemAtIndex:(NSInteger)index {
   if (index == 0) {
     return self.currentItem;
@@ -41,21 +46,15 @@
 - (void)setCurrentURL:(NSString*)currentItemURL
          backListURLs:(nullable NSArray<NSString*>*)backListURLs
       forwardListURLs:(nullable NSArray<NSString*>*)forwardListURLs {
-  self.currentItem = [self mockItemWithURLString:currentItemURL];
+  self.currentItem = [CRWTestBackForwardList itemWithURLString:currentItemURL];
   self.backList = [self mockSublistWithURLArray:backListURLs];
   self.forwardList = [self mockSublistWithURLArray:forwardListURLs];
 }
 
-- (WKBackForwardListItem*)mockItemWithURLString:(NSString*)URL {
-  id mock = OCMClassMock([WKBackForwardListItem class]);
-  OCMStub([mock URL]).andReturn([NSURL URLWithString:URL]);
-  return mock;
-}
-
 - (NSArray*)mockSublistWithURLArray:(NSArray<NSString*>*)URLs {
   NSMutableArray* array = [NSMutableArray arrayWithCapacity:URLs.count];
   for (NSString* URL : URLs) {
-    [array addObject:[self mockItemWithURLString:URL]];
+    [array addObject:[CRWTestBackForwardList itemWithURLString:URL]];
   }
   return [NSArray arrayWithArray:array];
 }