backout d25bfdc458cf a=bustage
authorMargaret Leibovic <margaret.leibovic@gmail.com>
Tue, 23 Nov 2010 21:16:23 -0800
changeset 58125 66d88ee2623736105f7da1add40adb871fb33208
parent 58124 64c4716a72dd8d53e09b25b99c3540c8b98caaeb
child 58126 367398a8cbfe92969021d926cf0beb4d64bca337
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
reviewersbustage
milestone2.0b8pre
backs outd25bfdc458cf2edd9dd590afd3dc54633095bc59
backout d25bfdc458cf a=bustage
content/base/public/nsIDocument.h
content/base/src/nsDocument.cpp
docshell/shistory/public/nsISHEntry.idl
docshell/shistory/src/nsSHEntry.cpp
docshell/shistory/src/nsSHEntry.h
dom/indexedDB/IDBDatabase.h
dom/indexedDB/IndexedDatabaseManager.cpp
dom/indexedDB/test/Makefile.in
dom/indexedDB/test/bfcache_iframe1.html
dom/indexedDB/test/bfcache_iframe2.html
dom/indexedDB/test/test_bfcache.html
layout/base/nsDocumentViewer.cpp
--- a/content/base/public/nsIDocument.h
+++ b/content/base/public/nsIDocument.h
@@ -102,17 +102,16 @@ template<class E> class nsCOMArray;
 class nsIDocumentObserver;
 class nsBindingManager;
 class nsIDOMNodeList;
 class mozAutoSubtreeModified;
 struct JSObject;
 class nsFrameLoader;
 class nsIBoxObject;
 class imgIRequest;
