Replace PruneAllButActive with PruneAllButVisible.

content/browser/web_contents/navigation_controller_impl.cc

 // Copyright (c) 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.
 
 #include "content/browser/web_contents/navigation_controller_impl.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/string_number_conversions.h"  // Temporary
 #include "base/string_util.h"
@@ skipping 1187 matching lines @@
   }
 
   FinishRestore(source.last_committed_entry_index_, RESTORE_CURRENT_SESSION);
 
   // Copy the max page id map from the old tab to the new tab.  This ensures
   // that new and existing navigations in the tab's current SiteInstances
   // are identified properly.
   web_contents_->CopyMaxPageIDsFrom(source.web_contents());
 }
 
-void NavigationControllerImpl::CopyStateFromAndPrune(
+bool NavigationControllerImpl::CopyStateFromAndPrune(
     NavigationController* temp) {
+  if (!CanPruneAllButVisible()) {
+    // It is up to callers to check the invariants before calling this.
+    NOTREACHED();
+    return false;
+  }
+
   NavigationControllerImpl* source =
       static_cast<NavigationControllerImpl*>(temp);
   // The SiteInstance and page_id of the last committed entry needs to be
   // remembered at this point, in case there is only one committed entry
   // and it is pruned.  We use a scoped_refptr to ensure the SiteInstance
   // can't be freed during this time period.
   NavigationEntryImpl* last_committed =
       NavigationEntryImpl::FromNavigationEntry(GetLastCommittedEntry());
   scoped_refptr<SiteInstance> site_instance(
-      last_committed ? last_committed->site_instance() : NULL);
-  int32 minimum_page_id = last_committed ? last_committed->GetPageID() : -1;
-  int32 max_page_id = last_committed ?
-      web_contents_->GetMaxPageIDForSiteInstance(site_instance.get()) : -1;
+      last_committed->site_instance());
+  int32 minimum_page_id = last_committed->GetPageID();
+  int32 max_page_id =
+      web_contents_->GetMaxPageIDForSiteInstance(site_instance.get());
 
   // This code is intended for use when the last entry is the active entry.
-  DCHECK(
-      (transient_entry_index_ != -1 &&
-       transient_entry_index_ == GetEntryCount() - 1) ||
-      (pending_entry_ && (pending_entry_index_ == -1 ||
-                          pending_entry_index_ == GetEntryCount() - 1)) ||
-      (!pending_entry_ && last_committed_entry_index_ == GetEntryCount() - 1));
+  DCHECK_EQ(GetEntryCount() - 1, last_committed_entry_index_);

[cbentzel, 2013/05/17 00:32:15]

I understand this was in here before. I think the DCHECK should either be
removed, or this logic needs to go into CanPruneAllButVisible.

In the prerender case, we could be in this state if the initial prerendered page
commits, does a navigation which also commits, and then goes back via
history.back()

I think it's OK to show the currently committed page in that case (which would
argue in favor of removing the DCHECK). Can you think of reasons that that
shouldn't happen?

[Charlie Reis, 2013/05/17 23:50:21]

We can't put it in CanPruneAllButVisible, because it's valid to call
PruneAllButVisible when we're not at the last entry.  ViewSource does that, for
example.
Hmm, I can't think of any reason why that wouldn't work.  I've removed the
condition and added a test for that case.

 
   // Remove all the entries leaving the active entry.
-  PruneAllButActiveInternal();
+  if (!PruneAllButVisibleInternal())
+    return false;
 
-  // We now have zero or one entries.  Ensure that adding the entries from
-  // source won't put us over the limit.
-  DCHECK(GetEntryCount() == 0 || GetEntryCount() == 1);
-  if (GetEntryCount() > 0)
-    source->PruneOldestEntryIfFull();
+  // We now have one entry, possibly with a new pending entry.  Ensure that
+  // adding the entries from source won't put us over the limit.
+  DCHECK_EQ(1, GetEntryCount());
+  source->PruneOldestEntryIfFull();
 
   // Insert the entries from source. Don't use source->GetCurrentEntryIndex as
-  // we don't want to copy over the transient entry.
-  int max_source_index = source->pending_entry_index_ != -1 ?
-      source->pending_entry_index_ : source->last_committed_entry_index_;
+  // we don't want to copy over the transient entry.  Ignore any pending entry,
+  // since it has not committed in source.
+  int max_source_index = source->last_committed_entry_index_;
   if (max_source_index == -1)
     max_source_index = source->GetEntryCount();
   else
     max_source_index++;
   InsertEntriesFrom(*source, max_source_index);
 
   // Adjust indices such that the last entry and pending are at the end now.
   last_committed_entry_index_ = GetEntryCount() - 1;
-  if (pending_entry_index_ != -1)
-    pending_entry_index_ = GetEntryCount() - 1;
-  if (transient_entry_index_ != -1) {
-    // There's a transient entry. In this case we want the last committed to
-    // point to the previous entry.
-    transient_entry_index_ = GetEntryCount() - 1;
-    if (last_committed_entry_index_ != -1)
-      last_committed_entry_index_--;
-  }
 
   web_contents_->SetHistoryLengthAndPrune(site_instance.get(),
                                           max_source_index,
                                           minimum_page_id);
 
   // Copy the max page id map from the old tab to the new tab.  This ensures
   // that new and existing navigations in the tab's current SiteInstances
   // are identified properly.
   web_contents_->CopyMaxPageIDsFrom(source->web_contents());
 
   // If there is a last committed entry, be sure to include it in the new
   // max page ID map.
   if (max_page_id > -1) {
     web_contents_->UpdateMaxPageIDForSiteInstance(site_instance.get(),
                                                   max_page_id);
   }
