Replace PruneAllButActive with PruneAllButVisible.

content/public/browser/navigation_controller.h

Index: content/public/browser/navigation_controller.h
diff --git a/content/public/browser/navigation_controller.h b/content/public/browser/navigation_controller.h
index 51093018a9ba54315560abc330ed74e3bc64f771..6273d1b86060097bc56ba41ec347922f9352c65c 100644
--- a/content/public/browser/navigation_controller.h
+++ b/content/public/browser/navigation_controller.h
@@ -386,18 +386,38 @@ class NavigationController {
   virtual void CopyStateFrom(const NavigationController& source) = 0;
 
   // A variant of CopyStateFrom. Removes all entries from this except the last
-  // entry, inserts all entries from |source| before and including the active
-  // entry. This method is intended for use when the last entry of |this| is the
-  // active entry. For example:
+  // committed entry, and inserts all entries from |source| before and including
+  // its last committed entry. For example:
   // source: A B *C* D
-  // this:   E F *G*   (last must be active or pending)
+  // this:   E F *G*
   // result: A B C *G*
-  // This ignores the transient index of the source and honors that of 'this'.
-  virtual void CopyStateFromAndPrune(NavigationController* source) = 0;
-
-  // Removes all the entries except the active entry. If there is a new pending
-  // navigation it is preserved.
-  virtual void PruneAllButActive() = 0;
+  // If there is a pending entry after *G* in |this|, it is also preserved.
+  // This ignores any pending or transient entries in |source|, and it returns
+  // false if it is unsafe to call (see CanPruneAllButVisible).
+  virtual bool CopyStateFromAndPrune(NavigationController* source) = 0;

[cbentzel, 2013/05/28 18:39:17]

The bool return argument for this (as well as PruneAllButVisible) are only based
on CanPruneAllButVisible in the implementation. However - calling that is
specified as a prerequisite to calling these functions.

I think it would make sense to change these to return void (and CHECK for
CanPruneAllButVisible internally).

What do you think?

[Charlie Reis, 2013/05/28 23:22:00]

The current approach was partly driven by my level of confidence around the
ViewSource case.  I don't think there's a way for it to fail there, but it would
avoid crashing the browser process if I was wrong.  That said, your suggestion
feels cleaner, and it's probably better to hear about and fix any issues with
ViewSource than silently dealing with them.  I've changed it to CHECK instead.

+
+  // Returns whether it is safe to call PruneAllButVisible or
+  // CopyStateFromAndPrune.  There must be a last committed entry, no transient
+  // entry, and if there is a pending entry, it must be new and not an existing
+  // entry.
+  //
+  // If there were no last committed entry, the pending entry might not commit,
+  // leaving us with a blank page.  This is unsafe when used with
+  // |CopyStateFromAndPrune|, which would show an existing entry above the blank
+  // page.
+  // If there were a transient entry, we would not want to prune the other
+  // entries, which the transient entry could be referring to.
+  // If there were an existing pending entry, we could not prune the last
+  // committed entry, in case it did not commit.  That would leave us with no
+  // sensible place to put the pending entry when it did commit, after all other
+  // entries are pruned.  For example, it could be going back several entries.
+  // (New pending entries are safe, because they can always commit to the end.)
+  virtual bool CanPruneAllButVisible() = 0;
+
+  // Removes all the entries except the last committed entry. If there is a new
+  // pending navigation it is preserved.  Returns false if it is unsafe to call
+  // (see CanPruneAllButVisible).
+  virtual bool PruneAllButVisible() = 0;
 
   // Clears all screenshots associated with navigation entries in this
   // controller. Useful to reduce memory consumption in low-memory situations.