Bug 542941 - Part1: Clean up and comment AddVisitChain code, r=dietrich
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 10 Mar 2010 13:40:32 +0100
changeset 39197 c9262d953b60a074c7b3125d1957eb1469079d7e
parent 39196 665c2a7bede3ed2634cecd76f66b98525be0f909
child 39198 b6ed4adcd542be32660a9d83c36521cd3f45d84a
push id12076
push usermak77@bonardo.net
push dateWed, 10 Mar 2010 12:41:41 +0000
treeherdermozilla-central@6fead70c1560 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdietrich
bugs542941
milestone1.9.3a3pre
Bug 542941 - Part1: Clean up and comment AddVisitChain code, r=dietrich
toolkit/components/places/src/nsNavHistory.cpp
toolkit/components/places/src/nsNavHistory.h
--- a/toolkit/components/places/src/nsNavHistory.cpp
+++ b/toolkit/components/places/src/nsNavHistory.cpp
@@ -1962,29 +1962,35 @@ nsNavHistory::InternalAddVisit(PRInt64 a
 //
 //    This finds the most recent visit to the given URL. If found, it will put
 //    that visit's ID and session into the respective out parameters and return
 //    true. Returns false if no visit is found.
 //
 //    This is used to compute the referring visit.
 
 PRBool
-nsNavHistory::FindLastVisit(nsIURI* aURI, PRInt64* aVisitID,
+nsNavHistory::FindLastVisit(nsIURI* aURI,
+                            PRInt64* aVisitID,
+                            PRTime* aTime,
                             PRInt64* aSessionID)
 {
   mozStorageStatementScoper scoper(mDBRecentVisitOfURL);
   nsresult rv = BindStatementURI(mDBRecentVisitOfURL, 0, aURI);
   NS_ENSURE_SUCCESS(rv, PR_FALSE);
 
   PRBool hasMore;
   rv = mDBRecentVisitOfURL->ExecuteStep(&hasMore);
   NS_ENSURE_SUCCESS(rv, PR_FALSE);
   if (hasMore) {
-    *aVisitID = mDBRecentVisitOfURL->AsInt64(0);
-    *aSessionID = mDBRecentVisitOfURL->AsInt64(1);
+    rv = mDBRecentVisitOfURL->GetInt64(0, aVisitID);
+    NS_ENSURE_SUCCESS(rv, PR_FALSE);
+    rv = mDBRecentVisitOfURL->GetInt64(1, aSessionID);
+    NS_ENSURE_SUCCESS(rv, PR_FALSE);
+    rv = mDBRecentVisitOfURL->GetInt64(2, aTime);
+    NS_ENSURE_SUCCESS(rv, PR_FALSE);
     return PR_TRUE;
   }
   return PR_FALSE;
 }
 
 
 // nsNavHistory::IsURIStringVisited
 //
@@ -2763,21 +2769,22 @@ nsNavHistory::AddVisit(nsIURI* aURI, PRT
     // set as visited once, no title
     nsString voidString;
     voidString.SetIsVoid(PR_TRUE);
     rv = InternalAddNewPage(aURI, voidString, hidden == 1, typed == 1, 1,
                             PR_TRUE, &pageID);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
-  // Get the place id for the referrer, if we have one
+  // Get the place id for the referrer, if it exists.
   PRInt64 referringVisitID = 0;
   PRInt64 referringSessionID;
+  PRTime referringTime;
   if (aReferringURI &&
-      !FindLastVisit(aReferringURI, &referringVisitID, &referringSessionID)) {
+      !FindLastVisit(aReferringURI, &referringVisitID, &referringTime, &referringSessionID)) {
     // Add the referrer
     rv = AddVisit(aReferringURI, aTime - 1, nsnull, TRANSITION_LINK, PR_FALSE,
                   aSessionID, &referringVisitID);
     if (NS_FAILED(rv))
       referringVisitID = 0;
   }
 
   rv = InternalAddVisit(pageID, referringVisitID, aSessionID, aTime,
@@ -4181,20 +4188,19 @@ nsNavHistory::GetQueryResults(nsNavHisto
     for (i = 0; i < aQueries.Count(); i++) {
       rv = BindQueryClauseParameters(statement, i, aQueries[i], aOptions);
       NS_ENSURE_SUCCESS(rv, rv);
     }
   }
 
   addParams.EnumerateRead(BindAdditionalParameter, statement.get());
 
-  // optimize the case where we just use the results as is
-  // and we don't need to do any post-query filtering
+  // Optimize the case where there is no need for any post-query filtering.
   if (NeedToFilterResultSet(aQueries, aOptions)) {
-    // generate the toplevel results
+    // Generate the top-level results.
     nsCOMArray<nsNavHistoryResultNode> toplevel;
     rv = ResultsAsList(statement, aOptions, &toplevel);
     NS_ENSURE_SUCCESS(rv, rv);
 
     FilterResultSet(aResultNode, toplevel, aResults, aQueries, aOptions);
   } else {
     rv = ResultsAsList(statement, aOptions, aResults);
     NS_ENSURE_SUCCESS(rv, rv);
@@ -5142,128 +5148,143 @@ nsNavHistory::AddURIInternal(nsIURI* aUR
 //
 //    aRedirectBookmark should be empty when this function is first called. If
 //    there are any redirects that are bookmarks the specs will be placed in
 //    this buffer. The caller can then determine if any bookmarked items were
 //    visited so it knows whether to update the bookmark service's redirect
 //    hashtable.
 
 nsresult
-nsNavHistory::AddVisitChain(nsIURI* aURI, PRTime aTime,
-                            PRBool aToplevel, PRBool aIsRedirect,
-                            nsIURI* aReferrerURI, PRInt64* aVisitID,
-                            PRInt64* aSessionID, PRInt64* aRedirectBookmark)
-{
-  PRUint32 transitionType = 0;
-  PRInt64 referringVisit = 0;
-  PRTime visitTime = 0;
+nsNavHistory::AddVisitChain(nsIURI* aURI,
+                            PRTime aTime,
+                            PRBool aToplevel,
+                            PRBool aIsRedirect,
+                            nsIURI* aReferrerURI,
+                            PRInt64* aVisitID,
+                            PRInt64* aSessionID,
+                            PRInt64* aRedirectBookmark)
+{
+  // This is the address that will be saved to from_visit column, will be
+  // overwritten later if needed.
   nsCOMPtr<nsIURI> fromVisitURI = aReferrerURI;
 
   nsCAutoString spec;
   nsresult rv = aURI->GetSpec(spec);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  nsCAutoString redirectSource;
-  if (GetRedirectFor(spec, redirectSource, &visitTime, &transitionType)) {
-    // this was a redirect: See GetRedirectFor for info on how this works
-    nsCOMPtr<nsIURI> redirectURI;
-    rv = NS_NewURI(getter_AddRefs(redirectURI), redirectSource);
+  // Check if this visit came from a redirect.
+  PRUint32 transitionType = 0;
+  PRTime redirectTime = 0;
+  nsCAutoString redirectSourceUrl;
+  if (GetRedirectFor(spec, redirectSourceUrl, &redirectTime, &transitionType)) {
+    // redirectSourceUrl redirected to aURL, at redirectTime, with
+    // a transitionType redirect.
+    nsCOMPtr<nsIURI> redirectSourceURI;
+    rv = NS_NewURI(getter_AddRefs(redirectSourceURI), redirectSourceUrl);
     NS_ENSURE_SUCCESS(rv, rv);
 
     // remember if any redirect sources were bookmarked
     nsNavBookmarks *bookmarkService = nsNavBookmarks::GetBookmarksService();
     PRBool isBookmarked;
     if (bookmarkService &&
-        NS_SUCCEEDED(bookmarkService->IsBookmarked(redirectURI, &isBookmarked))
+        NS_SUCCEEDED(bookmarkService->IsBookmarked(redirectSourceURI, &isBookmarked))
         && isBookmarked) {
-      GetUrlIdFor(redirectURI, aRedirectBookmark, PR_FALSE);
+      GetUrlIdFor(redirectSourceURI, aRedirectBookmark, PR_FALSE);
     }
 
-    // Find the visit for the source. Note that we decrease the time counter,
-    // which will ensure that the referrer and this page will appear in history
-    // in the correct order. Since the times are in microseconds, it should not
-    // normally be possible to get two pages within one microsecond of each
-    // other so the referrer won't appear before a previous page viewed.
-    rv = AddVisitChain(redirectURI, aTime - 1, aToplevel, PR_TRUE, aReferrerURI,
-                       &referringVisit, aSessionID, aRedirectBookmark);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    // for redirects in frames, we don't want to see those items in history
-    // see bug #381453 for more details
+    // Recusively call addVisitChain to walk up the chain till the first
+    // not-redirected URI.
+    // Ensure that the sources have a visit time smaller than aTime, otherwise
+    // visits would end up incorrectly ordered.
+    PRTime sourceTime = NS_MIN(redirectTime, aTime - 1);
+    PRInt64 sourceVisitId = 0;
+    rv = AddVisitChain(redirectSourceURI, sourceTime, aToplevel,
+                       PR_TRUE, // Is a redirect.
+                       aReferrerURI, // This one is the originating source.
+                       &sourceVisitId, // Get back the visit id of the source.
+                       aSessionID,
+                       aRedirectBookmark);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // All the visits for preceding pages in the redirects chain have been
+    // added, now add the visit to aURI.
+    // This page comes from a redirect and is in a frame, it should not appear
+    // in history views so mark the visit as EMBED.
+    // See bug 381453 for details.
     if (!aToplevel) {
       transitionType = nsINavHistoryService::TRANSITION_EMBED;
     }
 
-    // We have been redirected, if the previous site was not a redirect
-    // update the referrer so we can walk up the redirect chain.
-    // See bug 411966 and Bug 428690 for details.
-    fromVisitURI = redirectURI;
-  } else if (aReferrerURI) {
-    // We do not want to add a new visit if the referring site is the same as
-    // the new site.  This is the situation where a page refreshes itself to
-    // give the user updated information.
+    // This page is result of a redirect, save the source page in from_visit,
+    // to be able to walk up the chain.
+    // See bug 411966 and bug 428690 for details.
+    // TODO: Add a closure table with a chain id to easily reconstruct chains
+    // without having to recurse through the table.  See bug 468710.
+    fromVisitURI = redirectSourceURI;
+  }
+  else if (aReferrerURI) {
+    // This page does not come from a redirect and had a referrer.
+
+    // Don't add a new visit if the referring site is the same as
+    // the new site.  This happens when a page refreshes itself.
     PRBool referrerIsSame;
-    if (NS_SUCCEEDED(aURI->Equals(aReferrerURI, &referrerIsSame)) && referrerIsSame)
+    if (NS_SUCCEEDED(aURI->Equals(aReferrerURI, &referrerIsSame)) &&
+        referrerIsSame)
       return NS_OK;
 
-    // If there is a referrer, we know you came from somewhere, either manually
-    // or automatically. For toplevel windows, assume its manual and you want
-    // to see this in history. For other things, it's some kind of embedded
-    // navigation. This is true of images and other content the user doesn't
-    // want to see in their history, but also of embedded frames that the user
-    // navigated manually and probably DOES want to see in history.
-    // Unfortunately, there isn't any easy way to distinguish these.
-    //
-    // Generally, it boils down to the problem of detecting whether a frame
-    // content change is the result of a user action, which isn't well defined
-    // since script could change a frame's source as a result of user request,
-    // or just because it feels like loading a new ad. The "back" button will
-    // undo either of these actions.
+    // Since referrer is set, this visit comes from an originating page.
+    // For top-level windows, visit is considered user-initiated and it should
+    // appear in history views.
+    // Visits to framed pages instead should be distinguished between
+    // user-initiated ones and automatic ones.
+    // The former should be shown, while the latter hidden.
+    // Unfortunately, the docshell does not provide enough information to
+    // distinguish them, so both are marked as EMBED visits.
+    // This is bad since link coloring will be active only for the session,
+    // while user wants it to persist for explicitly clicked links.
     if (aToplevel)
       transitionType = nsINavHistoryService::TRANSITION_LINK;
     else
       transitionType = nsINavHistoryService::TRANSITION_EMBED;
 
-    // Note that here we should NOT use the GetNow function. That function
-    // caches the value of "now" until next time the event loop runs. This
-    // gives better performance, but here we may get many notifications without
-    // running the event loop. We must preserve these events' ordering. This
-    // most commonly happens on redirects.
-    visitTime = PR_Now();
-
-    // Try to turn the referrer into a visit.
+    // Check if the referrer has a visit,
     // This also populates the session id.
-    if (!FindLastVisit(aReferrerURI, &referringVisit, aSessionID)) {
-      // we couldn't find a visit for the referrer, don't set it
+    PRTime lastVisitTime;
+    PRInt64 referringVisitId;
+    if (!FindLastVisit(aReferrerURI, &referringVisitId, &lastVisitTime, aSessionID) ||
+        aTime - lastVisitTime > RECENT_EVENT_THRESHOLD) {
+      // Either referrer does not have any visit, or that visit is too
+      // old to be part of this session.  Start a new session then.
       *aSessionID = GetNewSessionID();
     }
-  } else {
-    // When there is no referrer, we know the user must have gotten the link
-    // from somewhere, so check our sources to see if it was recently typed or
-    // has a bookmark selected. We don't handle drag-and-drop operations.
-    // note:  the link may have also come from a new window (set to load a homepage)
-    // or on start up (if we've set to load the home page or restore tabs)
-    // we treat these as TRANSITION_LINK (if they are top level) or
-    // TRANSITION_EMBED (if not top level).  We don't want to to add visits to 
-    // history without a transition type.
+  }
+  else {
+    // When there is no referrer:
+    // - Check recent events for a typed-in uri.
+    // - Check recent events for a bookmark selection.
+    // - Otherwise mark as TRANSITION_LINK or TRANSITION_EMBED depending on
+    //   whether it happens in a frame (see above for reasoning about this).
+    // Drag and drop operations are not handled, so they will most likely
+    // be marked as links.
     if (CheckIsRecentEvent(&mRecentTyped, spec))
       transitionType = nsINavHistoryService::TRANSITION_TYPED;
     else if (CheckIsRecentEvent(&mRecentBookmark, spec))
       transitionType = nsINavHistoryService::TRANSITION_BOOKMARK;
     else if (aToplevel)
       transitionType = nsINavHistoryService::TRANSITION_LINK;
     else
       transitionType = nsINavHistoryService::TRANSITION_EMBED;
 
-    visitTime = PR_Now();
+    // Since there is no referrer, there is no way to continue am existing
+    // session.
     *aSessionID = GetNewSessionID();
   }
 
-  // this call will create the visit and create/update the page entry
-  return AddVisit(aURI, visitTime, fromVisitURI, transitionType,
+  // Create the visit and update the page entry.
+  return AddVisit(aURI, aTime, fromVisitURI, transitionType,
                   aIsRedirect, *aSessionID, aVisitID);
 }
 
 
 // nsNavHistory::IsVisited
 //
 //    Note that this ignores the "hidden" flag. This function just checks if the
 //    given page is in the DB for link coloring. The "hidden" flag affects
@@ -5414,17 +5435,19 @@ nsNavHistory::AddDocumentRedirect(nsICha
                                   nsIChannel *aNewChannel,
                                   PRInt32 aFlags,
                                   PRBool aTopLevel)
 {
   NS_ASSERTION(NS_IsMainThread(), "This can only be called on the main thread");
   NS_ENSURE_ARG(aOldChannel);
   NS_ENSURE_ARG(aNewChannel);
 
-  // ignore internal redirects
+  // Ignore internal redirects.
+  // These redirects are not initiated by the remote server, but specific to the
+  // channel implementation, so they are ignored.
   if (aFlags & nsIChannelEventSink::REDIRECT_INTERNAL)
     return NS_OK;
 
   nsresult rv;
   nsCOMPtr<nsIURI> oldURI, newURI;
   rv = aOldChannel->GetURI(getter_AddRefs(oldURI));
   NS_ENSURE_SUCCESS(rv, rv);
   rv = aNewChannel->GetURI(getter_AddRefs(newURI));
@@ -5432,29 +5455,29 @@ nsNavHistory::AddDocumentRedirect(nsICha
 
   nsCString oldSpec, newSpec;
   rv = oldURI->GetSpec(oldSpec);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = newURI->GetSpec(newSpec);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (mRecentRedirects.Count() > RECENT_EVENT_QUEUE_MAX_LENGTH) {
-    // expire out-of-date ones
+    // Expire outdated cached redirects.
     PRInt64 threshold = PR_Now() - RECENT_EVENT_THRESHOLD;
     mRecentRedirects.Enumerate(ExpireNonrecentRedirects,
                                reinterpret_cast<void*>(&threshold));
   }
 
   RedirectInfo info;
 
-  // remove any old entries for this redirect destination
+  // Remove any old entries for this redirect destination, since they are going
+  // to be replaced.
   if (mRecentRedirects.Get(newSpec, &info))
     mRecentRedirects.Remove(newSpec);
-
-  // save the new redirect info
+  // Save the new redirect info.
   info.mSourceURI = oldSpec;
   info.mTimeCreated = PR_Now();
   if (aFlags & nsIChannelEventSink::REDIRECT_TEMPORARY)
     info.mType = TRANSITION_REDIRECT_TEMPORARY;
   else
     info.mType = TRANSITION_REDIRECT_PERMANENT;
   mRecentRedirects.Put(newSpec, info);
 
@@ -6688,21 +6711,23 @@ nsNavHistory::ExpireNonrecentEvents(Rece
 //    transition of typed and an outgoing transition of redirect.
 //
 //    Note that this can get confused in some cases where you have a page
 //    open in more than one window loading at the same time. This should be rare,
 //    however, and should not affect much.
 
 PRBool
 nsNavHistory::GetRedirectFor(const nsACString& aDestination,
-                             nsACString& aSource, PRTime* aTime,
+                             nsACString& aSource,
+                             PRTime* aTime,
                              PRUint32* aRedirectType)
 {
   RedirectInfo info;
   if (mRecentRedirects.Get(aDestination, &info)) {
+    // Consume the redirect entry, it's no longer useful.
     mRecentRedirects.Remove(aDestination);
     if (info.mTimeCreated < GetNow() - RECENT_EVENT_THRESHOLD)
       return PR_FALSE; // too long ago, probably invalid
     aSource = info.mSourceURI;
     *aTime = info.mTimeCreated;
     *aRedirectType = info.mType;
     return PR_TRUE;
   }
--- a/toolkit/components/places/src/nsNavHistory.h
+++ b/toolkit/components/places/src/nsNavHistory.h
@@ -491,17 +491,19 @@ protected:
                          PRInt64* aSessionID, PRInt64* aRedirectBookmark);
   nsresult InternalAddNewPage(nsIURI* aURI, const nsAString& aTitle,
                               PRBool aHidden, PRBool aTyped,
                               PRInt32 aVisitCount, PRBool aCalculateFrecency,
                               PRInt64* aPageID);
   nsresult InternalAddVisit(PRInt64 aPageID, PRInt64 aReferringVisit,
                             PRInt64 aSessionID, PRTime aTime,
                             PRInt32 aTransitionType, PRInt64* aVisitID);
-  PRBool FindLastVisit(nsIURI* aURI, PRInt64* aVisitID,
+  PRBool FindLastVisit(nsIURI* aURI,
+                       PRInt64* aVisitID,
+                       PRTime* aTime,
                        PRInt64* aSessionID);
   PRBool IsURIStringVisited(const nsACString& url);
 
   /**
    * Loads all of the preferences that we use into member variables.
    *
    * @note If mPrefBranch is NULL, this does nothing.
    */