Fix missing back navigation item when pending item is displayed.

ios/web/navigation/crw_session_controller.mm

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/web/navigation/crw_session_controller.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
@@ skipping 184 matching lines @@
   return index == -1 ? nullptr : self.items[index].get();
 }
 
 - (web::NavigationItemImpl*)previousItem {
   NSInteger index = self.previousItemIndex;
   return index == -1 || self.items.empty() ? nullptr : self.items[index].get();
 }
 
 - (web::NavigationItemList)backwardItems {
   web::NavigationItemList items;
+
+  // Under normal circumstances, |visibleItem| is |lastCommittedItem| so
+  // backward history consists of all entries that precede |lastCommittedItem|.
+  // However, if the visible navigation item is a transient or uncommitted

[Eugene But (OOO till 7-30), 2017/04/25 05:49:01]

Do we need to include pending item into backwardItems list? We probably should
do the same thing as desktop.

[danyao, 2017/04/25 16:31:20]

I don't think pendingItem should be included in the backwardItems list. It never
finished navigation so is not yet part of the history.

The spec on history says "The joint session history of a top-level browsing
context is the union of all the session histories of all browsing contexts of
all the fully active Document objects that share that top-level browsing
context, with all the entries that are current entries in their respective
session histories removed except for the current entry of the joint session
history."

Desktop seems to implement this verbatim. They use a single list to keep all
navigation items. The last item in the list is either a transient item, a
pending item or the last committed item. The second last item in the list is
always a committed item. The transient and pending items never seem to stack on
each other. They implement currentItem as the last item in the navigation list,
hence history is everything before the last item. See
https://cs.chromium.org/chromium/src/chrome/browser/ui/toolbar/back_forward_m...

In Bling, it seems that we implemented session history as two pieces: _items,
which contains all committed items, and _pendingItem or _transientItem. So if
either _pendingItem of _transientItem is set, they are effectively the last item
in the session history. Using currentItem (switched from visibleItem that I used
in the previous patch) to detect this case matches the desktop implementation.

[Eugene But (OOO till 7-30), 2017/04/25 20:44:40]

Sorry, I incorrectly phrased my question, I meant "Do we need to include last
committed item if there is a pending item?". Consider this case:
SessionController has 2 committed items: url1 and url2. url2 is the last
committed item and url3 is a pending item. With the current implementation
backwardItems vector will have url1 and url2. So url2 will show up in the UI. On
desktop, however GetCurrentEntryIndex() will return 1 and url2 will not show up
in the UI. Am I missing something? Please note that in this case
pending_entry_index_ will be -1.

[Eugene But (OOO till 7-30), 2017/04/25 21:23:10]

I think this code would be correct, unless I'm missing something:
if (self.transientItem) {
  items.push_back(self.items[_lastCommittedItemIndex].get());
}

+  // pending item, an example being safe browsing warning interstitials, all

[Eugene But (OOO till 7-30), 2017/04/25 05:49:01]

s/safe browsing warning/SSL

Unlike other platforms, Chrome for iOS does not support Safe Browsing and Safe
Browsing Interstitials.

[danyao, 2017/04/25 16:31:20]

Done.

+  // committed items should be considered part of the backward history.
+  if (self.visibleItem != self.lastCommittedItem) {
+    items.push_back(self.items[_lastCommittedItemIndex].get());

[Eugene But (OOO till 7-30), 2017/04/25 05:49:01]

We probably should not include redirect item. Would it be better to initialize
|index| outside of for loop instead of pushing back lastCommittedItem?

[danyao, 2017/04/25 16:31:20]

The current behavior (starting line 215) is to include item[n] if item[n+1] is
not a redirect transition. For n = lastCommittedItemIndex, item[n+1] is either
the transient item or pending item, in that precedence order. In our case, the
transition type of the transient item is always PAGE_TRANSITION_CLIENT_REDIRECT
(see CRWSessionController, addTransientItemWithURL). So if we were to apply the
redirect check, it seems to me that the last committed item would still be
missing from backward history.

My interpretation of the redirect check rationale is that if item[n] -->
item[n+1] is a redirect transition, then there is no meaningful content on
item[n], so we should only show item[n-1] and item[n+1]. This doesn't apply to
our case since we know that the last committed has interesting information.

This change does not differentiate whether the item after lastCommitted is
pending or transient (and desktop seems not to).  One edge case I can think of
after this change is if we have page1 that automatically redirects to a
slow-loading page2. If the user clicks on back during loading, then page1 is
shown, which perhaps it shouldn't. I'm having a bit of trouble tracing desktop
code to figure out what they do. I'll see if I can find a WPT case or create
one.

[Eugene But (OOO till 7-30), 2017/04/25 20:44:40]

You are right. Please ignore me.

+  }
+
   for (size_t index = _lastCommittedItemIndex; index > 0; --index) {
     if (![self isRedirectTransitionForItemAtIndex:index])
       items.push_back(self.items[index - 1].get());
   }
   return items;
 }
 
 - (web::NavigationItemList)forwardItems {
   web::NavigationItemList items;
   NSUInteger lastNonRedirectedIndex = _lastCommittedItemIndex + 1;
@@ skipping 442 matching lines @@
   return item;
 }
 
 - (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;
 }
 
 @end