Replace PruneAllButActive with PruneAllButVisible.

content/public/browser/navigation_controller.h

 // 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.
 
 #ifndef CONTENT_PUBLIC_BROWSER_NAVIGATION_CONTROLLER_H_
 #define CONTENT_PUBLIC_BROWSER_NAVIGATION_CONTROLLER_H_
 
 #include <map>
 #include <string>
 #include <vector>
@@ skipping 368 matching lines @@
   // Broadcasts the NOTIFY_NAV_ENTRY_CHANGED notification for the given entry
   // (which must be at the given index). This will keep things in sync like
   // the saved session.
   virtual void NotifyEntryChanged(const NavigationEntry* entry, int index) = 0;
 
   // Copies the navigation state from the given controller to this one. This
   // one should be empty (just created).
   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;
+  // 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.

 
-  // Removes all the entries except the active entry. If there is a new pending
-  // navigation it is preserved.
-  virtual void PruneAllButActive() = 0;
+  // 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.
   virtual void ClearAllScreenshots() = 0;
 
  private:
   // This interface should only be implemented inside content.
   friend class NavigationControllerImpl;
   NavigationController() {}
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_NAVIGATION_CONTROLLER_H_