Remove CRWSessionEntry.

ios/web/navigation/crw_session_controller.mm

Index: ios/web/navigation/crw_session_controller.mm
diff --git a/ios/web/navigation/crw_session_controller.mm b/ios/web/navigation/crw_session_controller.mm
index 351320bf09e8689f35c698220d28a6fbfb0f98cc..71cddf776b6d396b4015d5f802ae9cd3e3b17d1e 100644
--- a/ios/web/navigation/crw_session_controller.mm
+++ b/ios/web/navigation/crw_session_controller.mm
@@ -8,7 +8,6 @@
 
 #include <algorithm>
 #include <utility>
-#include <vector>
 
 #include "base/format_macros.h"
 #include "base/logging.h"
@@ -18,7 +17,6 @@
 #include "ios/web/history_state_util.h"
 #import "ios/web/navigation/crw_session_certificate_policy_manager.h"
 #import "ios/web/navigation/crw_session_controller+private_constructors.h"
-#import "ios/web/navigation/crw_session_entry.h"
 #import "ios/web/navigation/navigation_item_impl.h"
 #include "ios/web/navigation/navigation_manager_facade_delegate.h"
 #import "ios/web/navigation/navigation_manager_impl.h"
@@ -38,27 +36,12 @@ @interface CRWSessionController () {
   // the incremental merging of the two classes.
   web::NavigationManagerImpl* _navigationManager;
 
-  // Identifies the index of the current navigation in the CRWSessionEntry
+  // Identifies the index of the current navigation in the NavigationItem
   // array.
   NSInteger _currentNavigationIndex;
-  // Identifies the index of the previous navigation in the CRWSessionEntry
+  // Identifies the index of the previous navigation in the NavigationItem
   // array.
   NSInteger _previousNavigationIndex;
-  // Ordered array of |CRWSessionEntry| objects, one for each site in session
-  // history. End of the list is the most recent load.
-  NSMutableArray* _entries;
-
-  // An entry we haven't gotten a response for yet. This will be discarded
-  // when we navigate again. It's used only so we know what the currently
-  // displayed tab is.  It backs the property of the same name and should only
-  // be set through its setter.
-  base::scoped_nsobject<CRWSessionEntry> _pendingEntry;
-
-  // The transient entry, if any. A transient entry is discarded on any
-  // navigation, and is used for representing interstitials that need to be
-  // represented in the session.  It backs the property of the same name and
-  // should only be set through its setter.
-  base::scoped_nsobject<CRWSessionEntry> _transientEntry;
 
    // Stores the certificate policies decided by the user.
   CRWSessionCertificatePolicyManager* _sessionCertificatePolicyManager;
@@ -66,9 +49,18 @@ @interface CRWSessionController () {
   // The browser state associated with this CRWSessionController;
   web::BrowserState* _browserState;  // weak
 
-  // Time smoother for navigation entry timestamps; see comment in
+  // Time smoother for navigation item timestamps; see comment in
   // navigation_controller_impl.h
   web::TimeSmoother _timeSmoother;
+
+  // Backing objects for properties of the same name.
+  web::ScopedNavigationItemImplList _items;
+  // |_pendingItem| only contains a NavigationItem for non-history navigations.
+  // For back/forward navigations within session history, _pendingItemIndex will
+  // be equal to -1, and self.pendingItem will return an item contained within
+  // |_items|.
+  std::unique_ptr<web::NavigationItemImpl> _pendingItem;
+  std::unique_ptr<web::NavigationItemImpl> _transientItem;

[Eugene But (OOO till 7-30), 2017/03/09 16:10:58]

Old comments for transientEntry looked useful. Is there a reason for removing
them?

[kkhorimoto, 2017/03/09 21:33:54]

I've moved the comment to the interface property declaration.  Since the ivars
are an implementation detail, I prefer to use comments in the interface.

 }
 
 // Redefine as readwrite.
@@ -76,7 +68,6 @@ @interface CRWSessionController () {
 
 // TODO(rohitrao): These properties must be redefined readwrite to work around a
 // clang bug. crbug.com/228650
-@property(nonatomic, readwrite, strong) NSArray* entries;
 @property(nonatomic, readwrite, strong)
     CRWSessionCertificatePolicyManager* sessionCertificatePolicyManager;
 
@@ -86,21 +77,20 @@ @interface CRWSessionController () {
 @property(nonatomic, readwrite, getter=isOpenedByDOM) BOOL openedByDOM;
 @property(nonatomic, readwrite, assign) NSInteger previousNavigationIndex;
 
-// Removes all entries after currentNavigationIndex_.
+// Removes all items after currentNavigationIndex_.
 - (void)clearForwardItems;
-// Discards the transient entry, if any.
+// Discards the transient item, if any.
 - (void)discardTransientItem;
-// Create a new autoreleased session entry.
-- (CRWSessionEntry*)sessionEntryWithURL:(const GURL&)url
-                               referrer:(const web::Referrer&)referrer
-                             transition:(ui::PageTransition)transition
-                         initiationType:
-                             (web::NavigationInitiationType)initiationType;
+// Creates a NavigationItemImpl with the specified properties.
+- (std::unique_ptr<web::NavigationItemImpl>)
+   itemWithURL:(const GURL&)url
+      referrer:(const web::Referrer&)referrer
+    transition:(ui::PageTransition)transition
+initiationType:(web::NavigationInitiationType)initiationType;
 // Returns YES if the PageTransition for the underlying navigationItem at
-// |index| in |entries_| has ui::PAGE_TRANSITION_IS_REDIRECT_MASK.
-- (BOOL)isRedirectTransitionForItemAtIndex:(NSInteger)index;
-// Returns a NavigationItemList containing the NavigationItems from |entries|.
-- (web::NavigationItemList)itemListForEntryList:(NSArray*)entries;
+// |index| in |items| has ui::PAGE_TRANSITION_IS_REDIRECT_MASK.
+- (BOOL)isRedirectTransitionForItemAtIndex:(size_t)index;
+
 @end
 
 @implementation CRWSessionController
@@ -108,7 +98,6 @@ @implementation CRWSessionController
 @synthesize currentNavigationIndex = _currentNavigationIndex;
 @synthesize previousNavigationIndex = _previousNavigationIndex;
 @synthesize pendingItemIndex = _pendingItemIndex;
-@synthesize entries = _entries;
 @synthesize openedByDOM = _openedByDOM;
 @synthesize sessionCertificatePolicyManager = _sessionCertificatePolicyManager;
 
@@ -118,7 +107,6 @@ - (instancetype)initWithBrowserState:(web::BrowserState*)browserState
   if (self) {
     _openedByDOM = openedByDOM;
     _browserState = browserState;
-    _entries = [NSMutableArray array];
     _currentNavigationIndex = -1;
     _previousNavigationIndex = -1;
     _pendingItemIndex = -1;
@@ -134,21 +122,12 @@ - (instancetype)initWithBrowserState:(web::BrowserState*)browserState
   self = [super init];
   if (self) {
     _browserState = browserState;
-
-    // Create entries array from list of navigations.
-    _entries = [[NSMutableArray alloc] initWithCapacity:items.size()];
-    for (auto& item : items) {
-      base::scoped_nsobject<CRWSessionEntry> entry(
-          [[CRWSessionEntry alloc] initWithNavigationItem:std::move(item)]);
-      [_entries addObject:entry];
-    }
+    _items = web::CreateScopedNavigationItemImplList(std::move(items));
     self.currentNavigationIndex = currentIndex;
-    // Prior to M34, 0 was used as "no index" instead of -1; adjust for that.

[Eugene But (OOO till 7-30), 2017/03/09 16:10:58]

This comment still looks like a valid one, because the code adjusts for 0. Maybe
remove both code and adjusting code?

[kkhorimoto, 2017/03/09 21:33:54]

I removed the empty() check.  The same result is accomplished in the min()
below.

-    if (![_entries count])
+    if (_items.empty())
       self.currentNavigationIndex = -1;
-    if (_currentNavigationIndex >= static_cast<NSInteger>(items.size())) {
-      self.currentNavigationIndex = static_cast<NSInteger>(items.size()) - 1;
-    }
+    _currentNavigationIndex = std::min(
+        _currentNavigationIndex, static_cast<NSInteger>(_items.size() - 1));
     _previousNavigationIndex = -1;
     _pendingItemIndex = -1;
     _sessionCertificatePolicyManager =
@@ -157,18 +136,7 @@ - (instancetype)initWithBrowserState:(web::BrowserState*)browserState
   return self;
 }
 
-- (id)copyWithZone:(NSZone*)zone {
-  CRWSessionController* copy = [[[self class] alloc] init];
-  copy->_openedByDOM = _openedByDOM;
-  copy->_currentNavigationIndex = _currentNavigationIndex;
-  copy->_previousNavigationIndex = _previousNavigationIndex;
-  copy->_pendingItemIndex = _pendingItemIndex;
-  copy->_entries =
-      [[NSMutableArray alloc] initWithArray:_entries copyItems:YES];
-  copy->_sessionCertificatePolicyManager =
-      [_sessionCertificatePolicyManager copy];
-  return copy;
-}
+#pragma mark - Accessors
 
 - (void)setCurrentNavigationIndex:(NSInteger)currentNavigationIndex {
   if (_currentNavigationIndex != currentNavigationIndex) {
@@ -178,139 +146,170 @@ - (void)setCurrentNavigationIndex:(NSInteger)currentNavigationIndex {
   }
 }
 
-- (void)setPendingItemIndex:(NSInteger)index {
-  DCHECK_GE(index, -1);
-  DCHECK_LT(index, static_cast<NSInteger>(_entries.count));
-  _pendingItemIndex = index;
-  CRWSessionEntry* entry = index != -1 ? _entries[index] : nil;
-  _pendingEntry.reset(entry);
-  DCHECK(_pendingItemIndex == -1 || _pendingEntry);
+- (void)setPendingItemIndex:(NSInteger)pendingItemIndex {
+  DCHECK_GE(pendingItemIndex, -1);
+  DCHECK_LT(pendingItemIndex, static_cast<NSInteger>(self.items.size()));
+  _pendingItemIndex = pendingItemIndex;
+  DCHECK(_pendingItemIndex == -1 || self.pendingItem);
 }
 
-- (void)setNavigationManager:(web::NavigationManagerImpl*)navigationManager {
-  _navigationManager = navigationManager;
-  if (_navigationManager) {
-    // _browserState will be nullptr if CRWSessionController has been
-    // initialized with -initWithCoder: method. Take _browserState from
-    // NavigationManagerImpl if that's the case.
-    if (!_browserState) {
-      _browserState = _navigationManager->GetBrowserState();
-    }
-    DCHECK_EQ(_browserState, _navigationManager->GetBrowserState());
-  }
-}
-
-- (void)setBrowserState:(web::BrowserState*)browserState {
-  _browserState = browserState;
-  DCHECK(!_navigationManager ||
-         _navigationManager->GetBrowserState() == _browserState);
-}
-
-- (NSString*)description {
-  return [NSString stringWithFormat:@"current index: %" PRIdNS
-                                    @"\nprevious index: %" PRIdNS
-                                    @"\npending index: %" PRIdNS
-                                    @"\n%@\npending: %@\ntransient: %@\n",
-                                    _currentNavigationIndex,
-                                    _previousNavigationIndex, _pendingItemIndex,
-                                    _entries, _pendingEntry.get(),
-                                    _transientEntry.get()];
-}
-
-- (web::NavigationItemList)items {
-  return [self itemListForEntryList:self.entries];
-}
-
-- (NSUInteger)itemCount {
-  return self.entries.count;
+- (const web::ScopedNavigationItemImplList&)items {
+  return _items;
 }
 
 - (web::NavigationItemImpl*)currentItem {
-  return self.currentEntry.navigationItemImpl;
+  if (self.transientItem)
+    return self.transientItem;
+  if (self.pendingItem)
+    return self.pendingItem;
+  return self.lastCommittedItem;
 }
 
 - (web::NavigationItemImpl*)visibleItem {
-  return self.visibleEntry.navigationItemImpl;
+  // Only return the |pendingItem| for new (non-history), browser-initiated
+  // navigations in order to prevent URL spoof attacks.
+  web::NavigationItemImpl* pendingItem = self.pendingItem;
+  if (pendingItem) {
+    bool isUserInitiated = pendingItem->NavigationInitiationType() ==
+                           web::NavigationInitiationType::USER_INITIATED;
+    bool safeToShowPending = isUserInitiated && _pendingItemIndex == -1;
+
+    if (safeToShowPending)
+      return pendingItem;
+  }
+  return self.lastCommittedItem;
 }
 
 - (web::NavigationItemImpl*)pendingItem {
-  return self.pendingEntry.navigationItemImpl;
+  if (self.pendingItemIndex == -1)
+    return _pendingItem.get();
+  return self.items[self.pendingItemIndex].get();
 }
 
 - (web::NavigationItemImpl*)transientItem {
-  return self.transientEntry.navigationItemImpl;
+  return _transientItem.get();
 }
 
 - (web::NavigationItemImpl*)lastCommittedItem {
-  return self.lastCommittedEntry.navigationItemImpl;
+  NSInteger index = self.currentNavigationIndex;
+  return index == -1 ? nullptr : self.items[index].get();
 }
 
 - (web::NavigationItemImpl*)previousItem {
-  return self.previousEntry.navigationItemImpl;
+  NSInteger index = self.previousNavigationIndex;
+  return index == -1 || self.items.empty() ? nullptr : self.items[index].get();
 }
 
 - (web::NavigationItemImpl*)lastUserItem {
-  return self.lastUserEntry.navigationItemImpl;
+  if (self.items.empty())
+    return nil;
+
+  NSInteger index = self.currentNavigationIndex;
+  // This will return the first NavigationItem if all other items are
+  // redirects, regardless of the transition state of the first item.
+  while (index > 0 && [self isRedirectTransitionForItemAtIndex:index])
+    --index;
+
+  return self.items[index].get();
 }
 
 - (web::NavigationItemList)backwardItems {
-  return [self itemListForEntryList:self.backwardEntries];
+  web::NavigationItemList items;
+  for (size_t index = _currentNavigationIndex; index > 0; --index) {
+    if (![self isRedirectTransitionForItemAtIndex:index])
+      items.push_back(self.items[index - 1].get());
+  }
+  return items;
 }
 
 - (web::NavigationItemList)forwardItems {
-  return [self itemListForEntryList:self.forwardEntries];
-}
-
-// Returns the current entry in the session list, or the pending entry if there
-// is a navigation in progress.
-- (CRWSessionEntry*)currentEntry {
-  if (_transientEntry)
-    return _transientEntry.get();
-  if (_pendingEntry)
-    return _pendingEntry.get();
-  return [self lastCommittedEntry];
+  web::NavigationItemList items;
+  NSUInteger lastNonRedirectedIndex = _currentNavigationIndex + 1;
+  while (lastNonRedirectedIndex < self.items.size()) {
+    web::NavigationItem* item = self.items[lastNonRedirectedIndex].get();
+    if (!ui::PageTransitionIsRedirect(item->GetTransitionType()))
+      items.push_back(item);
+    ++lastNonRedirectedIndex;
+  }
+  return items;
 }
 
-// See NavigationController::GetVisibleEntry for the motivation for this
-// distinction.
-- (CRWSessionEntry*)visibleEntry {
-  if (_transientEntry)
-    return _transientEntry.get();
-  // Only return the pending_entry for new (non-history), browser-initiated
-  // navigations in order to prevent URL spoof attacks.
-  web::NavigationItemImpl* pendingItem = [_pendingEntry navigationItemImpl];
-
-  if (pendingItem) {
-    bool isUserInitiated = pendingItem->NavigationInitiationType() ==
-                           web::NavigationInitiationType::USER_INITIATED;
-    bool safeToShowPending = isUserInitiated && _pendingItemIndex == -1;
+#pragma mark - NSObject
 
-    if (safeToShowPending)
-      return _pendingEntry.get();
+- (NSString*)description {
+  // Create description for |items|.
+  NSMutableString* itemsDescription = [NSMutableString stringWithString:@"[\n"];
+#ifndef NDEBUG
+  for (const auto& item : self.items)
+    [itemsDescription appendFormat:@"%@\n", item->GetDescription()];
+#endif
+  [itemsDescription appendString:@"]"];
+  // Create description for |pendingItem| and |transientItem|.
+  NSString* pendingItemDescription = @"(null)";
+  NSString* transientItemDescription = @"(null)";
+#ifndef NDEBUG
+  if (self.pendingItem)
+    pendingItemDescription = self.pendingItem->GetDescription();
+  if (self.transientItem)
+    transientItemDescription = self.transientItem->GetDescription();
+#else
+  if (self.pendingItem) {
+    pendingItemDescription =
+        [NSString stringWithFormat:@"%p", self.pendingItem];
   }
-
-  return [self lastCommittedEntry];
+  if (self.transientItem) {
+    transientItemDescription =
+        [NSString stringWithFormat:@"%p", self.transientItem];
+  }
+#endif
+  return [NSString stringWithFormat:@"current index: %" PRIdNS
+                                    @"\nprevious index: %" PRIdNS
+                                    @"\npending"
+                                    @" index: %" PRIdNS
+                                    @"\n%@\npending: %@\ntransient: %@\n",
+                                    _currentNavigationIndex,
+                                    _previousNavigationIndex, _pendingItemIndex,
+                                    itemsDescription, pendingItemDescription,
+                                    transientItemDescription];
 }
 
-- (CRWSessionEntry*)pendingEntry {
-  return _pendingEntry.get();
-}
+#pragma mark - NSCopying
 
-- (CRWSessionEntry*)transientEntry {
-  return _transientEntry.get();
+- (id)copyWithZone:(NSZone*)zone {
+  CRWSessionController* copy = [[[self class] alloc] init];
+  copy->_openedByDOM = _openedByDOM;
+  copy->_currentNavigationIndex = _currentNavigationIndex;
+  copy->_previousNavigationIndex = _previousNavigationIndex;
+  copy->_pendingItemIndex = _pendingItemIndex;
+  copy->_sessionCertificatePolicyManager =
+      [_sessionCertificatePolicyManager copy];
+  for (size_t index = 0; index < self.items.size(); ++index) {
+    std::unique_ptr<web::NavigationItemImpl> itemCopy(
+        new web::NavigationItemImpl(*self.items[index].get()));

[Eugene But (OOO till 7-30), 2017/03/09 16:10:58]

nit: Do you need |.get()|?

[Eugene But (OOO till 7-30), 2017/03/09 16:10:58]

nit: s/new/base::MakeUnique

[kkhorimoto, 2017/03/09 21:33:54]

Nope, turns out unique_ptr overrides the * operator.  Updated to MakeUnique as
well.

+    copy->_items.push_back(std::move(itemCopy));
+  }
+  return copy;
 }
 
-- (CRWSessionEntry*)lastCommittedEntry {
-  if (_currentNavigationIndex == -1)
-    return nil;
-  return [_entries objectAtIndex:_currentNavigationIndex];
+#pragma mark - Public
+
+- (void)setNavigationManager:(web::NavigationManagerImpl*)navigationManager {
+  _navigationManager = navigationManager;
+  if (_navigationManager) {
+    // _browserState will be nullptr if CRWSessionController has been
+    // initialized with -initWithCoder: method. Take _browserState from
+    // NavigationManagerImpl if that's the case.
+    if (!_browserState) {
+      _browserState = _navigationManager->GetBrowserState();
+    }
+    DCHECK_EQ(_browserState, _navigationManager->GetBrowserState());
+  }
 }
 
-// Returns the previous entry in the session list, or nil if there isn't any.
-- (CRWSessionEntry*)previousEntry {
-  if ((_previousNavigationIndex < 0) || (![_entries count]))
-    return nil;
-  return [_entries objectAtIndex:_previousNavigationIndex];
+- (void)setBrowserState:(web::BrowserState*)browserState {
+  _browserState = browserState;
+  DCHECK(!_navigationManager ||
+         _navigationManager->GetBrowserState() == _browserState);
 }
 
 - (void)addPendingItem:(const GURL&)url
@@ -318,9 +317,9 @@ - (void)addPendingItem:(const GURL&)url
             transition:(ui::PageTransition)trans
         initiationType:(web::NavigationInitiationType)initiationType {
   [self discardTransientItem];
-  _pendingItemIndex = -1;
+  self.pendingItemIndex = -1;
 
-  // Don't create a new entry if it's already the same as the current entry,
+  // Don't create a new item if it's already the same as the current item,
   // allowing this routine to be called multiple times in a row without issue.
   // Note: CRWSessionController currently has the responsibility to distinguish
   // between new navigations and history stack navigation, hence the inclusion
@@ -329,15 +328,13 @@ - (void)addPendingItem:(const GURL&)url
   // TODO(crbug.com/676129): Fix the way changes are detected/reported elsewhere
   // in the web layer so that this hack can be removed.
   // Remove the workaround code from -presentSafeBrowsingWarningForResource:.
-  CRWSessionEntry* currentEntry = self.currentEntry;
-  if (currentEntry) {
-    web::NavigationItem* item = [currentEntry navigationItem];
-
-    BOOL hasSameURL = item->GetURL() == url;
+  web::NavigationItemImpl* currentItem = self.currentItem;
+  if (currentItem) {
+    BOOL hasSameURL = currentItem->GetURL() == url;
     BOOL isPendingTransitionFormSubmit =
         PageTransitionCoreTypeIs(trans, ui::PAGE_TRANSITION_FORM_SUBMIT);
     BOOL isCurrentTransitionFormSubmit = PageTransitionCoreTypeIs(
-        item->GetTransitionType(), ui::PAGE_TRANSITION_FORM_SUBMIT);
+        currentItem->GetTransitionType(), ui::PAGE_TRANSITION_FORM_SUBMIT);
     BOOL shouldCreatePendingItem =
         !hasSameURL ||
         (isPendingTransitionFormSubmit && !isCurrentTransitionFormSubmit);
@@ -347,34 +344,32 @@ - (void)addPendingItem:(const GURL&)url
       // whether anything currently relies on this, but since both this whole
       // hack and the content facade will both be going away, it's not worth
       // trying to unwind.
-      if (_navigationManager && _navigationManager->GetFacadeDelegate()) {
+      if (_navigationManager && _navigationManager->GetFacadeDelegate())
         _navigationManager->GetFacadeDelegate()->OnNavigationItemPending();
-      }
       return;
     }
   }
 
-  _pendingEntry.reset([self sessionEntryWithURL:url
-                                       referrer:ref
-                                     transition:trans
-                                 initiationType:initiationType]);
+  _pendingItem = [self itemWithURL:url
+                          referrer:ref
+                        transition:trans
+                    initiationType:initiationType];
 
-  if (_navigationManager && _navigationManager->GetFacadeDelegate()) {
+  if (_navigationManager && _navigationManager->GetFacadeDelegate())
     _navigationManager->GetFacadeDelegate()->OnNavigationItemPending();
-  }
 }
 
 - (void)updatePendingItem:(const GURL&)url {
-  // If there is no pending entry, navigation is probably happening within the
-  // session history. Don't modify the entry list.
-  if (!_pendingEntry)
+  // If there is no pending item, navigation is probably happening within the
+  // session history. Don't modify the item list.
+  web::NavigationItemImpl* item = self.pendingItem;
+  if (!item)
     return;
 
-  web::NavigationItemImpl* item = [_pendingEntry navigationItemImpl];
   if (url != item->GetURL()) {
-    // Assume a redirection, and discard any transient entry.
+    // Assume a redirection, and discard any transient item.
     // TODO(stuartmorgan): Once the current safe browsing code is gone,
-    // consider making this a DCHECK that there's no transient entry.
+    // consider making this a DCHECK that there's no transient item.
     [self discardTransientItem];
 
     item->SetURL(url);
@@ -387,97 +382,87 @@ - (void)updatePendingItem:(const GURL&)url {
 
   // This should probably not be sent if the URLs matched, but that's what was
   // done before, so preserve behavior in case something relies on it.
-  if (_navigationManager && _navigationManager->GetFacadeDelegate()) {
+  if (_navigationManager && _navigationManager->GetFacadeDelegate())
     _navigationManager->GetFacadeDelegate()->OnNavigationItemPending();
-  }
 }
 
 - (void)clearForwardItems {
-  DCHECK_EQ(_pendingItemIndex, -1);
+  DCHECK_EQ(self.pendingItemIndex, -1);
   [self discardTransientItem];
 
   NSInteger forwardItemStartIndex = _currentNavigationIndex + 1;
   DCHECK(forwardItemStartIndex >= 0);
 
-  if (forwardItemStartIndex >= static_cast<NSInteger>([_entries count]))
+  size_t itemCount = self.items.size();
+  if (forwardItemStartIndex >= static_cast<NSInteger>(itemCount))
     return;
 
-  NSRange remove = NSMakeRange(forwardItemStartIndex,
-                               [_entries count] - forwardItemStartIndex);
-  // Store removed items in temporary NSArray so they can be deallocated after
-  // their facades.
-  base::scoped_nsobject<NSArray> removedItems(
-      [_entries subarrayWithRange:remove]);
-  [_entries removeObjectsInRange:remove];
   if (_previousNavigationIndex >= forwardItemStartIndex)
     _previousNavigationIndex = -1;
   if (_navigationManager) {
-    _navigationManager->OnNavigationItemsPruned(remove.length);
+    _navigationManager->OnNavigationItemsPruned(self.items.size() -

[Eugene But (OOO till 7-30), 2017/03/09 16:10:58]

Calling OnNavigationItemsPruned before removing items does not feel right.

[kkhorimoto, 2017/03/09 21:33:54]

Fixed

+                                                forwardItemStartIndex);
   }
+
+  // Remove the NavigationItems.
+  _items.erase(_items.begin() + forwardItemStartIndex, _items.end());
 }
 
 - (void)commitPendingItem {
-  if (_pendingEntry) {
-    NSInteger newNavigationIndex = _pendingItemIndex;
-    if (_pendingItemIndex == -1) {
+  if (self.pendingItem) {
+    // Once an item is committed it's not renderer-initiated any more. (Matches
+    // the implementation in NavigationController.)
+    self.pendingItem->ResetForCommit();
+
+    NSInteger newNavigationIndex = self.pendingItemIndex;
+    if (newNavigationIndex == -1) {
       [self clearForwardItems];
-      // Add the new entry at the end.
-      [_entries addObject:_pendingEntry];
-      newNavigationIndex = [_entries count] - 1;
+      // Add the new item at the end.
+      _items.push_back(std::move(_pendingItem));
+      newNavigationIndex = self.items.size() - 1;
     }
     _previousNavigationIndex = _currentNavigationIndex;
     self.currentNavigationIndex = newNavigationIndex;
-    // Once an entry is committed it's not renderer-initiated any more. (Matches
-    // the implementation in NavigationController.)
-    [_pendingEntry navigationItemImpl]->ResetForCommit();
-    _pendingEntry.reset();

[Eugene But (OOO till 7-30), 2017/03/09 16:10:58]

Do we need to clear the reference to pendingItem?

[kkhorimoto, 2017/03/09 21:33:54]

Nope, _pendingItem only holds a pointer for non-history navigations, which would
enter the condition above.  The std::move() will replace |_pendingItem| with a
nullptr.  I've added a DCHECK here that the ivar is indeed null at this point
though.

-    _pendingItemIndex = -1;
+    self.pendingItemIndex = -1;
   }
 
-  CRWSessionEntry* currentEntry = self.currentEntry;
-  web::NavigationItem* item = currentEntry.navigationItem;
+  web::NavigationItem* item = self.currentItem;
   // Update the navigation timestamp now that it's actually happened.
   if (item)
     item->SetTimestamp(_timeSmoother.GetSmoothedTime(base::Time::Now()));
 
   if (_navigationManager && item)
     _navigationManager->OnNavigationItemCommitted();
-  DCHECK_EQ(_pendingItemIndex, -1);
+  DCHECK_EQ(self.pendingItemIndex, -1);
 }
 
 - (void)addTransientItemWithURL:(const GURL&)URL {
-  _transientEntry.reset([self
-      sessionEntryWithURL:URL
-                 referrer:web::Referrer()
-               transition:ui::PAGE_TRANSITION_CLIENT_REDIRECT
-           initiationType:web::NavigationInitiationType::USER_INITIATED]);
-
-  web::NavigationItem* navigationItem = [_transientEntry navigationItem];
-  DCHECK(navigationItem);
-  navigationItem->SetTimestamp(
+  _transientItem =
+      [self itemWithURL:URL
+                referrer:web::Referrer()
+              transition:ui::PAGE_TRANSITION_CLIENT_REDIRECT
+          initiationType:web::NavigationInitiationType::USER_INITIATED];
+  _transientItem->SetTimestamp(
       _timeSmoother.GetSmoothedTime(base::Time::Now()));
 }
 
 - (void)pushNewItemWithURL:(const GURL&)URL
                stateObject:(NSString*)stateObject
                 transition:(ui::PageTransition)transition {
-  DCHECK(![self pendingEntry]);
-  DCHECK([self currentEntry]);
+  DCHECK(!self.pendingItem);
+  DCHECK(self.currentItem);
 
-  web::NavigationItem* lastCommittedItem =
-      self.lastCommittedEntry.navigationItem;
+  web::NavigationItem* lastCommittedItem = self.lastCommittedItem;
   CHECK(web::history_state_util::IsHistoryStateChangeValid(
       lastCommittedItem->GetURL(), URL));
 
   web::Referrer referrer(lastCommittedItem->GetURL(),
                          web::ReferrerPolicyDefault);
-  base::scoped_nsobject<CRWSessionEntry> pushedEntry([self
-      sessionEntryWithURL:URL
-                 referrer:referrer
-               transition:transition
-           initiationType:web::NavigationInitiationType::USER_INITIATED]);
-
-  web::NavigationItemImpl* pushedItem = [pushedEntry navigationItemImpl];
+  std::unique_ptr<web::NavigationItemImpl> pushedItem =
+      [self itemWithURL:URL
+                referrer:referrer
+              transition:transition
+          initiationType:web::NavigationInitiationType::USER_INITIATED];
   pushedItem->SetUserAgentType(lastCommittedItem->GetUserAgentType());
   pushedItem->SetSerializedStateObject(stateObject);
   pushedItem->SetIsCreatedFromPushState(true);
@@ -485,10 +470,10 @@ - (void)pushNewItemWithURL:(const GURL&)URL
   pushedItem->SetTimestamp(_timeSmoother.GetSmoothedTime(base::Time::Now()));
 
   [self clearForwardItems];
-  // Add the new entry at the end.
-  [_entries addObject:pushedEntry];
+  // Add the new item at the end.
+  _items.push_back(std::move(pushedItem));
   _previousNavigationIndex = _currentNavigationIndex;
-  self.currentNavigationIndex = [_entries count] - 1;
+  self.currentNavigationIndex = self.items.size() - 1;
 
   if (_navigationManager)
     _navigationManager->OnNavigationItemCommitted();
@@ -496,62 +481,60 @@ - (void)pushNewItemWithURL:(const GURL&)URL
 
 - (void)updateCurrentItemWithURL:(const GURL&)url
                      stateObject:(NSString*)stateObject {
-  DCHECK(!_transientEntry);
-  CRWSessionEntry* currentEntry = self.currentEntry;
-  web::NavigationItemImpl* currentItem = self.currentEntry.navigationItemImpl;
+  DCHECK(!self.transientItem);
+  web::NavigationItemImpl* currentItem = self.currentItem;
   currentItem->SetURL(url);
   currentItem->SetSerializedStateObject(stateObject);
   currentItem->SetHasStateBeenReplaced(true);
   currentItem->SetPostData(nil);
-  currentEntry.navigationItem->SetURL(url);
-  // If the change is to a committed entry, notify interested parties.
-  if (currentEntry != self.pendingEntry && _navigationManager)
+  // If the change is to a committed item, notify interested parties.
+  if (currentItem != self.pendingItem && _navigationManager)
     _navigationManager->OnNavigationItemChanged();
 }
 
 - (void)discardNonCommittedItems {
   [self discardTransientItem];
-  _pendingEntry.reset();
-  _pendingItemIndex = -1;
+  _pendingItem.reset();
+  self.pendingItemIndex = -1;
 }
 
 - (void)discardTransientItem {
-  // Keep the entry alive temporarily. There are flows that get the current
-  // entry, do some navigation operation, and then try to use that old current
-  // entry; since navigations clear the transient entry, these flows might
-  // crash. (This should be removable once more session management is handled
-  // within this class and/or NavigationManager).
-  _transientEntry.reset();
+  _transientItem.reset();
 }
 
 - (void)insertStateFromSessionController:(CRWSessionController*)sourceSession {
   DCHECK(sourceSession);
-  // The other session may not have any entries, in which case there is nothing
-  // to insert.  The other session's currentNavigationEntry will be bogus
-  // in such cases, so ignore it and return early.
-  NSArray* sourceEntries = sourceSession.entries;
-  if (!sourceEntries.count)
+
+  // The other session may not have any items, in which case there is nothing
+  // to insert.  The other session's currentItem will be bogus in such cases, so
+  // ignore it and return early.
+  web::ScopedNavigationItemImplList& sourceItems = sourceSession->_items;
+  if (sourceItems.empty())
     return;
 
-  // Cycle through the entries from the other session and insert them before any
-  // entries from this session.  Do not copy anything that comes after the other
-  // session's current entry.
+  // Cycle through the items from the other session and insert them before any
+  // items from this session.  Do not copy anything that comes after the other
+  // session's current item.
   NSInteger lastIndexToCopy = sourceSession.currentNavigationIndex;
   for (NSInteger i = 0; i <= lastIndexToCopy; ++i) {
-    [_entries insertObject:sourceEntries[i] atIndex:i];
+    std::unique_ptr<web::NavigationItemImpl> sourceItemCopy(
+        new web::NavigationItemImpl(*sourceItems[i].get()));

[Eugene But (OOO till 7-30), 2017/03/09 16:10:58]

nit: s/new/base::MakeUnique

[kkhorimoto, 2017/03/09 21:33:54]

Done.

+    _items.insert(_items.begin() + i, std::move(sourceItemCopy));
   }
 
+  // Update state to reflect inserted NavigationItems.
   _previousNavigationIndex = -1;
   _currentNavigationIndex += lastIndexToCopy + 1;
-  if (_pendingItemIndex != -1)
-    _pendingItemIndex += lastIndexToCopy + 1;
+  if (self.pendingItemIndex != -1)
+    self.pendingItemIndex += lastIndexToCopy + 1;
 
-  DCHECK_LT(static_cast<NSUInteger>(_currentNavigationIndex), _entries.count);
-  DCHECK(_pendingItemIndex == -1 || _pendingEntry);
+  DCHECK_LT(static_cast<NSUInteger>(_currentNavigationIndex),
+            self.items.size());
+  DCHECK(self.pendingItemIndex == -1 || self.pendingItem);
 }
 
 - (void)goToItemAtIndex:(NSInteger)index {
-  if (index < 0 || static_cast<NSUInteger>(index) >= _entries.count)
+  if (index < 0 || static_cast<NSUInteger>(index) >= self.items.size())
     return;
 
   if (index < _currentNavigationIndex) {
@@ -570,42 +553,19 @@ - (void)goToItemAtIndex:(NSInteger)index {
 }
 
 - (void)removeItemAtIndex:(NSInteger)index {
-  DCHECK(index < static_cast<NSInteger>([_entries count]));
+  DCHECK(index < static_cast<NSInteger>(self.items.size()));
   DCHECK(index != _currentNavigationIndex);
   DCHECK(index >= 0);
 
   [self discardNonCommittedItems];
 
-  [_entries removeObjectAtIndex:index];
+  _items.erase(_items.begin() + index);
   if (_currentNavigationIndex > index)
     _currentNavigationIndex--;
   if (_previousNavigationIndex >= index)
     _previousNavigationIndex--;
 }
 
-- (NSArray*)backwardEntries {
-  NSMutableArray* entries = [NSMutableArray array];
-  for (NSInteger index = _currentNavigationIndex; index > 0; --index) {
-    if (![self isRedirectTransitionForItemAtIndex:index])
-      [entries addObject:_entries[index - 1]];
-  }
-  return entries;
-}
-
-- (NSArray*)forwardEntries {
-  NSMutableArray* entries = [NSMutableArray array];
-  NSUInteger lastNonRedirectedIndex = _currentNavigationIndex + 1;
-  while (lastNonRedirectedIndex < [_entries count]) {
-    CRWSessionEntry* entry = [_entries objectAtIndex:lastNonRedirectedIndex];
-    if (!ui::PageTransitionIsRedirect(
-            entry.navigationItem->GetTransitionType())) {
-      [entries addObject:entry];
-    }
-    ++lastNonRedirectedIndex;
-  }
-  return entries;
-}
-
 - (BOOL)isSameDocumentNavigationBetweenItem:(web::NavigationItem*)firstItem
                                     andItem:(web::NavigationItem*)secondItem {
   if (!firstItem || !secondItem || firstItem == secondItem)
@@ -618,12 +578,12 @@ - (BOOL)isSameDocumentNavigationBetweenItem:(web::NavigationItem*)firstItem
   NSUInteger endIndex = firstIndex < secondIndex ? secondIndex : firstIndex;
 
   for (NSUInteger i = startIndex + 1; i <= endIndex; i++) {
-    web::NavigationItemImpl* item = [_entries[i] navigationItemImpl];
-    // Every entry in the sequence has to be created from a hash change or
+    web::NavigationItemImpl* item = self.items[i].get();
+    // Every item in the sequence has to be created from a hash change or
     // pushState() call.
     if (!item->IsCreatedFromPushState() && !item->IsCreatedFromHashChange())
       return NO;
-    // Every entry in the sequence has to have a URL that could have been
+    // Every item in the sequence has to have a URL that could have been
     // created from a pushState() call.
     if (!web::history_state_util::IsHistoryStateChangeValid(firstItem->GetURL(),
                                                             item->GetURL()))
@@ -632,42 +592,29 @@ - (BOOL)isSameDocumentNavigationBetweenItem:(web::NavigationItem*)firstItem
   return YES;
 }
 
-- (CRWSessionEntry*)lastUserEntry {
-  if (![_entries count])
-    return nil;
-
-  NSInteger index = _currentNavigationIndex;
-  // This will return the first session entry if all other entries are
-  // redirects, regardless of the transition state of the first entry.
-  while (index > 0 && [self isRedirectTransitionForItemAtIndex:index]) {
-    --index;
-  }
-  return [_entries objectAtIndex:index];
-}
-
 - (NSInteger)indexOfItem:(const web::NavigationItem*)item {
-  for (NSUInteger index = 0; index < self.entries.count; ++index) {
-    if ([self.entries[index] navigationItem] == item)
-      return static_cast<NSInteger>(index);
+  DCHECK(item);
+  for (size_t index = 0; index < self.items.size(); ++index) {
+    if (self.items[index].get() == item)
+      return index;
   }
   return NSNotFound;
 }
 
 - (web::NavigationItemImpl*)itemAtIndex:(NSInteger)index {
-  if (index < 0 || self.entries.count <= static_cast<NSUInteger>(index))
+  if (index < 0 || self.items.size() <= static_cast<NSUInteger>(index))
     return nullptr;
-  return static_cast<web::NavigationItemImpl*>(
-      [self.entries[index] navigationItem]);
+  return self.items[index].get();
 }
 
 #pragma mark -
 #pragma mark Private methods
 
-- (CRWSessionEntry*)sessionEntryWithURL:(const GURL&)url
-                               referrer:(const web::Referrer&)referrer
-                             transition:(ui::PageTransition)transition
-                         initiationType:
-                             (web::NavigationInitiationType)initiationType {
+- (std::unique_ptr<web::NavigationItemImpl>)
+   itemWithURL:(const GURL&)url
+      referrer:(const web::Referrer&)referrer
+    transition:(ui::PageTransition)transition
+initiationType:(web::NavigationInitiationType)initiationType {
   GURL loaded_url(url);
   BOOL urlWasRewritten = NO;
   if (_navigationManager) {
@@ -691,20 +638,13 @@ - (CRWSessionEntry*)sessionEntryWithURL:(const GURL&)url
   item->SetNavigationInitiationType(initiationType);
   if (web::GetWebClient()->IsAppSpecificURL(loaded_url))
     item->SetUserAgentType(web::UserAgentType::NONE);
-  return [[CRWSessionEntry alloc] initWithNavigationItem:std::move(item)];
+  return item;
 }
 
-- (BOOL)isRedirectTransitionForItemAtIndex:(NSInteger)index {
-  ui::PageTransition transition =
-      [_entries[index] navigationItem]->GetTransitionType();
+- (BOOL)isRedirectTransitionForItemAtIndex:(size_t)index {
+  DCHECK_LT(index, self.items.size());
+  ui::PageTransition transition = self.items[index]->GetTransitionType();
   return (transition & ui::PAGE_TRANSITION_IS_REDIRECT_MASK) ? YES : NO;
 }
 
-- (web::NavigationItemList)itemListForEntryList:(NSArray*)entries {
-  web::NavigationItemList list(entries.count);
-  for (size_t index = 0; index < entries.count; ++index)
-    list[index] = [entries[index] navigationItem];
-  return list;
-}
-
 @end