Bug 680727 - Part 1: IHistory::VisitURI should accept a reloaded URI. r=mak

toolkit/components/places/History.cpp

@@ -505,6 +505,7 @@ public:
 
     History* history = History::GetService();
     NS_ENSURE_STATE(history);
+    history->AppendToRecentlyVisitedURIs(uri);
     history->NotifyVisited(uri);
 
     return NS_OK;
@@ -1453,6 +1454,7 @@ History* History::gService = NULL;
 History::History()
   : mShuttingDown(false)
   , mShutdownMutex("History::mShutdownMutex")
+  , mRecentlyVisitedURIsNextIndex(0)
 {
   NS_ASSERTION(!gService, "Ruh-roh!  This service has already been created!");
   gService = this;
@@ -1782,6 +1784,29 @@ History::Shutdown()
   }
 }
 
+void
+History::AppendToRecentlyVisitedURIs(nsIURI* aURI) {
+  if (mRecentlyVisitedURIs.Length() < RECENTLY_VISITED_URI_SIZE) {
+    // Append a new element while the array is not full.
+    mRecentlyVisitedURIs.AppendElement(aURI);
+  } else {
+    // Otherwise, replace the oldest member.
+    mRecentlyVisitedURIsNextIndex %= RECENTLY_VISITED_URI_SIZE;
+    mRecentlyVisitedURIs.ElementAt(mRecentlyVisitedURIsNextIndex) = aURI;
+    mRecentlyVisitedURIsNextIndex++;
+  }
+}
+
+inline bool
+History::IsRecentlyVisitedURI(nsIURI* aURI) {
+  bool equals = false;
+  RecentlyVisitedArray::index_type i;
+  for (i = 0; i < mRecentlyVisitedURIs.Length() && !equals; ++i) {
+    aURI->Equals(mRecentlyVisitedURIs.ElementAt(i), &equals);
+  }
+  return equals;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 //// IHistory
 
@@ -1818,8 +1843,8 @@ History::VisitURI(nsIURI* aURI,
     bool same;
     rv = aURI->Equals(aLastVisitedURI, &same);
     NS_ENSURE_SUCCESS(rv, rv);
-    if (same) {
-      // Do not save refresh-page visits.
+    if (same && IsRecentlyVisitedURI(aURI)) {
+      // Do not save refresh visits if we have visited this URI recently.
       return NS_OK;
     }
   }

toolkit/components/places/History.h

@@ -63,6 +63,9 @@ struct VisitData;
 #define NS_HISTORYSERVICE_CID \
   {0x0937a705, 0x91a6, 0x417a, {0x82, 0x92, 0xb2, 0x2e, 0xb1, 0x0d, 0xa8, 0x6c}}
 
+// Max size of History::mRecentlyVisitedURIs
+#define RECENTLY_VISITED_URI_SIZE 8
+
 class History : public IHistory
               , public nsIDownloadHistory
               , public mozIAsyncHistory
@@ -148,6 +151,12 @@ public:
     return mShutdownMutex;
   }
 
+  /**
+   * Helper function to append a new URI to mRecentlyVisitedURIs. See
+   * mRecentlyVisitedURIs.
+   */
+  void AppendToRecentlyVisitedURIs(nsIURI* aURI);
+
 private:
   virtual ~History();
 
@@ -219,6 +228,17 @@ private:
                                          void*);
 
   nsTHashtable<KeyClass> mObservers;
+
+  /**
+   * mRecentlyVisitedURIs remembers URIs which are recently added to the DB,
+   * to avoid saving these locations repeatedly in a short period.
+   */
+  typedef nsAutoTArray<nsCOMPtr<nsIURI>, RECENTLY_VISITED_URI_SIZE>
+          RecentlyVisitedArray;
+  RecentlyVisitedArray mRecentlyVisitedURIs;
+  RecentlyVisitedArray::index_type mRecentlyVisitedURIsNextIndex;
+
+  bool IsRecentlyVisitedURI(nsIURI* aURI);
 };
 
 } // namespace places

toolkit/components/places/tests/browser/Makefile.in

@@ -53,6 +53,7 @@ _BROWSER_FILES = \
 	browser_visituri_privatebrowsing.js \
 	browser_settitle.js \
 	browser_bug646422.js \
+	browser_bug680727.js \
 	$(NULL)
 
 # These are files that need to be loaded via the HTTP proxy server

toolkit/components/places/tests/browser/browser_bug680727.js

@@ -0,0 +1,100 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/* Ensure that clicking the button in the Offline mode neterror page updates
+   global history. See bug 680727. */
+/* TEST_PATH=toolkit/components/places/tests/browser/browser_bug680727.js make -C $(OBJDIR) mochitest-browser-chrome */
+
+
+const kUniqueURI = Services.io.newURI("http://mochi.test:8888/#bug_680727",
+                                      null, null);
+var gAsyncHistory = 
+  Cc["@mozilla.org/browser/history;1"].getService(Ci.mozIAsyncHistory);
+
+function test() {
+  waitForExplicitFinish();
+
+  gBrowser.selectedTab = gBrowser.addTab();
+
+  // Clear network cache.
+  Components.classes["@mozilla.org/network/cache-service;1"]
+            .getService(Components.interfaces.nsICacheService)
+            .evictEntries(Components.interfaces.nsICache.STORE_ANYWHERE);
+
+  // Go offline, expecting the error page.
+  Services.io.offline = true;
+  window.addEventListener("DOMContentLoaded", errorListener, false);
+  content.location = kUniqueURI.spec;
+}
+
+//------------------------------------------------------------------------------
+// listen to loading the neterror page. (offline mode)
+function errorListener() {
+  if(content.location == "about:blank") {
+    info("got about:blank, which is expected once, so return");
+    return;
+  }
+
+  window.removeEventListener("DOMContentLoaded", errorListener, false);
+  ok(Services.io.offline, "Services.io.offline is true.");
+
+  // This is an error page.
+  is(gBrowser.contentDocument.documentURI.substring(0, 27),
+     "about:neterror?e=netOffline",
+     "Document URI is the error page.");
+
+  // But location bar should show the original request.
+  is(content.location.href, kUniqueURI.spec,
+     "Docshell URI is the original URI.");
+
+  // Global history does not record URI of a failed request.
+  waitForAsyncUpdates(function() {
+    gAsyncHistory.isURIVisited(kUniqueURI, errorAsyncListener);
+  });
+}
+
+function errorAsyncListener(aURI, aIsVisited) {
+  ok(kUniqueURI.equals(aURI) && !aIsVisited,
+     "The neterror page is not listed in global history.");
+
+  // Now press the "Try Again" button, with offline mode off.
+  Services.io.offline = false;
+
+  window.addEventListener("DOMContentLoaded", reloadListener, false);
+
+  ok(gBrowser.contentDocument.getElementById("errorTryAgain"),
+     "The error page has got a #errorTryAgain element");
+  gBrowser.contentDocument.getElementById("errorTryAgain").click();
+}
+
+//------------------------------------------------------------------------------
+// listen to reload of neterror.
+function reloadListener() {
+  window.removeEventListener("DOMContentLoaded", reloadListener, false);
+
+  // This listener catches "DOMContentLoaded" on being called
+  // nsIWPL::onLocationChange(...). That is right *AFTER*
+  // IHistory::VisitURI(...) is called.
+  ok(!Services.io.offline, "Services.io.offline is false.");
+
+  // This is not an error page.
+  is(gBrowser.contentDocument.documentURI, kUniqueURI.spec, 
+     "Document URI is not the offline-error page, but the original URI.");
+
+  // Check if global history remembers the successfully-requested URI.
+  waitForAsyncUpdates(function() {
+    gAsyncHistory.isURIVisited(kUniqueURI, reloadAsyncListener);
+  });
+}
+
+function reloadAsyncListener(aURI, aIsVisited) {
+  ok(kUniqueURI.equals(aURI) && aIsVisited, "We have visited the URI.");
+  waitForClearHistory(finish);
+}
+
+registerCleanupFunction(function() {
+  Services.io.offline = false;
+  window.removeEventListener("DOMContentLoaded", errorListener, false);
+  window.removeEventListener("DOMContentLoaded", reloadListener, false);
+  gBrowser.removeCurrentTab();
+});

toolkit/components/places/tests/browser/head.js

@@ -10,3 +10,25 @@ function waitForClearHistory(aCallback) {
   }, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false);
   PlacesUtils.bhistory.removeAllPages();
 }
+
+function waitForAsyncUpdates(aCallback, aScope, aArguments)
+{
+  let scope = aScope || this;
+  let args = aArguments || [];
+  let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
+                              .DBConnection;
+  let begin = db.createAsyncStatement("BEGIN EXCLUSIVE");
+  begin.executeAsync();
+  begin.finalize();
+
+  let commit = db.createAsyncStatement("COMMIT");
+  commit.executeAsync({
+    handleResult: function() {},
+    handleError: function() {},
+    handleCompletion: function(aReason)
+    {
+      aCallback.apply(scope, args);
+    }
+  });
+  commit.finalize();
+}