+
+  return true;
 }
 
-void NavigationControllerImpl::PruneAllButActive() {
-  PruneAllButActiveInternal();
+bool NavigationControllerImpl::CanPruneAllButVisible() {
+  // If there is no last committed entry, we cannot prune.  Even if there is a
+  // pending entry, it may not commit, leaving this WebContents blank, despite
+  // possibly giving it new entries via CopyStateFromAndPrune.
+  if (last_committed_entry_index_ == -1)
+    return false;
 
-  // If there is an entry left, we need to update the session history length of
-  // the RenderView.
-  if (!GetActiveEntry())
-    return;
+  // We cannot prune if there is a pending entry at an existing entry index.
+  // It may not commit, so we have to keep the last committed entry, and thus
+  // there is no sensible place to keep the pending entry.  It is ok to have
+  // a new pending entry, which can optionally commit as a new navigation.
+  if (pending_entry_index_ != -1)
+    return false;
+
+  // We should not prune if we are currently showing a transient entry.
+  if (transient_entry_index_ != -1)
+    return false;
+
+  return true;
+}
+
+bool NavigationControllerImpl::PruneAllButVisible() {
+  if (!PruneAllButVisibleInternal())
+    return false;
+
+  // We should still have a last committed entry.
+  DCHECK_NE(-1, last_committed_entry_index_);
 
   NavigationEntryImpl* entry =
       NavigationEntryImpl::FromNavigationEntry(GetActiveEntry());
   // We pass 0 instead of GetEntryCount() for the history_length parameter of
   // SetHistoryLengthAndPrune, because it will create history_length additional
   // history entries.
   // TODO(jochen): This API is confusing and we should clean it up.
   // http://crbug.com/178491
   web_contents_->SetHistoryLengthAndPrune(
       entry->site_instance(), 0, entry->GetPageID());
+
+  return true;
 }
 
-void NavigationControllerImpl::PruneAllButActiveInternal() {
-  if (transient_entry_index_ != -1) {
-    // There is a transient entry. Prune up to it.
-    DCHECK_EQ(GetEntryCount() - 1, transient_entry_index_);
-    entries_.erase(entries_.begin(), entries_.begin() + transient_entry_index_);
-    transient_entry_index_ = 0;
-    last_committed_entry_index_ = -1;
-    pending_entry_index_ = -1;
-  } else if (!pending_entry_) {
-    // There's no pending entry. Leave the last entry (if there is one).
-    if (!GetEntryCount())
-      return;
-
-    DCHECK_GE(last_committed_entry_index_, 0);
-    entries_.erase(entries_.begin(),
-                   entries_.begin() + last_committed_entry_index_);
-    entries_.erase(entries_.begin() + 1, entries_.end());
-    last_committed_entry_index_ = 0;
-  } else if (pending_entry_index_ != -1) {
-    entries_.erase(entries_.begin(), entries_.begin() + pending_entry_index_);
-    entries_.erase(entries_.begin() + 1, entries_.end());
-    pending_entry_index_ = 0;
-    last_committed_entry_index_ = 0;
-  } else {
-    // There is a pending_entry, but it's not in entries_.
-    pending_entry_index_ = -1;
-    last_committed_entry_index_ = -1;
-    entries_.clear();
+bool NavigationControllerImpl::PruneAllButVisibleInternal() {
+  if (!CanPruneAllButVisible()) {

[cbentzel, 2013/05/17 00:32:15]

Should this just be a DCHECK? The only callers are class methods which should
make sure that the invariant holds.

[Charlie Reis, 2013/05/17 23:50:21]

The public PruneAllButVisible doesn't call CanPruneAllButVisible.  We're relying
on external callers there, so I'd like to keep this.

+    // It is up to callers to check the invariants before calling this.
+    NOTREACHED();
+    return false;
   }
 
-  if (web_contents_->GetInterstitialPage()) {
-    // Normally the interstitial page hides itself if the user doesn't proceeed.
-    // This would result in showing a NavigationEntry we just removed. Set this
-    // so the interstitial triggers a reload if the user doesn't proceed.
-    static_cast<InterstitialPageImpl*>(web_contents_->GetInterstitialPage())->
-        set_reload_on_dont_proceed(true);
-  }
+  // Erase all entries but the last committed entry.  There may still be a
+  // new pending entry after this.
+  entries_.erase(entries_.begin(),
+                 entries_.begin() + last_committed_entry_index_);
+  entries_.erase(entries_.begin() + 1, entries_.end());
+  last_committed_entry_index_ = 0;
+
+  return true;
 }
 
 void NavigationControllerImpl::ClearAllScreenshots() {
   screenshot_manager_->ClearAllScreenshots();
 }
 
 void NavigationControllerImpl::SetSessionStorageNamespace(
     const std::string& partition_id,
     SessionStorageNamespace* session_storage_namespace) {
   if (!session_storage_namespace)
@@ skipping 331 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content