-class nsISHEntry;
 
 namespace mozilla {
 namespace css {
 class Loader;
 } // namespace css
 
 namespace dom {
 class Link;
@@ -446,26 +445,21 @@ public:
   virtual nsresult CreateShell(nsPresContext* aContext,
                                nsIViewManager* aViewManager,
                                nsStyleSet* aStyleSet,
                                nsIPresShell** aInstancePtrResult) = 0;
   virtual void DeleteShell() = 0;
 
   nsIPresShell* GetShell() const
   {
-    return GetBFCacheEntry() ? nsnull : mPresShell;
+    return mShellIsHidden ? nsnull : mPresShell;
   }
 
-  void SetBFCacheEntry(nsISHEntry* aSHEntry) {
-    mSHEntry = aSHEntry;
-    // Doing this just to keep binary compat for the gecko 2.0 release
-    mShellIsHidden = !!aSHEntry;
-  }
-
-  nsISHEntry* GetBFCacheEntry() const { return mSHEntry; }
+  void SetShellHidden(PRBool aHide) { mShellIsHidden = aHide; }
+  PRBool ShellIsHidden() const { return mShellIsHidden; }
 
   /**
    * Return the parent document of this document. Will return null
    * unless this document is within a compound document and has a
    * parent. Note that this parent chain may cross chrome boundaries.
    */
   nsIDocument *GetParentDocument() const
   {
@@ -1595,18 +1589,16 @@ protected:
   PRPackedBool mMathMLEnabled;
 
   // True if this document is the initial document for a window.  This should
   // basically be true only for documents that exist in newly-opened windows or
   // documents created to satisfy a GetDocument() on a window when there's no
   // document in it.
   PRPackedBool mIsInitialDocumentInWindow;
 
-  // True if we're currently bfcached. This is only here for binary compat.
-  // Remove once the gecko 2.0 has branched and just use mSHEntry instead.
   PRPackedBool mShellIsHidden;
 
   PRPackedBool mIsRegularHTML;
   PRPackedBool mIsXUL;
 
   enum {
     eTriUnset = 0,
     eTriFalse,
@@ -1709,20 +1701,16 @@ protected:
 
   // Weak reference to mScriptGlobalObject QI:d to nsPIDOMWindow,
   // updated on every set of mSecriptGlobalObject.
   nsPIDOMWindow *mWindow;
 
   nsCOMPtr<nsIDocumentEncoder> mCachedEncoder;
 
   AnimationListenerList mAnimationFrameListeners;
-
-  // The session history entry in which we're currently bf-cached. Non-null
-  // if and only if we're currently in the bfcache.
-  nsISHEntry* mSHEntry;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(nsIDocument, NS_IDOCUMENT_IID)
 
 /**
  * mozAutoSubtreeModified batches DOM mutations so that a DOMSubtreeModified
  * event is dispatched, if necessary, when the outermost mozAutoSubtreeModified
  * object is deleted.
--- a/content/base/src/nsDocument.cpp
+++ b/content/base/src/nsDocument.cpp
@@ -3171,17 +3171,17 @@ nsDocument::doCreateShell(nsPresContext*
                           nsIViewManager* aViewManager, nsStyleSet* aStyleSet,
                           nsCompatibility aCompatMode,
                           nsIPresShell** aInstancePtrResult)
 {
   *aInstancePtrResult = nsnull;
 
   NS_ASSERTION(!mPresShell, "We have a presshell already!");
 
-  NS_ENSURE_FALSE(GetBFCacheEntry(), NS_ERROR_FAILURE);
+  NS_ENSURE_FALSE(mShellIsHidden, NS_ERROR_FAILURE);
 
   FillStyleSet(aStyleSet);
   
   nsCOMPtr<nsIPresShell> shell;
   nsresult rv = NS_NewPresShell(getter_AddRefs(shell));
   if (NS_FAILED(rv)) {
     return rv;
   }
--- a/docshell/shistory/public/nsISHEntry.idl
+++ b/docshell/shistory/public/nsISHEntry.idl
@@ -245,22 +245,16 @@ interface nsISHEntry : nsIHistoryEntry
     boolean hasDynamicallyAddedChild();
 
     /**
      * The history ID of the docshell.
      */
     attribute unsigned long long docshellID;
 };
 
-[scriptable, uuid(e0b0ac6d-cb29-4be2-b685-1755d6301a69)]
-interface nsISHEntryInternal : nsISupports
-{
-    [notxpcom] void RemoveFromBFCacheAsync();
-    [notxpcom] void RemoveFromBFCacheSync();
-};
 
 %{ C++
 // {BFD1A791-AD9F-11d3-BDC7-0050040A9B44}
 #define NS_SHENTRY_CID \
 {0xbfd1a791, 0xad9f, 0x11d3, {0xbd, 0xc7, 0x0, 0x50, 0x4, 0xa, 0x9b, 0x44}}
 
 #define NS_SHENTRY_CONTRACTID \
     "@mozilla.org/browser/session-history-entry;1"
--- a/docshell/shistory/src/nsSHEntry.cpp
+++ b/docshell/shistory/src/nsSHEntry.cpp
@@ -160,20 +160,20 @@ nsSHEntry::~nsSHEntry()
 {
   StopTrackingEntry(this);
 
   // Since we never really remove kids from SHEntrys, we need to null
   // out the mParent pointers on all our kids.
   mChildren.EnumerateForwards(ClearParentPtr, nsnull);
   mChildren.Clear();
 
-  if (mContentViewer) {
-    // RemoveFromBFCacheSync is virtual, so call the nsSHEntry version
-    // explicitly
-    nsSHEntry::RemoveFromBFCacheSync();
+  nsCOMPtr<nsIContentViewer> viewer = mContentViewer;
+  DropPresentationState();
+  if (viewer) {
+    viewer->Destroy();
   }
 
   mEditorData = nsnull;
 
 #ifdef DEBUG
   // This is not happening as far as I can tell from breakpad as of early November 2007
   nsExpirationTracker<nsSHEntry,3>::Iterator iterator(gHistoryTracker);
   nsSHEntry* elem;
@@ -182,18 +182,18 @@ nsSHEntry::~nsSHEntry()
   }
 #endif
 }
 
 //*****************************************************************************
 //    nsSHEntry: nsISupports
 //*****************************************************************************
 
-NS_IMPL_ISUPPORTS5(nsSHEntry, nsISHContainer, nsISHEntry, nsIHistoryEntry,
-                   nsIMutationObserver, nsISHEntryInternal)
+NS_IMPL_ISUPPORTS4(nsSHEntry, nsISHContainer, nsISHEntry, nsIHistoryEntry,
+                   nsIMutationObserver)
 
 //*****************************************************************************
 //    nsSHEntry: nsISHEntry
 //*****************************************************************************
 
 NS_IMETHODIMP nsSHEntry::SetScrollPosition(PRInt32 x, PRInt32 y)
 {
   mScrollPositionX = x;
@@ -249,17 +249,17 @@ nsSHEntry::SetContentViewer(nsIContentVi
     gHistoryTracker->AddObject(this);
 
     nsCOMPtr<nsIDOMDocument> domDoc;
     mContentViewer->GetDOMDocument(getter_AddRefs(domDoc));
     // Store observed document in strong pointer in case it is removed from
     // the contentviewer
     mDocument = do_QueryInterface(domDoc);
     if (mDocument) {
-      mDocument->SetBFCacheEntry(this);
+      mDocument->SetShellHidden(PR_TRUE);
       mDocument->AddMutationObserver(this);
     }
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -742,17 +742,17 @@ nsSHEntry::SyncPresentationState()
 }
 
 void
 nsSHEntry::DropPresentationState()
 {
   nsRefPtr<nsSHEntry> kungFuDeathGrip = this;
 
   if (mDocument) {
-    mDocument->SetBFCacheEntry(nsnull);
+    mDocument->SetShellHidden(PR_FALSE);
     mDocument->RemoveMutationObserver(this);
     mDocument = nsnull;
   }
   if (mContentViewer)
     mContentViewer->ClearHistoryEntry();
 
   StopTrackingEntry(this);
   mContentViewer = nsnull;
@@ -805,17 +805,17 @@ nsSHEntry::CharacterDataWillChange(nsIDo
 {
 }
 
 void
 nsSHEntry::CharacterDataChanged(nsIDocument* aDocument,
                                 nsIContent* aContent,
                                 CharacterDataChangeInfo* aInfo)
 {
-  RemoveFromBFCacheAsync();
+  DocumentMutated();
 }
 
 void
 nsSHEntry::AttributeWillChange(nsIDocument* aDocument,
                                dom::Element* aContent,
                                PRInt32 aNameSpaceID,
                                nsIAtom* aAttribute,
                                PRInt32 aModType)
@@ -824,45 +824,45 @@ nsSHEntry::AttributeWillChange(nsIDocume
 
 void
 nsSHEntry::AttributeChanged(nsIDocument* aDocument,
                             dom::Element* aElement,
                             PRInt32 aNameSpaceID,
                             nsIAtom* aAttribute,
                             PRInt32 aModType)
 {
-  RemoveFromBFCacheAsync();
+  DocumentMutated();
 }
 
 void
 nsSHEntry::ContentAppended(nsIDocument* aDocument,
                            nsIContent* aContainer,
                            nsIContent* aFirstNewContent,
                            PRInt32 /* unused */)
 {
-  RemoveFromBFCacheAsync();
+  DocumentMutated();
 }
 
 void
 nsSHEntry::ContentInserted(nsIDocument* aDocument,
                            nsIContent* aContainer,
                            nsIContent* aChild,
                            PRInt32 /* unused */)
 {
-  RemoveFromBFCacheAsync();
+  DocumentMutated();
 }
 
 void
 nsSHEntry::ContentRemoved(nsIDocument* aDocument,
                           nsIContent* aContainer,
                           nsIContent* aChild,
                           PRInt32 aIndexInContainer,
                           nsIContent* aPreviousSibling)
 {
-  RemoveFromBFCacheAsync();
+  DocumentMutated();
 }
 
 void
 nsSHEntry::ParentChainChanged(nsIContent *aContent)
 {
 }
 
 class DestroyViewerEvent : public nsRunnable
@@ -880,37 +880,20 @@ public:
     return NS_OK;
   }
 
   nsCOMPtr<nsIContentViewer> mViewer;
   nsCOMPtr<nsIDocument> mDocument;
 };
 
 void
-nsSHEntry::RemoveFromBFCacheSync()
+nsSHEntry::DocumentMutated()
 {
   NS_ASSERTION(mContentViewer && mDocument,
-               "we're not in the bfcache!");
-
-  nsCOMPtr<nsIContentViewer> viewer = mContentViewer;
-  DropPresentationState();
-
-  // Warning! The call to DropPresentationState could have dropped the last
-  // reference to this nsSHEntry, so no accessing members beyond here.
-
-  if (viewer) {
-    viewer->Destroy();
-  }
-}
-
-void
-nsSHEntry::RemoveFromBFCacheAsync()
-{
-  NS_ASSERTION(mContentViewer && mDocument,
-               "we're not in the bfcache!");
+               "we shouldn't still be observing the doc");
 
   // Release the reference to the contentviewer asynchronously so that the
   // document doesn't get nuked mid-mutation.
 
   nsCOMPtr<nsIRunnable> evt =
       new DestroyViewerEvent(mContentViewer, mDocument);
   nsresult rv = NS_DispatchToCurrentThread(evt);
   if (NS_FAILED(rv)) {
--- a/docshell/shistory/src/nsSHEntry.h
+++ b/docshell/shistory/src/nsSHEntry.h
@@ -58,41 +58,40 @@
 #include "nsRect.h"
 #include "nsISupportsArray.h"
 #include "nsIMutationObserver.h"
 #include "nsExpirationTracker.h"
 #include "nsDocShellEditorData.h"
 
 class nsSHEntry : public nsISHEntry,
                   public nsISHContainer,
-                  public nsIMutationObserver,
-                  public nsISHEntryInternal
+                  public nsIMutationObserver
 {
 public: 
   nsSHEntry();
   nsSHEntry(const nsSHEntry &other);
 
   NS_DECL_ISUPPORTS
   NS_DECL_NSIHISTORYENTRY
   NS_DECL_NSISHENTRY
-  NS_DECL_NSISHENTRYINTERNAL
   NS_DECL_NSISHCONTAINER
   NS_DECL_NSIMUTATIONOBSERVER
 
   void DropPresentationState();
 
   void Expire();
   
   nsExpirationState *GetExpirationState() { return &mExpirationState; }
   
   static nsresult Startup();
   static void Shutdown();
   
 private:
   ~nsSHEntry();
+  void DocumentMutated();
 
   nsCOMPtr<nsIURI>                mURI;
   nsCOMPtr<nsIURI>                mReferrerURI;
   nsCOMPtr<nsIContentViewer>      mContentViewer;
   nsCOMPtr<nsIDocument>           mDocument; // document currently being observed
   nsString                        mTitle;
   nsCOMPtr<nsIInputStream>        mPostData;
   nsCOMPtr<nsILayoutHistoryState> mLayoutHistoryState;
--- a/dom/indexedDB/IDBDatabase.h
+++ b/dom/indexedDB/IDBDatabase.h
@@ -42,17 +42,16 @@
 
 #include "mozilla/dom/indexedDB/IndexedDatabase.h"
 
 #include "nsIIDBDatabase.h"
 
 #include "nsCycleCollectionParticipant.h"
 #include "nsDOMEventTargetHelper.h"
 #include "nsDOMLists.h"
-#include "nsIDocument.h"
 
 class nsIScriptContext;
 class nsPIDOMWindow;
 
 BEGIN_INDEXEDDB_NAMESPACE
 
 class AsyncConnectionHelper;
 struct DatabaseInfo;
@@ -105,23 +104,16 @@ public:
   }
 
   nsPIDOMWindow* Owner()
   {
     NS_ASSERTION(mOwner, "This should never be null!");
     return mOwner;
   }
 
-  already_AddRefed<nsIDocument> GetOwnerDocument()
-  {
-    NS_ASSERTION(mOwner, "This should never be null!");
-    nsCOMPtr<nsIDocument> doc = do_QueryInterface(mOwner->GetExtantDocument());
-    return doc.forget();
-  }
-
   bool IsQuotaDisabled();
 
   nsCString& Origin()
   {
     return mASCIIOrigin;
   }
 
   void Invalidate();
--- a/dom/indexedDB/IndexedDatabaseManager.cpp
+++ b/dom/indexedDB/IndexedDatabaseManager.cpp
@@ -51,17 +51,16 @@
 #include "nsXPCOMPrivate.h"
 
 #include "AsyncConnectionHelper.h"
 #include "IDBDatabase.h"
 #include "IDBEvents.h"
 #include "IDBFactory.h"
 #include "LazyIdleThread.h"
 #include "TransactionThreadPool.h"
-#include "nsISHEntry.h"
 
 // The amount of time, in milliseconds, that our IO thread will stay alive
 // after the last event it processes.
 #define DEFAULT_THREAD_TIMEOUT_MS 30000
 
 // The amount of time, in milliseconds, that we will wait for active database
 // transactions on shutdown before aborting them.
 #define DEFAULT_SHUTDOWN_TIMER_MS 30000
@@ -158,43 +157,28 @@ public:
     }
   }
 
   NS_IMETHOD Run()
   {
     NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
 
     // Fire version change events at all of the databases that are not already
-    // closed. Also kick bfcached documents out of bfcache.
+    // closed.
     for (PRUint32 index = 0; index < mWaitingDatabases.Length(); index++) {
       nsRefPtr<IDBDatabase>& database = mWaitingDatabases[index];
 
-      if (database->IsClosed()) {
-        continue;
-      }
+      if (!database->IsClosed()) {
 
-      // First check if the document the IDBDatabase is part of is bfcached
-      nsCOMPtr<nsIDocument> ownerDoc = database->GetOwnerDocument();
-      nsISHEntry* shEntry;
-      if (ownerDoc && (shEntry = ownerDoc->GetBFCacheEntry())) {
-        nsCOMPtr<nsISHEntryInternal> sheInternal = do_QueryInterface(shEntry);
-        if (sheInternal) {
-          sheInternal->RemoveFromBFCacheSync();
-        }
-        NS_ASSERTION(database->IsClosed(),
-                     "Kicking doc out of bfcache should have closed database");
-        continue;
+        nsCOMPtr<nsIDOMEvent> event(IDBVersionChangeEvent::Create(mVersion));
+        NS_ENSURE_TRUE(event, NS_ERROR_FAILURE);
+
+        PRBool dummy;
+        database->DispatchEvent(event, &dummy);
       }
-
-      // Otherwise fire a versionchange event.
-      nsCOMPtr<nsIDOMEvent> event(IDBVersionChangeEvent::Create(mVersion));
-      NS_ENSURE_TRUE(event, NS_ERROR_FAILURE);
-
-      PRBool dummy;
-      database->DispatchEvent(event, &dummy);
     }
 
     // Now check to see if any didn't close. If there are some running still
     // then fire the blocked event.
     for (PRUint32 index = 0; index < mWaitingDatabases.Length(); index++) {
       if (!mWaitingDatabases[index]->IsClosed()) {
         nsISupports* source =
           static_cast<nsPIDOMEventTarget*>(mRequestingDatabase);
--- a/dom/indexedDB/test/Makefile.in
+++ b/dom/indexedDB/test/Makefile.in
@@ -40,23 +40,20 @@ topsrcdir = @top_srcdir@
 srcdir = @srcdir@
 VPATH = @srcdir@
 relativesrcdir = dom/indexedDB/test
 
 include $(DEPTH)/config/autoconf.mk
 include $(topsrcdir)/config/rules.mk
 
 TEST_FILES = \
-  bfcache_iframe1.html \
-  bfcache_iframe2.html \
   event_propagation_iframe.html \
   helpers.js \
   test_add_twice_failure.html \
   test_bad_keypath.html \
-  test_bfcache.html \
   test_clear.html \
   test_create_index.html \
   test_create_objectStore.html \
   test_cursors.html \
   test_cursor_update_updates_indexes.html \
   test_event_propagation.html \
   test_event_source.html \
   test_getAll.html \
deleted file mode 100644
--- a/dom/indexedDB/test/bfcache_iframe1.html
+++ /dev/null
@@ -1,28 +0,0 @@
-<!DOCTYPE html>
-<html>
-<head>
-  <script>
-moz_indexedDB.open(parent.location).onsuccess = function(e) {
-  var db = e.result;
-  // This should never be called
-  db.onversionchange = function(e) {
-    db.transaction(["mystore"]).objectStore("mystore").put({ hello: "fail" }, 42);
-  }
-  db.setVersion("1.0").onsuccess = function(e) {
-    trans = e.transaction;
-    if (db.objectStoreNames.contains("mystore")) {
-      db.deleteObjectStore("mystore");
-    }
-    var store = db.createObjectStore("mystore", "");
-    store.add({ hello: "world" }, 42);
-    trans.oncomplete = function() {
-      parent.postMessage("go", "http://mochi.test:8888");
-    }
-  };
-};
-  </script>
-</head>
-<body>
-  This is page one.
-</body>
-</html>
deleted file mode 100644
--- a/dom/indexedDB/test/bfcache_iframe2.html
+++ /dev/null
@@ -1,31 +0,0 @@
-<!DOCTYPE html>
-<html>
-<head>
-  <script>
-var res = {};
-moz_indexedDB.open(parent.location).onsuccess = function(e) {
-  var db = e.result;
-  res.version = db.version;
-  res.storeCount = db.objectStoreNames.length;
-
-  req = db.setVersion("2.0");
-  req.onblocked = function() {
-    res.blockedFired = true;
-  }
-  req.onsuccess = function(e) {
-    var trans = e.transaction;
-    trans.objectStore("mystore").get(42).onsuccess = function(e) {
-      res.value = JSON.stringify(e.result);
-    }
-    trans.oncomplete = function() {
-      parent.postMessage(JSON.stringify(res), "http://mochi.test:8888");
-    }
-  }
-};
-
-  </script>
-</head>
-<body>
-  This is page two.
-</body>
-</html>
deleted file mode 100644
--- a/dom/indexedDB/test/test_bfcache.html
+++ /dev/null
@@ -1,65 +0,0 @@
-<!--
-  Any copyright is dedicated to the Public Domain.
-  http://creativecommons.org/publicdomain/zero/1.0/
--->
-<html>
-<head>
-  <title>Indexed Database Property Test</title>
-
-  <script type="text/javascript" src="/MochiKit/packed.js"></script>
-  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
-  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
-  <script type="text/javascript;version=1.7">
-    var gOrigMaxTotalViewers = undefined;
-    function setCachePref(enabled) {
-      netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
-      var prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
-                                 .getService(Components.interfaces.nsIPrefBranch);
-      if (enabled) {
-        is(typeof gOrigMaxTotalViewers, "undefined", "don't double-enable bfcache");
-        prefBranch.setBoolPref("browser.sessionhistory.cache_subframes", true);
-        gOrigMaxTotalViewers = prefBranch.getIntPref("browser.sessionhistory.max_total_viewers");
-        prefBranch.setIntPref("browser.sessionhistory.max_total_viewers", 10);
-      }
-      else {
-        is(typeof gOrigMaxTotalViewers, "number", "don't double-disable bfcache");
-        prefBranch.setIntPref("browser.sessionhistory.max_total_viewers", gOrigMaxTotalViewers);
-        gOrigMaxTotalViewers = undefined;
-        try {
-          prefBranch.clearUserPref("browser.sessionhistory.cache_subframes");
-        } catch (e) { /* Pref didn't exist, meh */ }
-      }
-    }
-
-    function testSteps()
-    {
-      var iframe = $("iframe");
-      setCachePref(true);
-      window.onmessage = grabEventAndContinueHandler;
-
-      iframe.src = "bfcache_iframe1.html";
-      var event = yield;
-      is(event.data, "go", "set up database successfully");
-
-      iframe.src = "bfcache_iframe2.html";
-      res = JSON.parse((yield).data);
-      is(res.version, "1.0", "version was set correctly");
-      is(res.storeCount, 1, "correct set of stores");
-      ok(!("blockedFired" in res), "blocked shouldn't fire");
-      is(res.value, JSON.stringify({ hello: "world" }),
-         "correct value found in store");
-
-      setCachePref(false);
-      finishTest();
-      yield;
-    }
-  </script>
-  <script type="text/javascript;version=1.7" src="helpers.js"></script>
-
-</head>
-
-<body onload="runTest();">
-  <iframe id="iframe"></iframe>
-</body>
-
-</html>
--- a/layout/base/nsDocumentViewer.cpp
+++ b/layout/base/nsDocumentViewer.cpp
@@ -1534,17 +1534,16 @@ DocumentViewerImpl::Destroy()
 
     Hide();
 
     // This is after Hide() so that the user doesn't see the inputs clear.
     if (mDocument) {
       nsresult rv = mDocument->Sanitize();
       if (NS_FAILED(rv)) {
         // If we failed to sanitize, don't save presentation.
-        // XXX Shouldn't we run all the stuff after the |if (mSHEntry)| then?
         savePresentation = PR_FALSE;
       }
     }
 
 
     // Reverse ownership. Do this *after* calling sanitize so that sanitize
     // doesn't cause mutations that make the SHEntry drop the presentation
     if (savePresentation) {
@@ -1556,19 +1555,18 @@ DocumentViewerImpl::Destroy()
     nsCOMPtr<nsISHEntry> shEntry = mSHEntry; // we'll need this below
     mSHEntry = nsnull;
 
     // Break the link from the document/presentation to the docshell, so that
     // link traversals cannot affect the currently-loaded document.
     // When the presentation is restored, Open() and InitInternal() will reset
     // these pointers to their original values.
 
-    if (mDocument) {
+    if (mDocument)
       mDocument->SetContainer(nsnull);
-    }
     if (mPresContext) {
       mPresContext->SetLinkHandler(nsnull);
       mPresContext->SetContainer(nsnull);
     }
     if (mPresShell)
       mPresShell->SetForwardingContainer(mContainer);
 
     // Do the same for our children.  Note that we need to get the child
@@ -1578,18 +1576,16 @@ DocumentViewerImpl::Destroy()
     while (NS_SUCCEEDED(shEntry->ChildShellAt(itemIndex++,
                                               getter_AddRefs(item))) && item) {
       DetachContainerRecurse(nsCOMPtr<nsIDocShell>(do_QueryInterface(item)));
     }
 
     return NS_OK;
   }
 
-  // The document was not put in the bfcache
-
   if (mDocument) {
     mDocument->Destroy();
     mDocument = nsnull;
   }
 
   // All callers are supposed to call destroy to break circular
   // references.  If we do this stuff in the destructor, the
   // destructor might never be called (especially if we're being