Commit 30381c30 authored by Eugene But's avatar Eugene But Committed by Commit Bot

Fix GetPendingItemIndex() for new navigations.

Before this CL, GetPendingItemIndex() used to return last committed item
index for the new navigations (none-reload, none-back-forward).
Presumably this was done before CRWSessionController implemented
pendingItemIndex support, when CRWSessionController immediately changed committed item
during back-forward navigations. pendingItemIndex support design doc: https://docs.google.com/document/d/1GvsaoNtMO7d87FzoysOW5GqH6XErI5g2cinKhkPk7bQ/edit

This CL returns -1 for new navigations, which matches //content.

There are 3 places where GetPendingItemIndex is used outside of ios/web:
 - IOSLiveTab::GetPendingEntryIndex (new implementation matches content,
   which is the right approach)
 - GetPossiblyPendingItemAtIndex (new implementation should have no
   effect because index should be in [0;size] bounds, but this is just
   an assumption and it's actually unclear if GetPossiblyPendingItemAtIndex
   relied on undocumented and incorrect behavior).
 - ReadingListWebStateObserver::LoadOfflineReadingListEntry (new
   implementation should make the whole block of code obsolete, but
   it should be fine to leave it as it is because
   ReadingListWebStateObserver is going to be removed soon).

Bug: 665189, 899827
Change-Id: I9538b693c924790620140b37d6c4fea6cce26fde
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504231
Commit-Queue: Eugene But <eugenebut@chromium.org>
Reviewed-by: default avatarGauthier Ambard <gambard@chromium.org>
Reviewed-by: default avatarOlivier Robin <olivierrobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638637}
parent a5c54e0e
...@@ -158,15 +158,7 @@ int LegacyNavigationManagerImpl::GetIndexOfItem( ...@@ -158,15 +158,7 @@ int LegacyNavigationManagerImpl::GetIndexOfItem(
} }
int LegacyNavigationManagerImpl::GetPendingItemIndex() const { int LegacyNavigationManagerImpl::GetPendingItemIndex() const {
if (GetPendingItem()) { return [session_controller_ pendingItemIndex];
if ([session_controller_ pendingItemIndex] != -1) {
return [session_controller_ pendingItemIndex];
}
// TODO(crbug.com/665189): understand why last committed item index is
// returned here.
return GetLastCommittedItemIndex();
}
return -1;
} }
int LegacyNavigationManagerImpl:: int LegacyNavigationManagerImpl::
...@@ -358,7 +350,7 @@ bool LegacyNavigationManagerImpl::IsRestoreSessionInProgress() const { ...@@ -358,7 +350,7 @@ bool LegacyNavigationManagerImpl::IsRestoreSessionInProgress() const {
} }
void LegacyNavigationManagerImpl::SetPendingItemIndex(int index) { void LegacyNavigationManagerImpl::SetPendingItemIndex(int index) {
NOTREACHED(); session_controller_.pendingItemIndex = index;
} }
} // namespace web } // namespace web
...@@ -236,8 +236,7 @@ TEST_P(NavigationManagerTest, GetPendingItemIndexWithoutPendingEntry) { ...@@ -236,8 +236,7 @@ TEST_P(NavigationManagerTest, GetPendingItemIndexWithoutPendingEntry) {
EXPECT_EQ(-1, navigation_manager()->GetPendingItemIndex()); EXPECT_EQ(-1, navigation_manager()->GetPendingItemIndex());
} }
// Tests that GetPendingItemIndex() returns current item index if there is a // Tests that GetPendingItemIndex() returns -1 if there is a pending item.
// pending entry.
TEST_P(NavigationManagerTest, GetPendingItemIndexWithPendingEntry) { TEST_P(NavigationManagerTest, GetPendingItemIndexWithPendingEntry) {
navigation_manager()->AddPendingItem( navigation_manager()->AddPendingItem(
GURL("http://www.url.com"), Referrer(), ui::PAGE_TRANSITION_TYPED, GURL("http://www.url.com"), Referrer(), ui::PAGE_TRANSITION_TYPED,
...@@ -251,6 +250,19 @@ TEST_P(NavigationManagerTest, GetPendingItemIndexWithPendingEntry) { ...@@ -251,6 +250,19 @@ TEST_P(NavigationManagerTest, GetPendingItemIndexWithPendingEntry) {
GURL("http://www.url.com/0"), Referrer(), ui::PAGE_TRANSITION_TYPED, GURL("http://www.url.com/0"), Referrer(), ui::PAGE_TRANSITION_TYPED,
web::NavigationInitiationType::BROWSER_INITIATED, web::NavigationInitiationType::BROWSER_INITIATED,
web::NavigationManager::UserAgentOverrideOption::INHERIT); web::NavigationManager::UserAgentOverrideOption::INHERIT);
EXPECT_EQ(-1, navigation_manager()->GetPendingItemIndex());
}
// Tests that setting and getting PendingItemIndex.
TEST_P(NavigationManagerTest, SetAndGetPendingItemIndex) {
navigation_manager()->AddPendingItem(
GURL("http://www.url.test"), Referrer(), ui::PAGE_TRANSITION_TYPED,
web::NavigationInitiationType::BROWSER_INITIATED,
web::NavigationManager::UserAgentOverrideOption::INHERIT);
[mock_wk_list_ setCurrentURL:@"http://www.url.test"];
navigation_manager()->CommitPendingItem();
navigation_manager()->SetPendingItemIndex(0);
EXPECT_EQ(0, navigation_manager()->GetPendingItemIndex()); EXPECT_EQ(0, navigation_manager()->GetPendingItemIndex());
} }
......
...@@ -372,15 +372,9 @@ int WKBasedNavigationManagerImpl::GetIndexOfItem( ...@@ -372,15 +372,9 @@ int WKBasedNavigationManagerImpl::GetIndexOfItem(
} }
int WKBasedNavigationManagerImpl::GetPendingItemIndex() const { int WKBasedNavigationManagerImpl::GetPendingItemIndex() const {
if (GetPendingItem()) { if (is_restore_session_in_progress_)
if (pending_item_index_ != -1) { return -1;
return pending_item_index_; return pending_item_index_;
}
// TODO(crbug.com/665189): understand why last committed item index is
// returned here.
return GetLastCommittedItemIndex();
}
return -1;
} }
bool WKBasedNavigationManagerImpl::RemoveItemAtIndex(int index) { bool WKBasedNavigationManagerImpl::RemoveItemAtIndex(int index) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment