Fix missing back navigation item when pending item is displayed.

Fix missing back navigation item when pending item is displayed.

Current back navigation list assumes that the visible item is the last
committed item. This causes the most recently committed item to be
missing from the back navigation list when an interstitials is
displayed. This change fixes this buggy behavior.

R=eugenebut@chromium.org
BUG=691311
TESTED=Tested in simulator. Also added unit tests to ios_web_unittests.

Review-Url: https://codereview.chromium.org/2838003002
Cr-Commit-Position: refs/heads/master@{#467477}
Committed: https://chromium.googlesource.com/chromium/src/+/a346dfe7ace9e2e9cf60ff07a43e2811e513d150

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

eugenebut@chromium.org changed reviewers:
+ kkhorimoto@chromium.org

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

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller.mm:208: // However, if the visible
navigation item is a transient or uncommitted
Do we need to include pending item into backwardItems list? We probably should
do the same thing as desktop.

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller.mm:209: // pending item, an example
being safe browsing warning interstitials, all
s/safe browsing warning/SSL

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

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller.mm:212:
items.push_back(self.items[_lastCommittedItemIndex].get());
We probably should not include redirect item. Would it be better to initialize
|index| outside of for loop instead of pushing back lastCommittedItem?

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
File ios/web/navigation/crw_session_controller_unittest.mm (right):

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller_unittest.mm:1263:
web::NavigationItemList backItems = [session_controller_ backwardItems];
s/backItems/back_items

https://chromium.googlesource.com/chromium/src/+/master/styleguide/objective-...

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller_unittest.mm:1264: EXPECT_EQ(1U,
backItems.size());
s/EXPECT_EQ/ASSERT_EQ, otherwise line 1265 may crash if vector is empty

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

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller.mm:208: // However, if the visible
navigation item is a transient or uncommitted
On 2017/04/25 05:49:01, Eugene But wrote:
> Do we need to include pending item into backwardItems list? We probably should
> do the same thing as desktop.

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.

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller.mm:209: // pending item, an example
being safe browsing warning interstitials, all
On 2017/04/25 05:49:01, Eugene But wrote:
> s/safe browsing warning/SSL
> 
> Unlike other platforms, Chrome for iOS does not support Safe Browsing and Safe
> Browsing Interstitials.

Done.

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller.mm:212:
items.push_back(self.items[_lastCommittedItemIndex].get());
On 2017/04/25 05:49:01, Eugene But wrote:
> We probably should not include redirect item. Would it be better to initialize
> |index| outside of for loop instead of pushing back lastCommittedItem?

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.

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
File ios/web/navigation/crw_session_controller_unittest.mm (right):

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller_unittest.mm:1263:
web::NavigationItemList backItems = [session_controller_ backwardItems];
On 2017/04/25 05:49:01, Eugene But wrote:
> s/backItems/back_items
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/objective-...

Oops sorry confused Objective C and JavaScript style guide again... Done.

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller_unittest.mm:1263:
web::NavigationItemList backItems = [session_controller_ backwardItems];
On 2017/04/25 05:49:01, Eugene But wrote:
> s/backItems/back_items
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/objective-...

Done.

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller_unittest.mm:1264: EXPECT_EQ(1U,
backItems.size());
On 2017/04/25 05:49:01, Eugene But wrote:
> s/EXPECT_EQ/ASSERT_EQ, otherwise line 1265 may crash if vector is empty

Done.

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller_unittest.mm:1264: EXPECT_EQ(1U,
backItems.size());
On 2017/04/25 05:49:01, Eugene But wrote:
> s/EXPECT_EQ/ASSERT_EQ, otherwise line 1265 may crash if vector is empty

Done.

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

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller.mm:208: // However, if the visible
navigation item is a transient or uncommitted
On 2017/04/25 16:31:20, danyao wrote:
> On 2017/04/25 05:49:01, Eugene But wrote:
> > Do we need to include pending item into backwardItems list? We probably
should
> > do the same thing as desktop.
> 
> 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.
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.

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller.mm:212:
items.push_back(self.items[_lastCommittedItemIndex].get());
On 2017/04/25 16:31:20, danyao wrote:
> On 2017/04/25 05:49:01, Eugene But wrote:
> > We probably should not include redirect item. Would it be better to
initialize
> > |index| outside of for loop instead of pushing back lastCommittedItem?
> 
> 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.
You are right. Please ignore me.

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

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
ios/web/navigation/crw_session_controller.mm:208: // However, if the visible
navigation item is a transient or uncommitted
On 2017/04/25 20:44:40, Eugene But wrote:
> On 2017/04/25 16:31:20, danyao wrote:
> > On 2017/04/25 05:49:01, Eugene But wrote:
> > > Do we need to include pending item into backwardItems list? We probably
> should
> > > do the same thing as desktop.
> > 
> > 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.
> 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.

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

[danyao, 2017-04-25 22:21:41]

On 2017/04/25 21:23:11, Eugene But wrote:
>
https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
> File ios/web/navigation/crw_session_controller.mm (right):
> 
>
https://codereview.chromium.org/2838003002/diff/1/ios/web/navigation/crw_sess...
> ios/web/navigation/crw_session_controller.mm:208: // However, if the visible
> navigation item is a transient or uncommitted
> On 2017/04/25 20:44:40, Eugene But wrote:
> > On 2017/04/25 16:31:20, danyao wrote:
> > > On 2017/04/25 05:49:01, Eugene But wrote:
> > > > Do we need to include pending item into backwardItems list? We probably
> > should
> > > > do the same thing as desktop.
> > > 
> > > 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.
> > 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.
> 
> I think this code would be correct, unless I'm missing something:
> if (self.transientItem) {
>   items.push_back(self.items[_lastCommittedItemIndex].get());
> }

I think you're right. I was mistaken earlier to think that in desktop
pendingEntry created for new navigation is also in entries_. It is not, so
checking transientItem alone should be sufficient. Thanks for helping to figure
this out! Done.

[Eugene But (OOO till 7-30), 2017-04-26 05:42:10]

lgtm!

[kkhorimoto, 2017-04-26 10:48:13]

lgtm

[kkhorimoto, 2017-04-26 10:50:37]

https://codereview.chromium.org/2838003002/diff/40001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2838003002/diff/40001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:210:
items.push_back(self.items[_lastCommittedItemIndex].get());
Optional nit: items.push_back(self.lastCommittedItem)

[danyao, 2017-04-26 21:14:43]

On 2017/04/26 10:50:37, kkhorimoto_ wrote:
>
https://codereview.chromium.org/2838003002/diff/40001/ios/web/navigation/crw_...
> File ios/web/navigation/crw_session_controller.mm (right):
> 
>
https://codereview.chromium.org/2838003002/diff/40001/ios/web/navigation/crw_...
> ios/web/navigation/crw_session_controller.mm:210:
> items.push_back(self.items[_lastCommittedItemIndex].get());
> Optional nit: items.push_back(self.lastCommittedItem)

Done. Thanks for the suggestion. It actually made me realize there's an edge
case (i.e. has transientItem but no committedItem). Added some defensive checks
and unit test to cover.

[danyao, 2017-04-26 21:15:40]

The CQ bit was checked by danyao@chromium.org

[danyao, 2017-04-26 21:15:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2838003002/#ps60001
(title: "Fix edge case (no committed item) and added unit test to cover.")

[commit-bot: I haz the power, 2017-04-26 21:16:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 22:07:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493241340872660,
"parent_rev": "0e5bf385b3882b6a124ace5495c957e735b5ffcc", "commit_rev":
"a346dfe7ace9e2e9cf60ff07a43e2811e513d150"}

[commit-bot: I haz the power, 2017-04-26 22:08:07]

Description was changed from

==========
Fix missing back navigation item when pending item is displayed.

Current back navigation list assumes that the visible item is the last
committed item. This causes the most recently committed item to be
missing from the back navigation list when an interstitials is
displayed. This change fixes this buggy behavior.

R=eugenebut@chromium.org
BUG=691311
TESTED=Tested in simulator. Also added unit tests to ios_web_unittests.
==========

to

==========
Fix missing back navigation item when pending item is displayed.

Current back navigation list assumes that the visible item is the last
committed item. This causes the most recently committed item to be
missing from the back navigation list when an interstitials is
displayed. This change fixes this buggy behavior.

R=eugenebut@chromium.org
BUG=691311
TESTED=Tested in simulator. Also added unit tests to ios_web_unittests.

Review-Url: https://codereview.chromium.org/2838003002
Cr-Commit-Position: refs/heads/master@{#467477}
Committed:
https://chromium.googlesource.com/chromium/src/+/a346dfe7ace9e2e9cf60ff07a43e...
==========

[commit-bot: I haz the power, 2017-04-26 22:08:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a346dfe7ace9e2e9cf60ff07a43e...