Always set the navigation pending item for page reload.

ios/chrome/browser/reading_list/reading_list_web_state_observer.mm

Index: ios/chrome/browser/reading_list/reading_list_web_state_observer.mm
diff --git a/ios/chrome/browser/reading_list/reading_list_web_state_observer.mm b/ios/chrome/browser/reading_list/reading_list_web_state_observer.mm
index 27d5f23d50e885e8c1fc888b6e2a617113a2d1c0..814bac16399c48b0fbdfa276da915eae7a2523b8 100644
--- a/ios/chrome/browser/reading_list/reading_list_web_state_observer.mm
+++ b/ios/chrome/browser/reading_list/reading_list_web_state_observer.mm
@@ -158,16 +158,12 @@ void ReadingListWebStateObserver::StartCheckingLoading() {
 
   web::NavigationManager* manager = web_state()->GetNavigationManager();
   web::NavigationItem* item = manager->GetPendingItem();
-  bool is_reload = false;
-
-  // Manager->GetPendingItem() returns null on reload.
+  // Manager->GetPendingItem() returns null on some cases.
   // TODO(crbug.com/676129): Remove this workaround once GetPendingItem()
-  // returns the correct value on reload.
+  // always returns the correct value.
   if (!item) {
     item = manager->GetLastCommittedItem();

[kkhorimoto, 2017/06/14 22:48:26]

By removing the |is_reload| calculation here, we're changing the behavior for
other scenarios in which there might not be a pending item.  What scenarios
remain where this can happen?  Do they affect this flow? 

[mrefaat, 2017/06/15 18:35:35]

from my testing & understanding nothing should change.
The reload calculation here was wrong, as sometimes the pending item is null and
its not a reload.

-    is_reload = true;
   }
-
   if (!ShouldObserveItem(item)) {
     StopCheckingProgress();
     return;
@@ -177,12 +173,12 @@ void ReadingListWebStateObserver::StartCheckingLoading() {
 
   pending_url_ = item->GetVirtualURL();
 
-  is_reload =
-      is_reload || ui::PageTransitionCoreTypeIs(item->GetTransitionType(),
-                                                ui::PAGE_TRANSITION_RELOAD);
   // If the user is reloading from the offline page, the intention is to access
   // the online page even on bad networks. No need to launch timer.
-  bool reloading_from_offline = last_load_was_offline && is_reload;
+  bool reloading_from_offline =
+      last_load_was_offline &&
+      ui::PageTransitionCoreTypeIs(item->GetTransitionType(),
+                                   ui::PAGE_TRANSITION_RELOAD);
 
   // No need to launch the timer either if there is no offline version to show.
   // Track |pending_url_| to mark the entry as read in case of a successful
@@ -244,13 +240,7 @@ void ReadingListWebStateObserver::VerifyIfReadingListEntryStartedLoading() {
   }
   web::NavigationManager* manager = web_state()->GetNavigationManager();
   web::NavigationItem* item = manager->GetPendingItem();
-
-  // Manager->GetPendingItem() returns null on reload.
-  // TODO(crbug.com/676129): Remove this workaround once GetPendingItem()
-  // returns the correct value on reload.
-  if (!item) {
-    item = manager->GetLastCommittedItem();
-  }

[kkhorimoto, 2017/06/14 22:48:26]

You're removing this fallback here, but not above.  Why is that?

[mrefaat, 2017/06/15 18:35:35]

I thought this is not going to be reachable in case of the reload but this is
actually not correct. i'll change it back 

+  DCHECK(item);

[Eugene But (OOO till 7-30), 2017/06/15 00:08:11]

Per Chromium Style Guide we should either have DCHECK or early return, but not
both:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[mrefaat, 2017/06/15 18:35:35]

Acknowledged.

   if (!item || !pending_url_.is_valid() ||
       !IsUrlAvailableOffline(pending_url_)) {
     StopCheckingProgress();
@@ -287,26 +277,10 @@ void ReadingListWebStateObserver::LoadOfflineReadingListEntry() {
   web::NavigationManager* navigationManager =
       web_state()->GetNavigationManager();
   web::NavigationItem* item = navigationManager->GetPendingItem();
-  if (!item) {
-    // Either the loading finished on error and the item is already committed,
-    // or the page is being reloaded and due to crbug.com/676129. there is no
-    // pending item. Either way, the correct item to reuse is the last committed
-    // item.
-    // TODO(crbug.com/676129): this case can be removed.
-    item = navigationManager->GetLastCommittedItem();

[kkhorimoto, 2017/06/14 22:48:26]

Same question here.

[mrefaat, 2017/06/15 18:35:35]

ditto

-    item->SetURL(url);
-    item->SetVirtualURL(pending_url_);
-    // It is not possible to call |navigationManager->Reload| at that point as
-    // an error page is currently displayed.
-    // Calling Reload will eventually call |ErrorPageContent reload| which
-    // simply loads |pending_url_| and will erase the distilled_url.
-    // Instead, go to the index that will branch further in the reload stack
-    // and avoid this situation.
-    navigationManager->GoToIndex(
-        navigationManager->GetLastCommittedItemIndex());
-  } else if (navigationManager->GetPendingItemIndex() != -1 &&
-             navigationManager->GetItemAtIndex(
-                 navigationManager->GetPendingItemIndex()) == item) {
+  DCHECK(item);
+  if (navigationManager->GetPendingItemIndex() != -1 &&
+      navigationManager->GetItemAtIndex(
+          navigationManager->GetPendingItemIndex()) == item) {
     // The navigation corresponds to a back/forward. The item on the stack can
     // be reused for the offline navigation.
     // TODO(crbug.com/665189): GetPendingItemIndex() will return currentEntry()