Refactor nsDocShell to use IHistory for marking pages as visited. Details are on this wiki page: https://wiki.mozilla.org/Firefox/Projects/Asynchronous_Add_Visit

Since this is all asynchronous, I am trying to figure out exactly what database calls need to be done. After taking a look at AddVisit and AddVisitChain (and getting a headache) and seeing this handy schema (http://people.mozilla.org/~dietrich/places-erd.png), my first stab at the necessary database calls: 1. see if entry exists in moz_places 2. see if entry is bookmarked 3. either update existing entry or add new entry in moz_places (update frecency, visit_count, hidden, and typed) 4. add entry to moz_historyvisits. visit_type is somehow grabbed from nsNavHistory

Frecency actually needs more thought for how to make it asynchronous. This patch will update the frecency synchronously after the other queries are made. The frecency work has been punted to dependent bug 556631.

(In reply to comment #1) > After taking a look at AddVisit and AddVisitChain (and getting a headache) and > seeing this handy schema (http://people.mozilla.org/~dietrich/places-erd.png), > my first stab at the necessary database calls: We were hoping you wouldn't look there; we are replacing most of that logic with this new method. > 1. see if entry exists in moz_places > 3. either update existing entry or add new entry in moz_places (update > frecency, visit_count, hidden, and typed) You can do these two things at the same time by simply doing an INSERT OR UPDATE statement possibly.

> We were hoping you wouldn't look there; we are replacing most of that logic > with this new method. Given the cryptic fields in the schema, I didn't have much of a choice ;)

Here's my first crack at this. Notes: * this uses synchronous nsNavHistory::GetNewSessionID--either needs a follow up bug or this needs to be fixed here * there is potentially race condition issues with updating places with visit information--I'm unsure if this would practically come up. Thoughts? Solutions? See the XXX in the patch for more info. * I used something similar to mak's favicon helper but more streamlined. It passes data around from step to step using an nsAutoPtr. * There are a few unimplemented things (see XXX comments), but I wanted to start getting feedback on my progress.

Comment on attachment 439094 [details] [diff] [review] WIP v1 uh this is huge, let's do this in pieces. >diff --git a/docshell/base/IHistory.h b/docshell/base/IHistory.h >+ /** >+ * @param aURI >+ * The URI of the page being visited. >+ * @param aReferrer >+ * The URI of the referring page. >+ * @param aFlags >+ * The VisitFlags describing this visit. >+ * @param aRedirectedFrom [optional] >+ * The URI this URI was redirected from, or NULL if not a re-direct. "re-direct"? >- mozilla::dom::Link *aContent); >+ mozilla::dom::Link *aContent); \ >+ NS_IMETHOD VisitURI( nsIURI *aURI, nsIURI *aReferrer, \ >+ PRUint32 aFlags, nsIURI *aRedirectedFrom = NULL); fancy indentation? >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp >+ nsCOMPtr<nsIURI> oldURI, newURI; >+ rv = aOldChannel->GetURI(getter_AddRefs(oldURI)); >+ rv = aNewChannel->GetURI(getter_AddRefs(newURI)); >+ if (!oldURI || !newURI) >+ return; >+ >+ mozilla::IHistory* history = nsContentUtils::GetHistory(); >+ if (!history) >+ return; >+ history->VisitURI(newURI, NULL, visitURIFlags, oldURI); maybe you want to if (history) history->Visit, otherwise you are cutting away next code > // check if the new load should go through the application cache. > nsCOMPtr<nsIApplicationCacheChannel> appCacheChannel = > do_QueryInterface(aNewChannel); > if (appCacheChannel) { > nsCOMPtr<nsIURI> newURI; looks like this code could reuse the uri object you made above? >@@ -10074,38 +10076,34 @@ nsDocShell::AddToGlobalHistory(nsIURI * > nsresult > nsDocShell::AddToGlobalHistory(nsIURI * aURI, PRBool aRedirect, > nsIURI * aReferrer) > { >+ if (mItemType != typeContent) >+ return NS_OK; >+ >+ nsIURI* redirect = NULL; >+ nsIURI* referrer = NULL; >+ if (aRedirect) { >+ redirect = aReferrer; >+ } else { >+ referrer = aReferrer; >+ } i'm not sure that aReferrer is the URI we have been redirected from here, i think it's probably different A -> B -> C if we are in C i think aReferrer could be A or A's referrer. we get redirects above in document redirect... Please ensure what happens. >+ >+ mozilla::IHistory* history = nsContentUtils::GetHistory(); >+ if (!history) >+ return NS_ERROR_FAILURE; were we throwing before? maybe we should just be silent >+ nsresult rv = history->VisitURI(aURI, referrer, >+ (IsFrame() ? 0: mozilla::IHistory::TOP_LEVEL), >+ redirect); >+ return rv; NS_ENSURE_SUCCESS(rv, rv); return NS_OK; so the failure origin is better tracked

(In reply to comment #6) > (From update of attachment 439094 [details] [diff] [review]) > >+ > >+ mozilla::IHistory* history = nsContentUtils::GetHistory(); > >+ if (!history) > >+ return NS_ERROR_FAILURE; > > were we throwing before? maybe we should just be silent ok, nvm, since this is an explicit "add to global history" request, throwing is fine, you can NS_ENSURE_STATE(history); it will throw NS_ERROR_FAILURE

Comment on attachment 439094 [details] [diff] [review] WIP v1 Yeah, i'll require more sweeps to see all code here, there is a lot. >diff --git a/toolkit/components/places/src/Helpers.cpp b/toolkit/components/places/src/Helpers.cpp >+void >+ReverseString(const nsString& aInput, nsAString& aReversed) >+{ >+ aReversed.Truncate(0); >+ for (PRInt32 i = aInput.Length() - 1; i >= 0; i --) that spacing before --... kill it! >diff --git a/toolkit/components/places/src/Helpers.h b/toolkit/components/places/src/Helpers.h > >+nsresult GetReversedHostname(nsIURI* aURI, nsAString& aRevHost); >+void GetReversedHostname(const nsString& aForward, nsAString& aRevHost); >+void ReverseString(const nsString& aInput, nsAString& aReversed); it is up to you, but we should probably move docs for these methods to the header as propert jsdocs >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >--- a/toolkit/components/places/src/History.cpp >+++ b/toolkit/components/places/src/History.cpp >@@ -34,39 +34,118 @@ > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "History.h" > #include "nsNavHistory.h" >+#include "Helpers.h" > > #include "mozilla/storage.h" > #include "mozilla/dom/Link.h" > #include "nsDocShellCID.h" > #include "nsIEventStateManager.h" >+#include "nsNavBookmarks.h" nit: i prefer headers to be grouped by some kind of logic, nsNavBookmarks.h should probably go near nsNavHistory.h >+#define URI_VISIT_SAVED "uri-visit-saved" what is this for? there is no comment explaining it, add one please >+// Microsecond timeout for "recent" events such as typed and bookmark following. >+// If you typed it more than this time ago, it's not recent. >+// This is 15 minutes m s/m us/s >+#define RECENT_EVENT_THRESHOLD ((PRInt64)15 * 60 * PR_USEC_PER_SEC) if this is shared with history (i seem to recall we have it) should probably be shared through nsNavHistory.h > namespace { > >+//////////////////////////////////////////////////////////////////////////////// >+// Step >+// >+class Step : public mozIStorageStatementCallback >+{ >+public: >+ NS_DECL_ISUPPORTS >+ >+ virtual ~Step() {} the destructor looks unused so far, can we just rely on the default and avoid definition? >+ // Executes statement asynchronously using this as a callback. >+ NS_IMETHOD ExecuteAsync(mozIStorageStatement *stmt); >+ >+ // Called once after query is completed. If your query has more than one >+ // result set to process, you will want to override HandleResult instead. >+ NS_IMETHOD Callback(mozIStorageResultSet *aResultSet); Not sure why this can't be called always on handleCompletion, is just useless to do so? >+ // Called when a SQL error occurs. >+ NS_IMETHOD HandleError(mozIStorageError *aError); you should probably inherit from AsyncStatementCallback that is just like mozIStorageStatementCallback but has a predefined HandleError Use javadocs for methods please >+ // These fire Callback with results, if there are any. >+ // HandleResult is not called if there are no rows from a >+ // select query. This is unfortunate. To make things >+ // easier, Callback is always called. Uh and this is the opposite as what is written above, or just bad explained (or just i don't understand English). One of these. >+ NS_IMETHOD HandleResult(mozIStorageResultSet *aResultSet); >+ NS_IMETHOD HandleCompletion(PRUint16 aReason); >+ >+private: >+ // Used by HandleResult to cache results until HandleComplete is called. HandleCompletion, not HandleComplete >+ nsCOMPtr<mozIStorageResultSet> mResultSet; hm, are you aware that HandleResult can return more rows every time it is called? Well maybe the resultSet could be the same, but i'm not sure. So, i'm unsure you can cache it till you get completion, please clarify with Shawn. >+NS_IMETHODIMP Step::ExecuteAsync(mozIStorageStatement *stmt) >+{ >+ mozStorageStatementScoper scoper(stmt); >+ nsCOMPtr<mozIStoragePendingStatement> handle; >+ nsresult rv = stmt->ExecuteAsync(this, getter_AddRefs(handle)); >+ scoper.Abandon(); >+ return rv; please NS_ENSURE_SUCCESS and return NS_OK >+namespace { >+ >+struct VisitURIData { >+ nsCOMPtr<mozIStorageConnection> dbConn; hm, what's the purpose of storing dbConn here? we can only have one connection, and nobody can create a new one. So, does not look like a param of a visit >+ nsCAutoString url; >+ nsCOMPtr<nsIURI> uri; perf sensitive choice? you can get spec out of uri, or viceversa, depending on what you need more often >+ nsCAutoString lastUrl; last? what's the context of "last"? >+ nsNavHistory* history = nsNavHistory::GetHistoryService(); >+ if (!history) >+ return NS_ERROR_FAILURE; >+ >+ // Update frecency (*after* the visit info is in the db) >+ // Swallow errors here, since if we've gotten this far, it's more >+ // important to notify the observers below. >+ nsNavBookmarks *bs = nsNavBookmarks::GetBookmarksService(); >+ if (!bs) >+ return NS_ERROR_FAILURE; in these cases we usually NS_ENSURE_TRUE (bs, NS_ERROR_OUT_OF_MEMORY); not sure what's the common style inside this file not going to repeat the comment below, just be consistent with the first one >+ (void)history->UpdateFrecency(mData->placeId, bs->IsRealBookmark(mData->placeId)); reason to ignore errors? >+class AddVisitStep : public VisitURIBasicStep { >+public: >+ NS_DECL_ISUPPORTS_INHERITED >+ >+ AddVisitStep(nsAutoPtr<VisitURIData>& aData) >+ { >+ mData = aData; >+ } >+ >+ NS_IMETHOD Callback(mozIStorageResultSet *aResultSet) >+ { >+ nsresult rv = NS_OK; >+ nsCOMPtr<mozIStorageStatement> stmt; this looks used much much later, define near usage please >+ // XXX need to figure out story for new session IDs that isn't synchronous actually we could even stop supporting session IDs if it's going to be hard... actually i'd prefer having a redirects/session that tracks all visits in a redirects chain. But this is probably OT here. >+ if (aResultSet) { >+ // Just got last visit information >+ nsCOMPtr<mozIStorageRow> row; >+ rv = aResultSet->GetNextRow(getter_AddRefs(row)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ PRInt64 possibleSessionId; >+ PRTime lastVisitOfSession; define near usage please >+ if (mData->dateTime - lastVisitOfSession <= RECENT_EVENT_THRESHOLD) { >+ mData->sessionId = possibleSessionId; >+ } else { >+ mData->sessionId = history->GetNewSessionID(); >+ } >+ } else { i'd say Places code style does not want cuddled braces, but Shawn would hit me, thus, do whatever you want :p >+ nsCOMPtr<Step> step = new UpdateFrecencyStep(mData); >+ rv = step->ExecuteAsync(stmt); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ return rv; return NS_OK, the previous NS_ENSURE_SUCCESS will already return rv if it's not NS_OK >+ rv = mData->dbConn->CreateStatement(NS_LITERAL_CSTRING( >+ "SELECT id, session, visit_date FROM moz_historyvisits WHERE place_id=( " WHERE on new line please, spaces around "=" Select first from TEMP table, later from disk table >+/** >+ * Step 1a Find the ID of a recently inserted place >+ */ make comments a bit more verbose, like adding why you do that, and when you'll use the data you get, and whatever. >+class FindNewIdStep : public VisitURIBasicStep { >+public: >+ NS_DECL_ISUPPORTS_INHERITED >+ >+ FindNewIdStep(nsAutoPtr<VisitURIData>& aData) >+ { >+ mData = aData; if possible assignments should be done as : mData(aData) since in case of memory failure this will prevent it from creating a not working object >+ >+} // end anonymous namespace for VisitURI the comment is a bit unclear, VisitURI is after it, i suppose it's for "VisitURI Helpers"? >+ // >+ // Populate data structure that will be used in our async SQL steps. >+ // >+ nsAutoPtr<VisitURIData> data(new VisitURIData()); we don't believe three single lines comment style i think :) >+ rv = data->dbConn->CreateStatement(NS_LITERAL_CSTRING( >+ "SELECT id, hidden, typed FROM moz_places WHERE url=?1 " >+ "UNION ALL " >+ "SELECT id, hidden, typed FROM moz_places_temp WHERE url=?1 " >+ "LIMIT 1" >+ ), getter_AddRefs(stmt)); >+ NS_ENSURE_SUCCESS(rv, rv); select from temp first where on a new line >+ nsCOMPtr<nsIObserverService> obsService = >+ do_GetService("@mozilla.org/observer-service;1"); >+ if (obsService) { >+ obsService->NotifyObservers(aURI, NS_LINK_VISITED_EVENT_TOPIC, nsnull); >+ } So, looks like we don't fire the notification if canAdd is false or something other fails? this looks maybe different from previous behavior?

Comment on attachment 439094 [details] [diff] [review] WIP v1 > /** >+ * Returns any recent activity done with a URL. >+ */ move javadocs to .h >+PRUint32 >+nsNavHistory::GetRecentFlags(nsIURI *aURI) >+{ >+ PRUint32 result = 0; >+ nsCAutoString spec; >+ nsresult rv = aURI->GetSpec(spec); >+ >+ if (rv == NS_OK) { >+ if (CheckIsRecentEvent(&mRecentTyped, spec)) { >+ result |= RECENT_TYPED; >+ } >+ if (CheckIsRecentEvent(&mRecentLink, spec)) { >+ result |= RECENT_ACTIVATED; >+ } >+ if (CheckIsRecentEvent(&mRecentBookmark, spec)) { >+ result |= RECENT_BOOKMARKED; >+ } >+ } >+ >+ return result; >+} yeah would be really cool in future to just have a single hash with this flag set for each uri. pretty cool (followup though) build like if this would be true :) >@@ -1844,16 +1861,19 @@ nsNavHistory::GetUrlIdFor(nsIURI* aURI, > > > // nsNavHistory::InternalAddNewPage > // > // Adds a new page to the DB. > // THIS SHOULD BE THE ONLY PLACE NEW moz_places ROWS ARE > // CREATED. This allows us to maintain better consistency. > // >+// XXX this functionality is being move to History.cpp, so >+// in fact there *are* two places where new pages are added. typo: "being move" >diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h >+struct RecentEventResult { >+ PRBool typed; // User typed in URL recently >+ PRBool activated; // User tapped URL link recently >+ PRBool bookmarked; // User bookmarked URL recently >+ PRBool redirected; // Was redirected to URL recently >+}; i like "activated", maybe even more than followed, you should have suggested that earlier ;) ok, first pass is fine so far, i'll look deeper at code paths in next version

Comment on attachment 439094 [details] [diff] [review] WIP v1 I'll take a look after mak's review comments are addressed.

> the destructor looks unused so far, can we just rely on the default and avoid > definition? Discussed in IRC. Base destructor needs to be virtual, otherwise member COM ptr will not be destructed when deleting something with base pointer (since refcount decrement is implemented in base class, delete would be called in Step). > Not sure why this can't be called always on handleCompletion, is just useless > to do so? I tried to comment this section better. The whole point of Callback is so derived classes don't have to implement two different methods, checking to see if there were any results inside of completion function. Callback is called exactly once, no matter what. > hm, are you aware that HandleResult can return more rows every time it is > called? Well maybe the resultSet could be the same, but i'm not sure. > So, i'm unsure you can cache it till you get completion, please clarify with > Shawn. Yes. Comments explicitly state if you need to handle multiple results you have to override HandleResult. I tried to clarify this in the new patch. > reason to ignore errors? Making sure observer is fired below. New patch addresses this. > actually we could even stop supporting session IDs if it's going to be hard... > actually i'd prefer having a redirects/session that tracks all visits in a > redirects chain. We discussed on IRC that it would probably be best to emulate current behavior? Any update on what that behavior exactly is? > if possible assignments should be done as > : mData(aData) > since in case of memory failure this will prevent it from creating a not > working object I got compiler errors about mData not being a member. C++ is lame. > So, looks like we don't fire the notification if canAdd is false or something > other fails? this looks maybe different from previous behavior? The new behavior is the same as the previous behavior. If there are any errors along the way or canAdd is false, then the message is not sent. --- In the new patch, the important change is in doc shell. Doc shell now sends visits in proper order (if there are redirects, referrer->redirect, redirect->final destination). I'm worried about race conditions with queries now. There are a few bad things that can happen: * a visit count isn't added * the hidden field goes from shown to hidden * the typed field goes from typed to not typed * session history is lost I'll have to chew on this more.

[23:28] <mak> stechz: so, the basic idea is that if we don't have a referrer visit (that means practically if we don't have a value to store in from_visit) then we need a new session id. [23:29] <mak> that can happen if docshell does not pass a referrer/origin... but in the old code could be we have a visit with a referrer, but we never saw a visit for the referrer uri. in such a case we add the referrer visit 1usec (or 1msec, just before) before the first visit in the chain, and we give the chain a new session id. [23:30] <mak> so, finally, if the chain cannot be connected to an original visit that we already know, this is a new session. [23:31] <mak> practically, what the old code does is go back in the chain till it can't find a visit, that's the point where a session starts. [23:34] <mak> also, if the referring visit is too old (current time is 15mins iirc) we consider the new chain a new session, even if we have a referrer.

Now race-condition friendly, removes compiler warnings, and checks for the same URL.

Comment on attachment 441151 [details] [diff] [review] Patch v3 >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp >+ nsresult rv; you can define this later, on first use. >+ PRUint32 visitURIFlags = 0; >+ if (IsFrame()) >+ visitURIFlags &= mozilla::IHistory::TOP_LEVEL; >+ if (aRedirectFlags & nsIChannelEventSink::REDIRECT_TEMPORARY) newline between these, otherwise looks like a typo (someone will guess "maybe this should be an else if") the old code was skipping REDIRECT_INTERNAL visits, you should probably do the same here, no reason to record those visits (but sorry, i don't know how you can simulate one to check that you don't break a chain, ask soneone that better knows nsIChannel impl, please) http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#5573 >+ nsCOMPtr<nsIURI> oldURI, newURI; >+ rv = aOldChannel->GetURI(getter_AddRefs(oldURI)); >+ rv = aNewChannel->GetURI(getter_AddRefs(newURI)); >+ if (!oldURI || !newURI) >+ return; you are not checking rv for these calls >+ mozilla::IHistory* history = nsContentUtils::GetHistory(); >+ if (history) { >+ nsCOMPtr<nsIURI> referrer; >+ NS_GetReferrerFromChannel(aOldChannel, getter_AddRefs(referrer)); check rv? >+ if (!mVisitedReferrer) { >+ history->VisitURI(oldURI, NULL, visitURIFlags, referrer); >+ mVisitedReferrer = PR_TRUE; Add a comment about what you're doing here, will help readers VisitURI returns an nsresult, isn't it? So why don't we check it? >+ } >+ >+ history->VisitURI(newURI, NULL, visitURIFlags, oldURI); ditto > nsresult > nsDocShell::AddToGlobalHistory(nsIURI * aURI, PRBool aRedirect, > nsIURI * aReferrer) > { >+ if (!mVisitedReferrer) { the name of the property does not help understanding what happens at an high level, add a comment please. I was expecting this to be called after redirects, so i'm a bit unsure why we don't add the visit, is it because we already added it in the redirects management? if so the property name is completely unclear since it's about a referrer... >+ mozilla::IHistory* history = nsContentUtils::GetHistory(); >+ NS_ENSURE_STATE(history); >+ nsresult rv = history->VisitURI(aURI, referrer, >+ (IsFrame() ? 0: mozilla::IHistory::TOP_LEVEL), >+ redirect); fancy indentation >diff --git a/toolkit/components/places/src/Helpers.cpp b/toolkit/components/places/src/Helpers.cpp you probably have some bitrotting here, sorry. >+nsresult >+GetReversedHostname(nsIURI* aURI, nsAString& aRevHost) >+{ >+ nsCString forward8; why not using an nsCAutoString? >+ nsresult rv = aURI->GetHost(forward8); >+ if (NS_FAILED(rv)) { >+ return rv; >+ } NS_ENSURE_SUCCESS? or do you expect this failure to be common and thus you don't want to warn about it? >+ // can't do reversing in UTF8, better use 16-bit chars >+ NS_ConvertUTF8toUTF16 forward(forward8); >+ GetReversedHostname(forward, aRevHost); i guess you can directly pass NS_ConvertUTF8toUTF16(forward8) to getreversedHostName >+void >+ReverseString(const nsString& aInput, nsAString& aReversed) >+{ >+ aReversed.Truncate(0); >+ for (PRInt32 i = aInput.Length() - 1; i >= 0; i--) >+ aReversed.Append(aInput[i]); brace loops please nit: this could probably increase aReversed size at each iteration, i guess if calling SetLength on it and then directly writing values would be faster. >diff --git a/toolkit/components/places/src/Helpers.h b/toolkit/components/places/src/Helpers.h bitrotting here too, sorry. > #include "mozIStorageStatementCallback.h" >+#include "nsIURI.h" >+#include "nsString.h" this now includes nsNavHistory.h and storage.h, that could make these includes redundant > /** >+ * This extracts the hostname from the URI and reverses it in the >+ * form that we use (always ending with a "."). So >+ * "http://microsoft.com/" becomes "moc.tfosorcim." >+ * >+ * The idea behind this is that we can create an index over the items in >+ * the reversed host name column, and then query for as much or as little >+ * of the host name as we feel like. >+ * >+ * For example, the query "host >= 'gro.allizom.' AND host < 'gro.allizom/' >+ * Matches all host names ending in '.mozilla.org', including >+ * 'developer.mozilla.org' and just 'mozilla.org' (since we define all >+ * reversed host names to end in a period, even 'mozilla.org' matches). >+ * The important thing is that this operation uses the index. Any substring >+ * calls in a select statement (even if it's for the beginning of a string) >+ * will bypass any indices and will be slow). >+ */ >+nsresult GetReversedHostname(nsIURI* aURI, nsAString& aRevHost); >+ >+/** >+ * Similar method to GetReversedHostName but for strings >+ */ >+void GetReversedHostname(const nsString& aForward, nsAString& aRevHost); >+ >+void ReverseString(const nsString& aInput, nsAString& aReversed); missing javadoc also missing @param above >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >--- a/toolkit/components/places/src/History.cpp >+++ b/toolkit/components/places/src/History.cpp >@@ -33,40 +33,131 @@ > * decision by deleting the provisions above and replace them with the notice > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "History.h" >-#include "nsNavHistory.h" >+#include "Helpers.h" > > #include "mozilla/storage.h" > #include "mozilla/dom/Link.h" > #include "nsDocShellCID.h" > #include "nsIEventStateManager.h" >+#include "nsNavBookmarks.h" >+#include "nsNavHistory.h" These 2 are fine above with other Places headers, if i said the opposite i should have been drunk. >+// Observer event that's fired after a visit has been registered in the DB. nit: "that's" looks redundant from the height of my english ignorance >+struct VisitURIData >+{ >+ PRInt64 placeId; >+ PRInt32 hidden; >+ PRInt32 typed; >+ nsCOMPtr<nsIURI> uri; >+ >+ // URL of last visit in chain >+ nsCAutoString lastUrl; nit: // Url of the last visit in chain. what do you mean by "last"? which side? last in time? So maybe "last added visit"? >+ PRInt32 transitionType; >+ PRInt64 lastVisitId; is this related to lastUrl (they should be visually grouped then) >+ /** >+ * By default, stores the last result set received. where? (i guess in mResultSet, but should be specified here) > History::History() > { > NS_ASSERTION(!gService, "Ruh-roh! This service has already been created!"); > gService = this; >+ >+ nsCOMPtr<nsIObserverService> observerService = >+ do_GetService(NS_OBSERVERSERVICE_CONTRACTID); you can use mozilla::services::GetObserverService(); >+ NS_ASSERTION(observerService, "Could not get observer service."); >+ nsresult rv = observerService->AddObserver(this, NS_ASSERTION is a no-op in opt builds, thus the next try to use observerService will crash on error. You should: if (observerService) AddObserver >+namespace { >+ >+/** >+ * Has basic error handler for visiting URIs. >+ * Each step passing along data >+ */ >+class VisitURIBasicStep : public Step >+{ >+public: >+ NS_DECL_ISUPPORTS_INHERITED >+ >+ NS_IMETHOD HandleError(mozIStorageError *aError) >+ { >+ NS_WARNING("VisitURI SQL query failed"); >+ return NS_OK; >+ } Why are you not inheriting your steps from AsyncStatementCallback that has already a predefined handleError impl? >+protected: >+ VisitURIData* mData; >+}; >+NS_IMPL_ISUPPORTS_INHERITED0( >+ VisitURIBasicStep >+, Step >+) wrong indentation >+ >+/** >+ * STEP 4: Update frecency of URI and notify observers. >+ */ >+class UpdateFrecencyStep : public VisitURIBasicStep { Then the step should be called UpdateFrecencyAndNotify >+public: >+ NS_DECL_ISUPPORTS_INHERITED >+ >+ UpdateFrecencyStep(VisitURIData* aData) >+ { >+ mData = aData; >+ } >+ >+ NS_IMETHOD Callback(mozIStorageResultSet *aResultSet) >+ { >+ // XXX need to figure out story for not synchronous frecency updating >+ // (bug 556631) I expect us to save a fake value in the page entry (-1 for example) and then call UpdateFrecency, that will be completely async and will update the entry lazily. It won't even need to be implemented here frecency updating is something we can do at any time. >+ // Swallow errors here, since if we've gotten this far, it's more >+ // important to notify the observers below. I agree, but WARN_IF_FALSE them please, we want to be notified, even if not interrupted. >+ nsNavHistory* history = nsNavHistory::GetHistoryService(); >+ nsNavBookmarks *bs = nsNavBookmarks::GetBookmarksService(); not consistent pointers style (go toward type please) >+ if (history && bs) { >+ // Update frecency (*after* the visit info is in the db) >+ (void)history->UpdateFrecency(mData->placeId, >+ bs->IsRealBookmark(mData->placeId)); OT related to async frecency: it's absolutely crazy we have to tell updateFrecency if this is a bookmark, if we pass null it shouldn figure it out by itself. >+ nsCOMPtr<nsIObserverService> obsService = >+ do_GetService("@mozilla.org/observer-service;1"); mozilla::services:: etc >+/** >+ * STEP 3: Add visit to moz_history_visits table. >+ */ hm, do we calculate frecency before adding the visit? >+ NS_IMETHOD Callback(mozIStorageResultSet *aResultSet) >+ { >+ nsresult rv; >+ >+ nsNavHistory* history = nsNavHistory::GetHistoryService(); >+ NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY); >+ >+ // XXX need to figure out story for new session IDs that isn't synchronous >+ // (bug 561450) nit: replace XXX with nice TODO: ? >+ >+ if (mData->dateTime - lastVisitOfSession <= RECENT_EVENT_THRESHOLD) { >+ mData->sessionId = possibleSessionId; >+ } else { >+ mData->sessionId = history->GetNewSessionID(); >+ mData->lastVisitId = 0; >+ } >+ } else { >+ mData->sessionId = history->GetNewSessionID(); >+ mData->lastVisitId = 0; >+ } nice comments make newcomers happier >+ // Add visit to database While useless comments make them shrug :p >+ nsCOMPtr<mozIStorageStatement> stmt; >+ rv = history->GetStorageConnection()->CreateStatement(NS_LITERAL_CSTRING( >+ "INSERT INTO moz_historyvisits_view " >+ "(from_visit, place_id, visit_date, visit_type, session)" >+ "VALUES(?1, ?2, ?3, ?4, ?5)" >+ ), getter_AddRefs(stmt)); >+ NS_ENSURE_SUCCESS(rv, rv); does this differ from the insert statement that nsNavHistory has? if not i think you can history->getStatementById and add an id for it. >+ if (!mData->lastUrl.IsEmpty()) { what is this block going to do? we need comments ffrom an high perspective view. >+ // Find last visit ID and session ID using lastUrl since we need them to... >+ nsNavHistory* history = nsNavHistory::GetHistoryService(); >+ NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY); >+ nsCOMPtr<mozIStorageStatement> stmt; >+ rv = history->GetStorageConnection()->CreateStatement(NS_LITERAL_CSTRING( >+ "SELECT id, session, visit_date FROM moz_historyvisits_temp " >+ "WHERE place_id = ( " >+ "SELECT id FROM moz_places_temp WHERE url = ?1 " >+ "UNION ALL " >+ "SELECT id FROM moz_places WHERE url = ?1 " >+ "LIMIT 1 " >+ ") UNION ALL " >+ "SELECT id, session, visit_date FROM moz_historyvisits " >+ "WHERE place_id = (" >+ "SELECT id FROM moz_places_temp WHERE url = ?1 " >+ "UNION ALL " >+ "SELECT id FROM moz_places WHERE url = ?1 " >+ "LIMIT 1 " >+ ") ORDER BY visit_date DESC " >+ "LIMIT 1" >+ ), getter_AddRefs(stmt)); >+ NS_ENSURE_SUCCESS(rv, rv); I don't recall if history has a query like this one, but sounds possible, check please and eventually use it through getStatementById. >+ rv = BindStatementURLCString(stmt, 0, mData->lastUrl); Use the new URIBinder::Bind() that is in Helpers.h >+ } >+ else { somewhere you use the cuddled } else { somwhere not, please check file consistency. >+ // Empty lastUrl. >+ // Not part of a session. Just run next step's callback with no results. since... >+ nsCAutoString url; >+ rv = mData->uri->GetSpec(url); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = BindStatementURLCString(stmt, 0, url); >+ NS_ENSURE_SUCCESS(rv, rv); no need to getSpec, URIBinder::Bind(stmt, 0, mData->uri); >+ // Update moz_places row >+ >+ // Note: trigger updates visit_count. nit: that newline looks out of place >+ rv = history->GetStorageConnection()->CreateStatement(NS_LITERAL_CSTRING( >+ "UPDATE moz_places_view SET typed = ?1, hidden = ?2 WHERE id = ?3" >+ ), getter_AddRefs(stmt)); >+ NS_ENSURE_SUCCESS(rv, rv); You can probably modify and reuse an existing statement here as well >+ rv = history->GetStorageConnection()->CreateStatement(NS_LITERAL_CSTRING( >+ "INSERT INTO moz_places_view (url, rev_host, typed, hidden, frecency)" >+ "VALUES(?1, ?2, ?3, ?4, ?5)" >+ ), getter_AddRefs(stmt)); >+ NS_ENSURE_SUCCESS(rv, rv); ditto >+ nsCAutoString url; >+ rv = mData->uri->GetSpec(url); >+ NS_ENSURE_SUCCESS(rv, rv); >+ nsAutoString url16; >+ CopyUTF8toUTF16(url, url16); >+ nsAutoString revHost; >+ GetReversedHostname(url16, revHost); i think we have a method to get rev_host directly from an nsIURI >+ rv = BindStatementURLCString(stmt, 0, url); >+ NS_ENSURE_SUCCESS(rv, rv); Use the URIBinder, as well >+#define STARTVISIT_CHECK(stmt) \ >+ if (!stmt) { \ >+ NS_WARNING("Could not save visit"); \ >+ return; \ >+ } >+ >+} // end anonymous namespace for VisitURI helpers hm, this macro doesn't look really useful >+void History::StartNextVisitURI() >+{ >+ VisitURIData* data = (VisitURIData*) mPendingVisits.PeekFront(); >+ if (!data) >+ return; >+ trailing spaces is the !data an expected condition? if not you should warning at least >+ nsCAutoString url; >+ nsresult rv = data->uri->GetSpec(url); >+ STARTVISIT_CHECK(rv == NS_OK); You don't need it :) >+ // Find existing entry in moz_places table, if any. >+ nsCOMPtr<mozIStorageStatement> stmt; >+ rv = history->GetStorageConnection()->CreateStatement(NS_LITERAL_CSTRING( >+ "SELECT id, hidden, typed FROM moz_places_temp WHERE url = ?1 " >+ "UNION ALL " >+ "SELECT id, hidden, typed FROM moz_places WHERE url = ?1 " >+ "LIMIT 1" >+ ), getter_AddRefs(stmt)); >+ STARTVISIT_CHECK(rv == NS_OK); We should have one stmt you can reuse, for sure. rv == NS_OK is wrong, you should always use NS_SUCCEEDED(rv) and i don't like the macro :) >+ rv = BindStatementURLCString(stmt, 0, url); >+ STARTVISIT_CHECK(rv == NS_OK); URIBinder and wrong check >+ nsCOMPtr<Step> step = new CheckExistingStep(data); >+ rv = step->ExecuteAsync(stmt); >+ STARTVISIT_CHECK(rv == NS_OK); wrong check >+NS_IMETHODIMP >+History::VisitURI(nsIURI* aURI, >+ nsIURI* aReferrer, >+ PRUint32 aFlags, >+ nsIURI* aRedirectFrom) >+{ >+ NS_ASSERTION(aURI, "Must pass a non-null URI!"); fine, but this should probably also NS_ENSURE_ARG and NS_ENSURE_ARG_POINTER the params it's expecting non null NS_ASSERTION is debug only indeed. >+ PRUint32 recentFlags = history->GetRecentFlags(aURI); >+ if (aFlags & REDIRECT_TEMPORARY) { >+ data->transitionType = nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY; >+ } else if (aFlags & REDIRECT_PERMANENT) { >+ data->transitionType = nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT; >+ } else if (recentFlags & nsNavHistory::RECENT_TYPED) { >+ data->transitionType = nsINavHistoryService::TRANSITION_TYPED; >+ } else if (recentFlags & nsNavHistory::RECENT_BOOKMARKED) { >+ data->transitionType = nsINavHistoryService::TRANSITION_BOOKMARK; >+ } else if (aFlags & IHistory::TOP_LEVEL) { >+ // User was redirected or link was clicked in the main window. >+ data->transitionType = nsINavHistoryService::TRANSITION_LINK; >+ } else if (recentFlags & nsNavHistory::RECENT_ACTIVATED) { >+ // User activated a link in a frame. >+ data->transitionType = nsINavHistoryService::TRANSITION_FRAMED_LINK; >+ } else { >+ // A frame redirected to a new site without user interaction. >+ data->transitionType = nsINavHistoryService::TRANSITION_EMBED; >+ } This is why i hate cuddled else/else if, the code is so dense that i'm not even willing to read it. tri state would not be better to understand what's under this, I feel like uncuddling else would make this better, others don't. I guess it's up to you :) >+ data->typed = (data->transitionType == nsINavHistoryService::TRANSITION_TYPED) ? 1 : 0; >+ data->hidden = >+ (data->transitionType == nsINavHistoryService::TRANSITION_FRAMED_LINK || >+ data->transitionType == nsINavHistoryService::TRANSITION_EMBED || >+ (aRedirectFrom && data->transitionType != nsINavHistoryService::TRANSITION_TYPED)) ? 1 : 0; Use some local bool var here to short the code a bit, for example the last condition could go to a temp bool with a meaningful name. I'm fine with having typed and hidden ints since in future we could extend them from simple bools >+ nsCOMPtr<nsIObserverService> obsService = >+ do_GetService("@mozilla.org/observer-service;1"); use mozilla::services > //////////////////////////////////////////////////////////////////////////////// >+//// IObserver nsIObserver maybe? >+ >+NS_IMETHODIMP >+History::Observe(nsISupports* aSubject, >+ const char* aTopic, >+ const PRUnichar* aData) >+{ >+ VisitURIData* data = (VisitURIData*) mPendingVisits.PeekFront(); >+ nsresult rv; >+ nsCOMPtr<nsIURI> uri = do_QueryInterface(aSubject, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); you can NS_ENSURE_STATE(uri) and don't pass rv i think? >+//////////////////////////////////////////////////////////////////////////////// > //// nsISupports > > NS_IMPL_ISUPPORTS1( > History, > IHistory So, it's also implementing nsIObserver >diff --git a/toolkit/components/places/src/History.h b/toolkit/components/places/src/History.h >+ /** >+ * Since visits rapidly fire at once, it's very likely to have race >+ * conditions for SQL queries. We often need to see if a row exists >+ * or peek at values, and by the time we have retrieved them they could >+ * be different. >+ * >+ * We guarantee an ordering of our SQL statements Storage guarantees that async queries are run in order they are executed, it's a pool of statements. >diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp > /** >+ * Returns any recent activity done with a URL. >+ */ move doc to the header and explain the return value. >+PRUint32 >+nsNavHistory::GetRecentFlags(nsIURI *aURI) >+{ >+ PRUint32 result = 0; >+ nsCAutoString spec; >+ nsresult rv = aURI->GetSpec(spec); >+ >+ if (rv == NS_OK) { NS_SUCCEEDED(rv) but actually, could we ever save an invalid uri to the hash? looks like this could just be NS_ENSURE_SUCCESS >+ if (CheckIsRecentEvent(&mRecentTyped, spec)) { >+ result |= RECENT_TYPED; >+ } >+ if (CheckIsRecentEvent(&mRecentLink, spec)) { >+ result |= RECENT_ACTIVATED; >+ } >+ if (CheckIsRecentEvent(&mRecentBookmark, spec)) { >+ result |= RECENT_BOOKMARKED; >+ } feel free to drop braces here >diff --git a/toolkit/components/places/tests/cpp/places_test_harness.h b/toolkit/components/places/tests/cpp/places_test_harness.h >+#include "mozIStorageConnection.h" >+#include "mozIStorageStatement.h" i think you can just include mozilla/storage.h >+ do_check_true(rv == NS_OK); >+ return dbConn; you know, i won't repeat it for next code :)

Comments addressed and un-bitrotted.

Comment on attachment 443672 [details] [diff] [review] Patch v4 diff --git a/docshell/base/IHistory.h b/docshell/base/IHistory.h + enum VisitFlags { + /** + * Whether the URI was loaded in a top-level window. + */ + TOP_LEVEL = 1 << 1, + /** + * Whether the URI was loaded as part of a permanent redirect. + */ + REDIRECT_PERMANENT = 1 << 2, + /** + * Whether the URI was loaded as part of a temporary redirect. + */ + REDIRECT_TEMPORARY = 1 << 3 + }; reasons to skip 1 << 0? + mozilla::dom::Link *aContent); \ + NS_IMETHOD VisitURI(nsIURI *aURI, nsIURI *aReferrer, \ + PRUint32 aFlags, \ + nsIURI *aRedirectedFrom = NULL); nit: either each one on its own line, or no need to newline for aRedirectedFrom, limit is still 80 chars and it fits. diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp nsresult nsDocShell::AddToGlobalHistory(nsIURI * aURI, PRBool aRedirect, nsIURI * aReferrer) { - if (mItemType != typeContent || !mGlobalHistory) - return NS_OK; - - PRBool visited; - nsresult rv = mGlobalHistory->IsVisited(aURI, &visited); - if (NS_FAILED(rv)) - return rv; - - rv = mGlobalHistory->AddURI(aURI, aRedirect, !IsFrame(), aReferrer); - if (NS_FAILED(rv)) - return rv; We should probably publicly advertise this change, since other docshell history implementations not using Places are going to break here. We could retain both paths for compatibility (we would just use one and ignore the other) but since the added complexity I'd say you should bring the change to public (blog AND newgroup) and collect feedback, I don't know which globalhistory users could be around, I don't suspect many, but since we are all set for transparency, we should say we are going to do this change and get eventual concerns. This is less important for the GH3 changes since GH3 has been mostly added at Places time + if (mItemType != typeContent) + return NS_OK; Is this somehow handled for redirect? Should it be handled? And if so, is it the information known at that time? diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h --- a/docshell/base/nsDocShell.h +++ b/docshell/base/nsDocShell.h @@ -674,16 +674,19 @@ protected: nsCOMPtr<nsICommandManager> mCommandManager; // Reference to the SHEntry for this docshell until the page is destroyed. // Somebody give me better name nsCOMPtr<nsISHEntry> mOSHE; // Reference to the SHEntry for this docshell until the page is loaded // Somebody give me better name nsCOMPtr<nsISHEntry> mLSHE; + // Tracks whether the referrer was called with VisitURI + PRBool mCalledVisitURIForReferrer; I don't recall seeing initialization of this, you should init it to false in the constructor. Also, since this is for internal usage, you can just use bool here. diff --git a/toolkit/components/places/src/Helpers.cpp b/toolkit/components/places/src/Helpers.cpp +nsresult +GetReversedHostname(nsIURI* aURI, nsAString& aRevHost) +{ + nsCAutoString forward8; + nsresult rv = aURI->GetHost(forward8); + NS_ENSURE_SUCCESS(rv, rv); + + // can't do reversing in UTF8, better use 16-bit chars + NS_ConvertUTF8toUTF16 forward(forward8); + GetReversedHostname(forward, aRevHost); You can probably inline NS_ConvertUTF8toUTF16(forward8) in GetReversedHostname +void +ReverseString(const nsString& aInput, nsAString& aReversed) +{ + aReversed.Truncate(0); + for (PRInt32 i = aInput.Length() - 1; i >= 0; i--) + aReversed.Append(aInput[i]); while here brace this loop please diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp +struct VisitURIData +{ + PRInt64 placeId; + PRInt32 hidden; + PRInt32 typed; + nsCOMPtr<nsIURI> uri; + + // Url of last added visit in chain. + nsCAutoString lastUrl; usually in classes the common suggestion is to avoid AutoStrings for members, this is a struct but the same suggestion could be valid, I'd go for nsCString here + nsCOMPtr<nsIObserverService> observerService = + mozilla::services::GetObserverService(); + if (observerService) + (void)observerService->RemoveObserver(this, + URI_VISIT_SAVED); nit: looks like this fits the 80 chars limit, why newline? + NS_IMETHOD Callback(mozIStorageResultSet* aResultSet) + { + // TODO need to figure out story for not synchronous frecency updating + // (bug 556631) + + // Swallow errors here, since if we've gotten this far, it's more + // important to notify the observers below. + nsNavHistory* history = nsNavHistory::GetHistoryService(); + nsNavBookmarks* bs = nsNavBookmarks::GetBookmarksService(); while bs is a nice shortcut in js, I'd prefer if you used "bookmarks" here since it's more common in our cpp code. + NS_WARN_IF_FALSE(history, "Could not get history service"); + NS_WARN_IF_FALSE(bs, "Could not get bookmarks service"); nit: interleave with getters (get, check, get, check). + if (history && bs) { + // Update frecency (*after* the visit info is in the db) nit: the parethesis part looks important enough to deserve its own phrase. So, period and a new phrase please. + nsresult rv = history->UpdateFrecency(mData->placeId, + bs->IsRealBookmark(mData->placeId)); indentation, you can eventually: nsresult rv = history->UpdateFrecency( mData->placeId, bs->IsRealBookmark(mData->placeId) ); + nsCOMPtr<nsIObserverService> obsService = + do_GetService("@mozilla.org/observer-service;1"); mozilla::services + NS_IMETHOD Callback(mozIStorageResultSet* aResultSet) + { + if (aResultSet) { + // Just got last visit information Better comment please. + nsCOMPtr<mozIStorageStatement> stmt = + history->GetStatementById(DB_INSERT_VISIT); You should absolutely NS_ENSURE_STATE(stmt), if we are shutting down you could get null + rv = stmt->BindInt64ByIndex(0, mData->lastVisitId); + NS_ENSURE_SUCCESS(rv, rv); + rv = stmt->BindInt64ByIndex(1, mData->placeId); + NS_ENSURE_SUCCESS(rv, rv); + rv = stmt->BindInt64ByIndex(2, mData->dateTime); + NS_ENSURE_SUCCESS(rv, rv); + rv = stmt->BindInt32ByIndex(3, mData->transitionType); + NS_ENSURE_SUCCESS(rv, rv); + rv = stmt->BindInt64ByIndex(4, mData->sessionId); + NS_ENSURE_SUCCESS(rv, rv); DB_INSERT_VISIT binds by name + NS_IMETHOD Callback(mozIStorageResultSet* aResultSet) + { + if (!mData->lastUrl.IsEmpty()) { + // Find last visit ID and session ID using lastUrl so we can add them + // to a browsing session if the visit was recent. + nsNavHistory* history = nsNavHistory::GetHistoryService(); + NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY); + nsCOMPtr<mozIStorageStatement> stmt = + history->GetStatementById(DB_RECENT_VISIT_OF_URL); + NS_ENSURE_SUCCESS(rv, rv); this rv checking is now wrong, you should NS_ENSURE_STATE(stmt) +class FindNewIdStep : public Step +{ + NS_IMETHOD Callback(mozIStorageResultSet* aResultSet) + { + // Get newly created ID + nsNavHistory* history = nsNavHistory::GetHistoryService(); + NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY); + nsCOMPtr<mozIStorageStatement> stmt = + history->GetStatementById(DB_GET_PAGE_VISIT_STATS); You should NS_ENSURE_STATE(stmt) + nsCAutoString url; + nsresult rv = mData->uri->GetSpec(url); + NS_ENSURE_SUCCESS(rv, rv); + rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), url); + NS_ENSURE_SUCCESS(rv, rv); No reason to GetSpec, URIBinder will do that for you, just pass in mData->uri +class CheckExistingStep : public Step +{ + if (aResultSet) { + // Note: trigger updates visit_count. nit: "will update" + stmt = history->GetStatementById(DB_UPDATE_PAGE_VISIT_STATS); as said NS_ENSURE_STATE(stmt) + } else { + // No entry exists, so create one. + stmt = history->GetStatementById(DB_ADD_NEW_PAGE); as said NS_ENSURE_STATE(stmt) + nsCAutoString url; + rv = mData->uri->GetSpec(url); + NS_ENSURE_SUCCESS(rv, rv); useless call, just pass nsIURI obj to URIBinder +void History::StartNextVisitURI() + nsCAutoString url; + nsresult rv = data->uri->GetSpec(url); + if (!NS_SUCCEEDED(rv)) { + NS_WARNING("Could not get URI spec"); + return; + } ditto, useless call. + // Find existing entry in moz_places table, if any. + nsCOMPtr<mozIStorageStatement> stmt = + history->GetStatementById(DB_GET_PAGE_VISIT_STATS); ditto, NS_ENSURE_STATE(stmt); + nsCOMPtr<nsIObserverService> obsService = + do_GetService("@mozilla.org/observer-service;1"); mozilla::services +NS_IMETHODIMP +History::Observe(nsISupports* aSubject, + const char* aTopic, + const PRUnichar* aData) +{ + VisitURIData* data = (VisitURIData*) mPendingVisits.PeekFront(); + nsCOMPtr<nsIURI> uri = do_QueryInterface(aSubject); + NS_ENSURE_STATE(uri); + + if (data && uri == data->uri) { + nsAutoPtr<VisitURIData> deadDataWalking( + (VisitURIData*) mPendingVisits.PopFront()); static_cast? diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp +PRUint32 +nsNavHistory::GetRecentFlags(nsIURI *aURI) +{ + PRUint32 result = 0; + nsCAutoString spec; + nsresult rv = aURI->GetSpec(spec); WARN_IF_FALSE? otherwise this error would just be completely ignored. diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h +// Microsecond timeout for "recent" events such as typed and bookmark following. +// If you typed it more than this time ago, it's not recent. +// This is 15 minutes m s/m us/s nit: while here could you please remove this last line of comment (This is 15 mins...), it's useless and bad formatted. + enum RecentEventFlags { + RECENT_TYPED = 1<<1, // User typed in URL recently + RECENT_ACTIVATED = 1<<2, // User tapped URL link recently + RECENT_BOOKMARKED = 1<<3 // User bookmarked URL recently (hey there's a <3!) + }; some spacing there? And yeah, there's lot of <3 around! diff --git a/toolkit/components/places/tests/cpp/places_test_harness.h b/toolkit/components/places/tests/cpp/places_test_harness.h +void +getPlace(nsIURI* aURI, PlaceRecord& result) +{ + nsCOMPtr<mozIStorageConnection> dbConn = do_get_mozIStorageConnection(); + nsCOMPtr<mozIStorageStatement> stmt; + nsCString spec; + + nsresult rv = aURI->GetSpec(spec); + do_check_true(NS_SUCCEEDED(rv)); + + rv = dbConn->CreateStatement(NS_LITERAL_CSTRING( + "SELECT id, hidden, typed, visit_count FROM moz_places WHERE url=?1 " + "UNION ALL " + "SELECT id, hidden, typed, visit_count FROM moz_places_temp WHERE url=?1 " + "LIMIT 1" + ), getter_AddRefs(stmt)); + do_check_true(NS_SUCCEEDED(rv)); + + rv = stmt->BindUTF8StringParameter(0, spec); + do_check_true(NS_SUCCEEDED(rv)); Can you use the URIBinder here? diff --git a/toolkit/components/places/tests/cpp/test_IHistory.cpp b/toolkit/components/places/tests/cpp/test_IHistory.cpp +class VisitURIFinishObserver : public nsIObserver +{ +public: + NS_DECL_ISUPPORTS + + VisitURIFinishObserver(int totalVisits = 1) : + mVisits(0), + mTotalVisits(totalVisits) + { + nsCOMPtr<nsIObserverService> observerService = + do_GetService(NS_OBSERVERSERVICE_CONTRACTID); nit: moz::svcs + NS_IMETHOD Observe(nsISupports* aSubject, + const char* aTopic, + const PRUnichar* aData) + { + mVisits++; + + if (mVisits == mTotalVisits) { + nsCOMPtr<nsIObserverService> observerService = + do_GetService(NS_OBSERVERSERVICE_CONTRACTID); ditto +void +test_visituri_inserts() +{ + nsCOMPtr<IHistory> history(do_get_IHistory()); + nsCOMPtr<nsIURI> lastURI(new_test_uri()); + nsCOMPtr<nsIURI> visitedURI(new_test_uri()); + + history->VisitURI(visitedURI, lastURI, mozilla::IHistory::TOP_LEVEL, NULL); should you check history validity here? + nsCOMPtr<VisitURIFinishObserver> finisher = new VisitURIFinishObserver(); and finisher as well, also in other methods... ok these are tests but a crashing test would be bad regardless :)

Ok, my r+ does not mean this is done, just that in next iterations I'd not be able to provide really useful comments (Things are much better from first iteration). There are a couple things that should absolutely be fixed, you should always check for statements validity with NS_ENSURE_STATE(stmt) after getting them You should also bind by name in a statement where you bind by index. Things left to do: - address my comments - blog and newgroup post about changes you are going to do to docshell (notify whoever could use the old globalhistory implementation, and wait a bit for feedback/concerns) - get a second review, from sdwilsh eventually - tryserver run At this point everything should be fine, but I strongly suggest to do some manual test like: - take a selection of webpages, that also involve some redirect - visit them with a fixed list of steps (link click, bookmark, bookmark click, ecc) - repeat without and with this patch, then compare history tables. You could get help from someone in QA for this.

(In reply to comment #18) > You should also bind by name in a statement where you bind by index. When I say this I mean that if a stmt uses names, you should not bind to it by index, there is one case above where you do that iirc.

(In reply to comment #17) > reasons to skip 1 << 0? It's useful for constants to not be zero. > We should probably publicly advertise this change, since other docshell history > implementations not using Places are going to break here. > We could retain both paths for compatibility (we would just use one and ignore > the other) but since the added complexity I'd say you should > bring the change to public (blog AND newgroup) and collect feedback, I don't > know which globalhistory users could be around, I don't suspect many, but > since we are all set for transparency, we should say we are going to do this > change and get eventual concerns. bz should really look at this and make sure that he's OK with it. In fact, he should be the sr for this patch (we can do that after I review). > indentation, you can eventually: > > nsresult rv = history->UpdateFrecency( > mData->placeId, bs->IsRealBookmark(mData->placeId) > ); Don't we usually place each argument on a new line when we switch to this form? (I know that's how I do it)

(In reply to comment #18) we should make sure we have tests for HTTP redirects and that they end up in the db properly.

(In reply to comment #20) > bz should really look at this and make sure that he's OK with it. In fact, he > should be the sr for this patch (we can do that after I review). This is absolutely true, but regardless, we should advertise the change so that people interested can be aware of it. > > nsresult rv = history->UpdateFrecency( > > mData->placeId, bs->IsRealBookmark(mData->placeId) > > ); > Don't we usually place each argument on a new line when we switch to this form? It depends, not all styles are carved in stone, recently we are using a bit more the form I've posted above. I don't see the gain of going newline here.

(In reply to comment #22) > It depends, not all styles are carved in stone, recently we are using a bit > more the form I've posted above. I don't see the gain of going newline here. You don't accidentally miss an argument when reading the code.

(In reply to comment #23) > (In reply to comment #22) > > It depends, not all styles are carved in stone, recently we are using a bit > > more the form I've posted above. I don't see the gain of going newline here. > You don't accidentally miss an argument when reading the code. what? This is a really edge case. Btw, most of newer code I've seen was going this direction, included recent Storage changes (you also pointed me to that style, that I saw first from Asuth :))

> We should probably publicly advertise this change, since other docshell history > implementations not using Places are going to break here. > We could retain both paths for compatibility (we would just use one and ignore > the other) but since the added complexity I'd say you should > bring the change to public (blog AND newgroup) and collect feedback, I don't > know which globalhistory users could be around, I don't suspect many, but > since we are all set for transparency, we should say we are going to do this > change and get eventual concerns. > This is less important for the GH3 changes since GH3 has been mostly added at > Places time What will these other history implementations do? Implement IHistory? > + if (mItemType != typeContent) > + return NS_OK; > > Is this somehow handled for redirect? Should it be handled? And if so, is it > the information known at that time? I was modeling this based off the previous nsDocShell code. Since it checked for typeContent for AddToGlobalHistory and not for redirects, I did exactly the same thing. > Can you use the URIBinder here? > nit: moz::svcs Can't do either of these because it's an external program. Anything I use in libxul I have to QI. I tried to be sure, but I get undefined symbols. > should you check history validity here? do_check_true continues program flow no matter what, so tests can still crash. > It's useful for constants to not be zero. 1 << 0 == 1 > You don't accidentally miss an argument when reading the code. Could we move on from this discussion? It's a nit and we don't seem to be going anywhere. I chose the parameter-per-line style.

(In reply to comment #25) > What will these other history implementations do? Implement IHistory? No they could just not have Places and have their own GlobalHistory and GlobalHistory2 implementations > > + if (mItemType != typeContent) > > + return NS_OK; > > > > Is this somehow handled for redirect? Should it be handled? And if so, is it > > the information known at that time? > > I was modeling this based off the previous nsDocShell code. Since it checked > for typeContent for AddToGlobalHistory and not for redirects, I did exactly the > same thing. Yes but the old implementation was working reversed! thus the check was running before adding any visit. > > Can you use the URIBinder here? > > nit: moz::svcs > > Can't do either of these because it's an external program. Anything I use in > libxul I have to QI. I tried to be sure, but I get undefined symbols. fine > Could we move on from this discussion? It's a nit and we don't seem to be > going anywhere. I chose the parameter-per-line style. fine

Fix bitrot and address comments. Things (apparently) left to do: * Make sure the docshell typeContent checks are correct * HTTP redirect tests * tryserver run * Blog post / newsgroup post on docshell changes * sr review * sdwilsh review * (maybe?) Manual testing of correct history entries pre-patch and post-patch

(In reply to comment #25) > do_check_true continues program flow no matter what, so tests can still crash. A reported failure followed by a crash seems fine (especially in this case where the failure is very unlikely to happen)

Now with docshell redirect browser chrome test. I noticed from looking at other tests that nsNavHistoryObserver's OnVisit was not being fired with the new code path, so this patch includes a fix. > * Make sure the docshell typeContent checks are correct I think we are OK here. VisitURI has code that rejects saving visits for anything that is not web content (like http:// etc).

(In reply to comment #29) > > * Make sure the docshell typeContent checks are correct > > I think we are OK here. VisitURI has code that rejects saving visits for > anything that is not web content (like http:// etc). the canAdd? that is a blacklist, not likely to stop a bunch of things... changing it to a whitelist should not be hard though.

Comment on attachment 444993 [details] [diff] [review] Patch v6 Really only made it through the docshell stuff here, but I'm giving you enough comments that fixing that up first will be very helpful. It's probably the most complicated part of this patch, so it's good to get it right first. Review comments with more context can be found at http://reviews.visophyte.org/r/444993/ on file: docshell/base/IHistory.h line 61 > enum VisitFlags { We should move this to either just above the method that uses them. on file: docshell/base/IHistory.h line 63 > * Whether the URI was loaded in a top-level window. How about using a proper full sentence: Indicates if the URI was loaded in a top-level window. on file: docshell/base/IHistory.h line 67 > * Whether the URI was loaded as part of a permanent redirect. ditto on file: docshell/base/IHistory.h line 71 > * Whether the URI was loaded as part of a temporary redirect. ditto on file: docshell/base/IHistory.h line 115 > * @param aURI > * The URI of the page being visited. > * @param aReferrer > * The URI of the referring page. > * @param aFlags > * The VisitFlags describing this visit. > * @param aRedirectedFrom [optional] > * The URI this URI was redirected from, or NULL if not a redirect. Can we have a description of the method please? Also: need preconditions about things not being null like the other methods have. on file: docshell/base/nsDocShell.cpp line 5679 > if (IsFrame()) > visitURIFlags &= mozilla::IHistory::TOP_LEVEL; nit: please brace one line ifs in this file (not going to comment on this any further) on file: docshell/base/nsDocShell.cpp line 5695 > mozilla::IHistory* history = nsContentUtils::GetHistory(); > if (history) { Again, you can assume this will never fail. Startup blows up if it can. on file: docshell/base/nsDocShell.cpp line 5701 > if (NS_SUCCEEDED(rv) && !mCalledVisitURIForReferrer) { > // Since the primary purpose of this event is to handle a redirect, > // silently continue if VisitURI call fails. > (void)history->VisitURI(oldURI, NULL, visitURIFlags, referrer); > mCalledVisitURIForReferrer = true; This block of code is here because we won't add the first link and referrer in the redirect chain, correct? If so, we need to do a few things here to clarify why it's here (it took me a good 15 minutes to figure out what this code is doing, why it was needed, etc): 1) Add a comment explaining the situation. 2) rename mCalledVisitURIForReferrer to something like mFirstRedirect to be more clear what state it is actually tracking and help explain why we need to do it. on file: docshell/base/nsDocShell.cpp line 5708 > // Ignore internal redirects. > // These redirects are not initiated by the remote server, but specific to the > // channel implementation, so they are ignored. > if (!(aRedirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL)) > (void)history->VisitURI(newURI, NULL, visitURIFlags, oldURI); We should also add a comment explaining why we will do the last step even if it is an internal redirect. on file: docshell/base/nsDocShell.cpp line 10148 > nsresult > nsDocShell::AddToGlobalHistory(nsIURI * aURI, PRBool aRedirect, > nsIURI * aReferrer) Since we are rewriting this whole method, why don't we update the pointer style here too. on file: docshell/base/nsDocShell.cpp line 10163 > if (!mCalledVisitURIForReferrer) { > // This request has not called VisitURI linking the referrer to the new > // location yet. There were no redirects. So basically, we don't call this in the case where there were redirects, correct? I think it might be best to remove nsDocShell::AddToGlobalHistory and have each call site call VisitURI. The following sites would have to be modified: * nsDocShell::OnRedirectStateChange This is already being handled by this patch. * nsDocShell::OnNewURI This is the most complicated case. We'd have to check if it's a POST page, because we don't add those to history. However, we can do this check after we get the nsIHTTPChannel and then just set updateHistory to PR_FALSE. And getting the referrer from the channel is easy, so I won't explain that part. * nsDocShell::AddState This will be simple. Simply call history->AddVisit(newURI, oldURI, (IsFrame() ? 0 : mozilla::IHistory::TOP_LEVEL)); Maybe we also want to make a helper method called GetVisitFlags to do this for us. Doing this gets rid of the complicated logic in AddToGlobalHistory since the callers already have all the right information. The abstraction the method provides ends up being more confusing for us. We also shouldn't have to worry about if we've notified on redirect now (but you should check me on that). on file: docshell/base/nsDocShell.cpp line 10167 > NS_ENSURE_STATE(history); do not need to check this, can assert though on file: toolkit/components/places/src/Helpers.h line 156 > * @param aURI URI that contains spec to reverse > * @param aRevHost Out parameter nit: description on new line inline with the "a" after @param Will stop commenting on this issue. on file: toolkit/components/places/src/History.h line 57 > class History : public IHistory, public nsIObserver nit: class History : public IHistory , public nsIObserver

(In reply to comment #31) > Again, you can assume this will never fail. Startup blows up if it can. Again? Changed in this spot at least. > We should also add a comment explaining why we will do the last step even if > it is an internal redirect. Actually, this is a bug! Fixed. > So basically, we don't call this in the case where there were redirects, > correct? I think it might be best to remove nsDocShell::AddToGlobalHistory > and have each call site call VisitURI. The following sites would have to be > modified: > ... > Doing this gets rid of the complicated logic in AddToGlobalHistory since the > callers already have all the right information. The abstraction the method > provides ends up being more confusing for us. Not sure this buys us anything. See below. > We also shouldn't have to worry about if we've notified on redirect now (but > you should check me on that). Yeah, we still do. Ignore push state docshell stuff for a moment. Here are two cases that should illuminate the problem: CASE 1 ====== uri A (click link) => uri B URI A is referrer of URI B. A redirect never occurs, so OnNewURI will call VisitURI(A => B). CASE 2 ====== uri A (click link) => uri B (redirect) => uri C Think of the flow of events on a timeline. DocShell knew about URI B during the redirect, but by the time OnNewURI is called site B has been "forgotten," so we can't call VisitURI(B => C). We have to add *both* visits during redirect. Note that in this case, OnNewURI had no work to do. In case 1, OnNewURI calls AddVisit. In case 2, OnNewURI does not call AddVisit at all. So we need some sort of state to remember if a redirect occurred. New patch soon...

Review comments addressed, fixed internal redirect bug, check for mItemType for redirects, and a good heaping of comments for nsDocShell.

Comment on attachment 445204 [details] [diff] [review] Patch v7 Not done reviewing this yet, but getting the docshell stuff up so you can see my comments. For comments with expandable context, please see http://reviews.visophyte.org/r/445204/ on file: docshell/base/nsDocShell.h line 684 > bool mFirstRedirect; better name: mRedirectOccurred on file: docshell/base/nsDocShell.cpp line 5691 > nsresult rv = aOldChannel->GetURI(getter_AddRefs(oldURI)); > if (NS_FAILED(rv)) { > return; > } > rv = aNewChannel->GetURI(getter_AddRefs(newURI)); > if (NS_FAILED(rv)) { > return; > } We can condense all of this by not checking the return variable, and then just doing: if (!newURI || ! oldURI) { return; } on file: docshell/base/nsDocShell.cpp line 5700 > if (mItemType == typeContent) { > mozilla::IHistory* history = nsContentUtils::GetHistory(); > // Ignore internal redirects. > // These redirects are not initiated by the remote server, but specific to the > // channel implementation, so they are ignored. > if (!(aRedirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL)) { Let's get rid of some nesting here and combine these two. Less indenting would be nice. Change the comment to something like "only recording history of content docshells that are not internal redirects..." Also, please add a comment in nsDocShell.h indicating that mItemType indicates if this document (?) is a chrome document or a content document. I just dug through a ton of CVS blame to verify this. on file: docshell/base/nsDocShell.cpp line 10145 > nsresult > nsDocShell::AddToGlobalHistory(nsIURI* aURI, PRBool aRedirect, > nsIChannel* aChannel) Let's nuke this variant. That means that nsDocShell::OnNewURI will need to get the response type somewhere in here (http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8980) and set the updateHistory flag to PR_FALSE if GetRequestType is != "GET" (this is a change that I discussed with biesi and bz and they agree that that is the right behavior and the current one is wrong). Then grab the referrer here (http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9088) and we are all set to call our new method... on file: docshell/base/nsDocShell.cpp line 10166 > nsresult > nsDocShell::AddToGlobalHistory(nsIURI * aURI, PRBool aRedirect, > nsIURI * aReferrer) ...and this will get nixed as well to be replaced by nsDocShell::AddURIVisit(nsIURI* aURI, nsIURI* aReferrer). In it, you can have an early return for mItemType not being typeContent, and also an early return if mRedirectOccurred is true (but be sure to set it back to false!). Then, just call VistuURI(aURI, aReferrer (IsFrame() ? 0 : mozilla::IHistory::TOP_LEVEL)); OnNewURI and AddState will call this. on file: toolkit/components/places/src/Helpers.h line 161 > nsresult GetReversedHostname(nsIURI* aURI, nsAString& aRevHost); nit: can we try to use nsString here? on file: toolkit/components/places/src/Helpers.h line 166 > void GetReversedHostname(const nsString& aForward, nsAString& aRevHost); nit: can we try to use nsString here? on file: toolkit/components/places/src/Helpers.h line 176 > void ReverseString(const nsString& aInput, nsAString& aReversed); nit: can we try to use nsString here?

More context at the same place as last time. Also, Helpers.cpp doesn't apply. on file: toolkit/components/places/src/nsNavHistory.h line 91 > // Microsecond timeout for "recent" events such as typed and bookmark following. > // If you typed it more than this time ago, it's not recent. > #define RECENT_EVENT_THRESHOLD PRTime((PRInt64)15 * 60 * PR_USEC_PER_SEC) define what this time is in the comments that is more readable on file: toolkit/components/places/src/nsNavHistory.h line 136 > PRBool typed; // User typed in URL recently > PRBool activated; // User tapped URL link recently > PRBool bookmarked; // User bookmarked URL recently > PRBool redirected; // Was redirected to URL recently nit: can these be bool? on file: toolkit/components/places/src/nsNavHistory.h line 410 > RECENT_TYPED = 1<<0, // User typed in URL recently > RECENT_ACTIVATED = 1<<1, // User tapped URL link recently > RECENT_BOOKMARKED = 1<<2 // User bookmarked URL recently (hey there's a <3!) nit: spaces before and after << on file: toolkit/components/places/src/nsNavHistory.h line 416 > * Returns any recent activity done with a URL. > * @return Any recent events associated with this URI. Each bit is set > * according to RecentEventFlags enum values. nit: @returns and please include @param on file: toolkit/components/places/src/nsNavHistory.h line 420 > PRUint32 GetRecentFlags(nsIURI *aURI); nit: pointer style is not correct on file: toolkit/components/places/src/nsNavHistory.h line 453 > void FireOnVisit(nsIURI* aURI, I think we actually want NotifyOnVisit on file: toolkit/components/places/tests/cpp/places_test_harness.h line 179 > "SELECT id, hidden, typed, visit_count FROM moz_places WHERE url=?1 " > "UNION ALL " > "SELECT id, hidden, typed, visit_count FROM moz_places_temp WHERE url=?1 " > "LIMIT 1" nit: format this like we normally do SQL please on file: toolkit/components/places/tests/cpp/places_test_harness.h line 194 > rv = stmt->GetInt64(0, &result.id); > do_check_true(NS_SUCCEEDED(rv)); > rv = stmt->GetInt32(1, &result.hidden); > do_check_true(NS_SUCCEEDED(rv)); > rv = stmt->GetInt32(2, &result.typed); > do_check_true(NS_SUCCEEDED(rv)); > rv = stmt->GetInt32(3, &result.visitCount); > do_check_true(NS_SUCCEEDED(rv)); in practice, these just won't fail. Use AsInt[32|64] Also, please have mak review the tests.

(In reply to comment #35) > nit: @returns and please include @param just to note that: @returns is wrong javadoc syntax, @return is correct > on file: toolkit/components/places/tests/cpp/places_test_harness.h line 194 > > rv = stmt->GetInt64(0, &result.id); > > do_check_true(NS_SUCCEEDED(rv)); > > rv = stmt->GetInt32(1, &result.hidden); > > do_check_true(NS_SUCCEEDED(rv)); > > rv = stmt->GetInt32(2, &result.typed); > > do_check_true(NS_SUCCEEDED(rv)); > > rv = stmt->GetInt32(3, &result.visitCount); > > do_check_true(NS_SUCCEEDED(rv)); > > in practice, these just won't fail. Use AsInt[32|64] As a side note, we still have that AsIntXX bug open (bug 547190)

(In reply to comment #36) > just to note that: > @returns is wrong javadoc syntax, @return is correct Ack...my brain flipped those :( > As a side note, we still have that AsIntXX bug open (bug 547190) In this case (test), I don't think it hurts us.

And, a we bit more that I managed to forget: on file: toolkit/components/places/src/History.cpp line 71 > // Step nit: four slashes, not two here on file: toolkit/components/places/src/History.cpp line 95 > /** Executes statement asynchronously using this as a callback. */ > NS_IMETHOD ExecuteAsync(mozIStorageStatement* stmt); > > /** > * Called once after query is completed. If your query has more than one > * result set to process, you will want to override HandleResult to process > * each one. > */ > NS_IMETHOD Callback(mozIStorageResultSet* aResultSet); > > /** > * By default, stores the last result set received in mResultSet. > * For queries with only one result set, you don't need to override. > */ > NS_IMETHOD HandleResult(mozIStorageResultSet* aResultSet); > > /** > * By default, this calls Callback with any saved results from HandleResult. > * For queries with only one result set, you don't need to override. > */ > NS_IMETHOD HandleCompletion(PRUint16 aReason); none of these have proper javadoc comments. this makes me sad. on file: toolkit/components/places/src/History.cpp line 127 > NS_IMETHODIMP Step::ExecuteAsync(mozIStorageStatement* stmt) nit: NS_IMETHODIMP should be on a line on it's own nit: stmt->aStatement or aStmt on file: toolkit/components/places/src/History.cpp line 129 > mozStorageStatementScoper scoper(stmt); not benefit of using the scoper here on file: toolkit/components/places/src/History.cpp line 163 > NS_ENSURE_ARG_POINTER(aURI); I hope Marco didn't tell you to change this. We control the only consumers of this, and we know it's not going to pass null, so keep it assertion (we want to know when we screw up our invariants in debug builds, but with tests, we never will). on file: toolkit/components/places/src/History.cpp line 335 > //////////////////////////////////////////////////////////////////////////////// > //// IHistory > > namespace { none of this stuff in the namespace block is part if the IHistory interface, even though it's in that section. This stuff should all be at the top of the file. on file: toolkit/components/places/src/History.cpp line 340 > /** > * STEP 5: Update frecency of URI and notify observers. > */ Could you please order these in the right numerical order. Going backwards is confusing. on file: toolkit/components/places/src/History.cpp line 343 > class UpdateFrecencyAndNotifyStep : public Step { nit: style guide says to have the brace on a new line on file: toolkit/components/places/src/History.cpp line 344 > protected: > VisitURIData* mData; nit: this is almost always after public; I'd prefer it there on file: toolkit/components/places/src/History.cpp line 350 > UpdateFrecencyAndNotifyStep(VisitURIData* aData) : mData(aData) nit: this doesn't match our convention. Please place : mData on a new line on file: toolkit/components/places/src/History.cpp line 357 > NS_ENSURE_STATE(aResultSet); you should never have to worry about this being false (we'd fail tests so hard...) on file: toolkit/components/places/src/History.cpp line 415 > protected: > VisitURIData* mData; > protected at end please on file: toolkit/components/places/src/History.cpp line 421 > GetVisitIDStep(VisitURIData* aData) : mData(aData) : mData on new line (will stop commenting on this) on file: toolkit/components/places/src/History.cpp line 492 > } else { nit: this file uses the } else { format for this. Only commenting once. on file: toolkit/components/places/src/History.cpp line 614 > // Get newly created ID Use proper, full sentences for comments please (with punctuation!) on file: toolkit/components/places/src/History.cpp line 621 > nsCAutoString url; unused on file: toolkit/components/places/src/History.cpp line 732 > void History::StartNextVisitURI() nit: void on own line Also, really sucks that we need to do this, but I can't think of a way around it :( on file: toolkit/components/places/src/History.cpp line 770 > NS_ENSURE_ARG_POINTER(aURI); It's a precondition and we control the consumers, so this should just be NS_PRECONDITION (like the other IHistory methods use NS_ASSERTION) on file: toolkit/components/places/src/History.cpp line 951 > NS_IMPL_ISUPPORTS2( > History, > IHistory, > nsIObserver > ) We don't actually need to QI to nsIObserver, so we don't need to add it.

I think i said to change NS_ASSERTION to NS_ENSURE_ARG in visitURI since it's exposed in the idl... I probably missed the internal calls :\

(In reply to comment #39) > I think i said to change NS_ASSERTION to NS_ENSURE_ARG in visitURI since it's > exposed in the idl... I probably missed the internal calls :\ It's not in an idl; it's in a .h file that is only used internally. We don't ship it as part of the SDK.

> We can condense all of this by not checking the return variable, and then just > doing: > if (!newURI || ! oldURI) { > return; > } Comment #15 mak had me change exactly this, so I think I get to decide :) Leaving it alone. > Let's get rid of some nesting here and combine these two. Less indenting > would be nice. Change the comment to something like "only recording history > of content docshells that are not internal redirects..." That doesn't add anything to just reading the if statement. The information about internal channel redirects is useful, so I'll rephrase that part. > Also, please add a comment in nsDocShell.h indicating that mItemType indicates > if this document (?) is a chrome document or a content document. I just dug > through a ton of CVS blame to verify this. Same here (except I used MXR :)). Also, mItemType does not really refer to anything about the current document, but the type of the docshell. Once a chrome or content docshell is created, its type does not change.

> Could you please order these in the right numerical order. Going backwards is > confusing. Actually, I can't unless a forward declare all the steps. I personally like having less lines of code by declaring them inline, but the out-of-order thing might be worse. Should we put them in order with function declarations outside of classes? > you should never have to worry about this being false (we'd fail tests so > hard...) Actually it is totally possible to get null here if the SQL statement was an update. Callback is always called. > Also, really sucks that we need to do this, but I can't think of a way around > it :( Same here. > on file: toolkit/components/places/src/History.cpp line 770 > > NS_ENSURE_ARG_POINTER(aURI); > > It's a precondition and we control the consumers, so this should just be > NS_PRECONDITION (like the other IHistory methods use NS_ASSERTION) Why is this distinction important? When should I use NS_ASSERTION, NS_PRECONDITION, or NS_ENSURE_ARG_POINTER? And why should I care? > We don't actually need to QI to nsIObserver, so we don't need to add it. mak asked me to add it in a previous review. Does it hurt to have it?

Addresses review comments.

(In reply to comment #41) > > We can condense all of this by not checking the return variable, and then just > > doing: > > if (!newURI || ! oldURI) { > > return; > > } > > Comment #15 mak had me change exactly this, so I think I get to decide :) > Leaving it alone. well actually both ways are fine, I complained that you were assigning rv and then not checking it, and I was unclear on the solution... btw, we can survive it, if you want to remove rv completely I'm fine with it, otherwise I'm fine still. (In reply to comment #42) > Why is this distinction important? When should I use NS_ASSERTION, > NS_PRECONDITION, or NS_ENSURE_ARG_POINTER? And why should I care? let me see if I can give a decent answer... NS_ENSURE_XXX methods exist in optimized builds too, thus the most useless checks we can avoid, the most code we avoid in optimized builds (checks are not expensive, but 1 thousands useless checks are). So we ensure just things that are problematic durint optimized runtime. iirc NS_PRECONDITION and NS_ASSERTION (there is also NS_POSTCONDITION iirc) are practically the same thing, PRECONDITION should just be a name that people prefers to the generic assertion, since it's condition checked before running the method core. Those are debug methods, thus in optimized builds there is no code there.

please notice bug 130327 is going to change docshell AddToGlobalHistory

(In reply to comment #41) > Comment #15 mak had me change exactly this, so I think I get to decide :) > Leaving it alone. With a few exceptions, docshell seems to trend towards not checking the return variable and instead checking the URI from channels. We should probably try to be consistent. > Same here (except I used MXR :)). Also, mItemType does not really refer to > anything about the current document, but the type of the docshell. Once a > chrome or content docshell is created, its type does not change. Right, docshell not document. (In reply to comment #44) > let me see if I can give a decent answer... > NS_ENSURE_XXX methods exist in optimized builds too, thus the most useless > checks we can avoid, the most code we avoid in optimized builds (checks are not > expensive, but 1 thousands useless checks are). So we ensure just things that > are problematic durint optimized runtime. > > iirc NS_PRECONDITION and NS_ASSERTION (there is also NS_POSTCONDITION iirc) are > practically the same thing, PRECONDITION should just be a name that people > prefers to the generic assertion, since it's condition checked before running > the method core. Those are debug methods, thus in optimized builds there is no > code there. BINGO (In reply to comment #45) > please notice bug 130327 is going to change docshell AddToGlobalHistory This may actually land first, but either way, it won't be hard to integrate this change in. Hopefully it lands with tests?

(In reply to comment #42) > Actually, I can't unless a forward declare all the steps. I personally like > having less lines of code by declaring them inline, but the out-of-order thing > might be worse. Should we put them in order with function declarations outside > of classes? *sigh* Sometimes I hate C++. Leave it as is.

> (In reply to comment #45) > > please notice bug 130327 is going to change docshell AddToGlobalHistory > This may actually land first, but either way, it won't be hard to integrate > this change in. Hopefully it lands with tests? I really hope so, a test is feasible and I feel like it is needed.

(In reply to comment #34) > nit: can we try to use nsString here? I see you did this, which is awesome, but I never explained why. nsAString is an abstract class and there is therefore overhead in using it. Per the xpcom string guide, we are supposed to avoid using the abstract classes whenever possible.

Comment on attachment 445415 [details] [diff] [review] Patch v8 To see more context with these comments, please go to http://reviews.visophyte.org/r/445415/. on file: docshell/base/nsDocShell.h line 469 > // Global History This is no longer for Global History, so this can be nuked. Please also add proper java-doc style comment explaining what this does. on file: docshell/base/nsDocShell.h line 680 > // It is used for VisitURI calls. nit: s/VisitURI/IHistory::VisitURI/ so it's less ambiguous. on file: docshell/base/nsDocShell.h line 741 > // This can either be a content docshell or a chrome docshell. After > // Create() is called, the type is not expected to change. Awesome. Thank you! on file: docshell/base/nsDocShell.cpp line 5691 > nsresult rv = aOldChannel->GetURI(getter_AddRefs(oldURI)); > if (NS_FAILED(rv)) { > return; > } > rv = aNewChannel->GetURI(getter_AddRefs(newURI)); > if (NS_FAILED(rv)) { > return; > } per comment 46 please on file: docshell/base/nsDocShell.cpp line 5700 > if (mItemType == typeContent && > !(aRedirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL)) { > > // Internal redirects are ignored because they are specific to the > // channel implementation. > mozilla::IHistory* history = nsContentUtils::GetHistory(); > if (!(aRedirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL)) { You combined the two if statements like I asked, but then forgot to remove the second one ;) Also, I missed this in past iterations, but we should add a comment about only adding typeContent things to history as well. on file: docshell/base/nsDocShell.cpp line 9022 > // If this is a POST request, we do not want to include this in global > // history. > updateHistory = PR_FALSE; As I recall my discussion with bz on this, every http channel should be able to QI to nsIUploadChannel, so just knowing that it is an upload channel is not enough. We need to call GetRequestType and if it is not a GET request, we should not add it to history. on file: docshell/base/nsDocShell.cpp line 9105 > // Get the Cache Key and store it in SH. > if (cacheChannel) Death to extraneous white space! on file: docshell/base/nsDocShell.cpp line 9130 > if (!inputStream) { > nsCOMPtr<nsIURI> referrer; > if (aChannel) > NS_GetReferrerFromChannel(aChannel, getter_AddRefs(referrer)); > > AddURIVisit(aURI, referrer); oh...I see what you did. I think checking input stream 100 lines later is less clear than doing the check up above. on file: docshell/base/nsDocShell.cpp line 10157 > nsDocShell::AddURIVisit(nsIURI * aURI, > nsIURI * aReferrer) nit: nsIURI* aURI (not that this file is consistent with this, but we should go with what the style guide says in that case). on file: docshell/base/nsDocShell.cpp line 10160 > if (mItemType != typeContent) > return NS_OK; may want to add for this for clarity nit: brace one line if for this new code on file: toolkit/components/places/src/History.cpp line 93 > virtual ~Step() {} > you should not need this on file: toolkit/components/places/src/History.cpp line 180 > NS_ASSERTION(aURI, "Null URI"); nit: since you are touching this, care to make it NS_PRECONDITION? on file: toolkit/components/places/src/History.cpp line 671 > nsresult rv = observerService->AddObserver(this, > URI_VISIT_SAVED, > PR_FALSE); nit: weird indentation here on file: toolkit/components/places/src/History.cpp line 812 > nsCAutoString url; > rv = aURI->GetSpec(url); still a uri, not a url. It sorta matters. Just use "spec" instead. on file: toolkit/components/places/src/History.cpp line 815 > if (aReferrer) { > rv = aReferrer->GetSpec(data->lastUrl); > NS_ENSURE_SUCCESS(rv, rv); > } > else if (aRedirectFrom) { > rv = aRedirectFrom->GetSpec(data->lastUrl); > NS_ENSURE_SUCCESS(rv, rv); > } ditto with lastUrl. Should be lastSpec on file: toolkit/components/places/src/History.cpp line 832 > if (aFlags & REDIRECT_TEMPORARY) { > data->transitionType = nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY; > } > else if (aFlags & REDIRECT_PERMANENT) { > data->transitionType = nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT; > } We could just add two PR_STATIC_ASSERTS here to ensure that REDIRECT_TEMPORARY == nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY and the other constant, and then just do: data->transitionType = aFlags; // all the other ifs we care about on file: toolkit/components/places/src/nsNavHistory.h line 135 > struct RecentEventResult { > PRBool typed; // User typed in URL recently > PRBool activated; // User tapped URL link recently > PRBool bookmarked; // User bookmarked URL recently > PRBool redirected; // Was redirected to URL recently > }; comment 35 asked if these could be bool. Never saw a response from you saying why they couldn't be. on file: toolkit/components/places/src/nsNavHistory.h line 409 > enum RecentEventFlags { > RECENT_TYPED = 1<<0, // User typed in URL recently > RECENT_ACTIVATED = 1<<1, // User tapped URL link recently > RECENT_BOOKMARKED = 1<<2 // User bookmarked URL recently (hey there's a <3!) > }; comment 17 asked for spacing around this. Also, your comment about <3 is no longer true on file: toolkit/components/places/tests/browser/browser_visituri.js line 1 > /* ***** BEGIN LICENSE BLOCK ***** > * Version: MPL 1.1/GPL 2.0/LGPL 2.1 > * > * The contents of this file are subject to the Mozilla Public License Version > * 1.1 (the "License"); you may not use this file except in compliance with > * the License. You may obtain a copy of the License at > * http://www.mozilla.org/MPL/ > * > * Software distributed under the License is distributed on an "AS IS" basis, > * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License > * for the specific language governing rights and limitations under the > * License. > * > * The Original Code is mozilla.org code. > * > * The Initial Developer of the Original Code is > * Mozilla Corporation. > * Portions created by the Initial Developer are Copyright (C) 2008 > * the Initial Developer. All Rights Reserved. > * > * Contributor(s): > * > * Alternatively, the contents of this file may be used under the terms of > * either the GNU General Public License Version 2 or later (the "GPL"), or > * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), > * in which case the provisions of the GPL or the LGPL are applicable instead > * of those above. If you wish to allow use of your version of this file only > * under the terms of either the GPL or the LGPL, and not to allow others to > * use your version of this file under the terms of the MPL, indicate your > * decision by deleting the provisions above and replace them with the notice > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ Test files should use public domain dedication per http://www.mozilla.org/MPL/license-policy.html Make sure you add it to your other test files too. on file: toolkit/components/places/tests/browser/browser_visituri.js line 16 > * The Initial Developer of the Original Code is > * Mozilla Corporation. nit (and for future reference): this should always be "the Mozilla Foundation" on file: toolkit/components/places/tests/cpp/places_test_harness.h line 155 > already_AddRefed<mozIStorageConnection> > do_get_mozIStorageConnection() would prefer one of these: do_get_db() do_get_dbConnection() on file: toolkit/components/places/tests/cpp/places_test_harness.h line 168 > void > getPlace(nsIURI* aURI, PlaceRecord& result) not clear what this does by the name, so please add java-doc style comments Also, everything else is in the style of do_get_X, but this isn't on file: toolkit/components/places/tests/cpp/places_test_harness.h line 176 > do_check_true(NS_SUCCEEDED(rv)); we have do_check_success, so please replace these (many) instances with that instead. on file: toolkit/components/places/tests/cpp/places_test_harness.h line 178 > rv = dbConn->CreateStatement(NS_LITERAL_CSTRING( > "SELECT id, hidden, typed, visit_count FROM moz_places WHERE url=?1 " > "UNION ALL " > "SELECT id, hidden, typed, visit_count FROM moz_places_temp WHERE url=?1 " > "LIMIT 1" > ), getter_AddRefs(stmt)); this query isn't formatted right, and it's also going to give you the wrong results. You need to always query from moz_places_temp because that has the most recent record. This should almost use the query api so that you don't even have to worry about temp tables (and when we get rid of them, this test will continue to work without being changed). on file: toolkit/components/places/tests/cpp/places_test_harness.h line 204 > void > getLastVisit(PRInt64 placeId, VisitRecord& result) ditt on on java-doc and do_get_X comments on file: toolkit/components/places/tests/cpp/places_test_harness.h line 216 > do_check_true(rv == NS_OK); do_check_success for these too on file: toolkit/components/places/tests/cpp/test_IHistory.cpp line 368 > class VisitURIFinishObserver : public nsIObserver This class is not a test function, so it should go above the Test Functions section. Also, just call it "VisitURIObserver" on file: toolkit/components/places/tests/cpp/test_IHistory.cpp line 373 > VisitURIFinishObserver(int totalVisits = 1) : nit: aExpectedVisits? on file: toolkit/components/places/tests/cpp/test_IHistory.cpp line 385 > ~VisitURIFinishObserver() > { > } > you can get rid of this; it's not needed on file: toolkit/components/places/tests/cpp/test_IHistory.cpp line 389 > void MainEventLoop() Let's call this WaitForNotification so it is more clear what it's doing. on file: toolkit/components/places/tests/cpp/test_IHistory.cpp line 412 > int mTotalVisits; and then mExpectedVisits on file: toolkit/components/places/tests/cpp/test_IHistory.cpp line 435 > do_check_true(!place.hidden); > do_check_true(!place.typed); I think you want do_check_false on file: toolkit/components/places/tests/cpp/test_IHistory.cpp line 483 > do_check_true(!place.hidden); do_check_false on file: toolkit/components/places/tests/cpp/test_IHistory.cpp line 514 > nsCOMPtr<nsIBrowserHistory> browserHistory = > do_QueryInterface((nsCOMPtr<nsINavHistoryService>) do_get_NavHistory()); I'm not sure this is actually safe to do. You should just put this on two different lines on file: toolkit/components/places/tests/cpp/test_IHistory.cpp line 573 > > TEST(test_visituri_inserts), > TEST(test_visituri_updates), > TEST(test_visituri_preserves_shown_and_typed), > TEST(test_visituri_creates_visit), > TEST(test_visituri_transition_typed), > TEST(test_visituri_transition_embed), nit: empty line there is not needed I'd also like to see a test to check that if one part of the chain is not marked as hidden the whole this is not hidden. r=sdwilsh with comments addressed.

cc'ing bz since he'll need to be on this bug since he needs to sr this next (once a new patch is up)

> on file: docshell/base/nsDocShell.cpp line 10160 > > if (mItemType != typeContent) > > return NS_OK; > > may want to add for this for clarity Add what? > nit: brace one line if for this new code I think the above means "change quoted code to a one-lined if statement with braces." If I'm right, why would I do that? > on file: toolkit/components/places/src/History.cpp line 93 > > virtual ~Step() {} > > > > you should not need this bz: Shawn and I had a discussion on IRC about this. I think the virtual destructor is needed because there is no "virtual" keyword in the generated .h files for its base classes (nsISupports and AsyncCallback). Since DecRef will do a "delete this" on a Step* this pointer, I would expect any destructors of the member variables in Step's children would not get called. Shawn thought everything would probably work out if I removed it, but I'd like to understand *why* it would. Could you enlighten me? > on file: toolkit/components/places/src/History.cpp line 832 > > if (aFlags & REDIRECT_TEMPORARY) { > > data->transitionType = nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY; > > } > > else if (aFlags & REDIRECT_PERMANENT) { > > data->transitionType = nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT; > > } > > We could just add two PR_STATIC_ASSERTS here to ensure that REDIRECT_TEMPORARY > == nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY and the other constant, > and then just do: > data->transitionType = aFlags; > // all the other ifs we care about That seems like a much less readable "optimisation" to me. > comment 35 asked if these could be bool. Never saw a response from you saying > why they couldn't be. This is actually never used! So I removed it. > comment 17 asked for spacing around this. Also, your comment about <3 is no > longer true I added more spacing between the comments and the enums. What precisely do you want here? Also, that's pretty funny that the comment doesn't work anymore :) > Test files should use public domain dedication per > http://www.mozilla.org/MPL/license-policy.html > > Make sure you add it to your other test files too. Technically, according to the wiki, MPL/GPL/LGPL is completely fine for test cases too (but it's also appropriate to use public domain). Changing anyways. > nit (and for future reference): this should always be "the Mozilla Foundation" Borrowed from browser_bug399606.js. Good to know though. > on file: toolkit/components/places/tests/cpp/test_IHistory.cpp line 514 > > nsCOMPtr<nsIBrowserHistory> browserHistory = > > do_QueryInterface((nsCOMPtr<nsINavHistoryService>) do_get_NavHistory()); > > I'm not sure this is actually safe to do. You should just put this on two > different lines Why? Changing it regardless. > I'd also like to see a test to check that if one part of the chain is not > marked as hidden the whole this is not hidden. test_visituri_preserves_shown_and_typed does this. New patch soon...

Adding review flag back since there are some outstanding questions.

(In reply to comment #52) > > may want to add for this for clarity > Add what? may want to add *a comment* for this for clarity. Not sure how I dropped those two words... > I think the above means "change quoted code to a one-lined if statement with > braces." If I'm right, why would I do that? It does. We sometimes do it in this file, and sometimes don't, so that's a sign that we should stick with the style guide which says https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Control_Structures. I'll note that this file also seems to follow the } else { which is contrary to the style guide, but seems consistent in the file (I know, this is a mess). > > comment 17 asked for spacing around this. Also, your comment about <3 is no > > longer true > > I added more spacing between the comments and the enums. What precisely do you > want here? Also, that's pretty funny that the comment doesn't work anymore :) 1 << 2 instead of 1<<2 is what mak was looking for > Technically, according to the wiki, MPL/GPL/LGPL is completely fine for test > cases too (but it's also appropriate to use public domain). Changing anyways. Yeah, while it's fine, I'm encouraging patch authors to do public domain. Having only four lines of license block instead of ~40 is much nicer. > Borrowed from browser_bug399606.js. Good to know though. It has been more of a recent thing that we realized we were doing wrong. Trying to make sure code going forward is correct. > Why? Changing it regardless. It looked funny before, but now I see it was just a C style cast. We frown upon C style casts in the codebase anyway (use static_cast if you must). Thanks for changing it. > test_visituri_preserves_shown_and_typed does this. Missed that, but so it does!

Comment on attachment 446403 [details] [diff] [review] Patch v9 I gave this r+, so as long as you addressed my review comments (plus clarifications which I just posted), you are fine. Up for sr to bz!

> Shawn thought everything would probably work out if I removed it I believe this false, for exactly the reason you describe. But I'm not exactly a C++ language lawyer....

> I think the above means "change quoted code to a one-lined if statement with > braces." If I'm right, why would I do that? Oops, I realized you meant just add braces as opposed to add braces and put the *entire thing* on one line. My bad. Patch addresses review comments, let's get this to bz!

Comment on attachment 446533 [details] [diff] [review] Patch v10 Should probably use nsnull, not NULL. The flag-munging code in OnRedirectStateChange doesn't make any sense; it always ends up with visitURIFlags == 0. Did the tests not catch this? Why are you not adding visits in chrome docshells that have enabled global history? I'd prefer a |using mozilla::IHistory| or equivalent typedef to typing it all out for all those constants. This seems to be dropping support for non-IHistory history impls, right? Is that a hard requirement for 1.9.3? If so, have we actually announced that somewhere? I'm not sure why undocumented implementation details of your particular IHistory impl (visit chains, etc) are living in nsDocShell::OnRedirectStateChange. Please move that code elsewhere, or clearly document the API so that it's obvious why the VisitURI call with the referrer as the "redirected from" URI makes any sense. I also don't understand why the "don't include POST result pages" policy is being enforced in core docshell code, not in the history impl. Is that something we're sure we want to impose on all possible Gecko embeddings? Setting updateHistory to false for POSTs breaks session history, no? I'm saddened (but not surprised) we don't have existing tests for this; perhaps you can rectify that problem? I'd assume you need tests for this case anyway for your changes, right? I don't know why you decided to remove curly braces in code not related to your changes, but please don't do that. On the other hand, please do curly-brace your single-line if bodies in this code. I know it's not consistent about that, but that's what it should be doing going forward. mRedirectOccurred is broken. For example, you can set it but not clear it (e.g. for POST, but there are various other cases that I can describe if you care), which will make the next AddVisit call (totally unrelated to the redirect) get ignored. Why exactly do we need this anyway? Certainly the IHistory documentation says nothing useful that would make it clear it's needed. There's more comments about "the IHistory API" in AddURIVisit that have no basis in anything the API documentation actually says. If the API does guarantee such behavior, it should say so. If not, then we shouldn't be assuming it here. I would much prefer to make it clear to the API exactly what happened on our end and have it do whatever it wants with it instead of faking redirects, etc... I didn't read any of the changes outside docshell for now.

And perhaps more generally.... The history API should provide mechanism for the history impl to enforce whatever policy it wants. We clearly need to provide enough information for Firefox's history impl to work, but that doesn't mean that should be all the information provided. In particular, is all the information we used to provide to nsIGlobalHistory* still provided? If not, how do we expect history implementors to deal with that?

(In reply to comment #58) > Why are you not adding visits in chrome docshells that have enabled global > history? As far as I could tell, this is not a change in behavior. > This seems to be dropping support for non-IHistory history impls, right? Is > that a hard requirement for 1.9.3? If so, have we actually announced that > somewhere? Didn't we already do that with async-is-visited? I don't think we announced it though. > I also don't understand why the "don't include POST result pages" policy is > being enforced in core docshell code, not in the history impl. Is that > something we're sure we want to impose on all possible Gecko embeddings? Again, this is no different from the existing code in docshell as far as I can tell. I think I'm fine with changing these things, but I think that's best to do in a follow-up.

> As far as I could tell, this is not a change in behavior. Ah, indeed. > Didn't we already do that with async-is-visited? Not quite. We require support or mozilla::IHistory to do link coloring, and that was an API change we needed to make that work sanely. We do indeed need to announce that. However in this case, I'm not sure why we're making an API change. It seems like the existing history APIs are asyncable just fine (no more and no less than your new API). So this seems like API breakage for the sake of API breakage to me. Of a frozen API. I thought we weren't going to do that. Am I missing something? > Again, this is no different from the existing code in docshell as far as I can > tell. Hmm, yeah, ok. Ignore that part. <sigh>. I guess I even reviewed that code...

The old API would make hard to implement the new order we add visit information, that is reversed now (or better, it was reversed before). Maybe we could leave in the old GH2 and GH3 implementations in the docshell, detect if the implementation is Places, and in such a case skip the calls? (so if IHistory exists use it, otherwise fallback to the old GH2/GH3 impl).

> The old API would make hard to implement the new order we add visit > information, that is reversed now (or better, it was reversed before). I'm not sure I follow... Old API: void addURI(in nsIURI aURI, in boolean aRedirect, in boolean aToplevel, in nsIURI aReferrer); new API: VisitURI(nsIURI *aURI, nsIURI *aReferrer, PRUint32 aFlags, nsIURI *aRedirectedFrom = NULL) = 0; with the (undocumented!) precondition that !aReferrer or !aRedirectedFrom (or at least that's certainly true for all callers, and assumed by the impl). The impl then uses aReferrer if it's set, and aRedirectFrom otherwise; this means we could just use a single URI argument for the two. So what's reversed, exactly?

the old api was calling onDocumentRedirect and finally addURI, then visits were added from the newest to the oldest using cached data. the new api calls visitURI on redirects too, and visits are added from older to newer. There are 2 call points, one for redirects, one for addToGlobalHistory

> the old api was calling onDocumentRedirect and finally addURI, OK... > then visits were added from the newest to the oldest using cached data. This was inside the history impl, right? > the new api calls visitURI on redirects too Right... where the old one called onDocumentRedirect, yes?

(In reply to comment #65) > > then visits were added from the newest to the oldest using cached data. > This was inside the history impl, right? > > > the new api calls visitURI on redirects too > Right... where the old one called onDocumentRedirect, yes? yes, to both.

(In reply to comment #61) > Not quite. We require support or mozilla::IHistory to do link coloring, and > that was an API change we needed to make that work sanely. We do indeed need > to announce that. However in this case, I'm not sure why we're making an API > change. It seems like the existing history APIs are asyncable just fine (no > more and no less than your new API). So this seems like API breakage for the > sake of API breakage to me. Of a frozen API. I thought we weren't going to do > that. Am I missing something? nsIGlobalHistory3 is *not* frozen. nsIGlobalHistory2 is marked as "Under Review" but not frozen either (I'm actually not sure why we even created 3 if 2 wasn't frozen...). The problem here is that how docshell used to notify about visits was rather confusing with regards to how things were actually navigated. Basically, if you are at page A, click a link to page B, which redirects to page C, you would have gotten the following calls (as a history implementer): addDocumentRedirect([B's channel], [C's channel], ...) addURI([C], false /* always passed as false, unless nsIGlobalHistory3 was not implemented */, [top level or not, this case true], [A]); This meant that history implementations had to do a bunch of bookkeeping just to figure out that the user actually navigated like this: A -> B -> C

> nsIGlobalHistory3 is *not* frozen. nsIGlobalHistory2 is marked as "Under > Review" but not frozen either (I'm actually not sure why we even created 3 if 2 > wasn't frozen...). 2 was de-facto frozen. More importantly, note that we have shims in place so that all you have to implement is nsIGlobalHistory and the right things will happen. I have no problem with adjusting the calls in docshell for the A -> B -> C case to make it saner, honestly. It's just not clear to me that we need a new ABI/API at the same time...

> with the (undocumented!) precondition that !aReferrer or !aRedirectedFrom (or > at least that's certainly true for all callers, and assumed by the impl). The > impl then uses aReferrer if it's set, and aRedirectFrom otherwise; this means > we could just use a single URI argument for the two. This is a good point. The redirect information is already in the flags, we could completely get rid of one of the aRedirectFrom and call it aPreviousURI or something similar, if I'm not missing something. Shawn: I thought IHistory was supposed to be an internal API that nobody else would be implementing. This is why I thought we were moving away from nsIGlobalHistoryX. Is this not the case?

(In reply to comment #69) > Shawn: I thought IHistory was supposed to be an internal API that nobody else > would be implementing. This is why I thought we were moving away from > nsIGlobalHistoryX. Is this not the case? Embedders may choose to have their own history implementation still. But regardless, we have other reasons to be moving away from nsIGlobalHistory (see below). (In reply to comment #68) > 2 was de-facto frozen. More importantly, note that we have shims in place so > that all you have to implement is nsIGlobalHistory and the right things will > happen. > > I have no problem with adjusting the calls in docshell for the A -> B -> C case > to make it saner, honestly. It's just not clear to me that we need a new > ABI/API at the same time... Not sure what to do about the shims. However, I think making nsIGlobalHistory2::addVisit async is an API incompatible change. While the signature will not change, you could no longer do the following: history->AddURI(aTestURI, PR_FALSE, PR_TRUE, NULL); PRBool isVisited; history->IsVisited(aTestURI, &isVisited); The answer provided by isVisited will be completely non-deterministic if we make addURI asynchronous. Given that docshell even uses isVisited to determine if it needs to notify NS_LINK_VISITED_EVENT_TOPIC (which, this patch might actually break now that I think about it; we should fix that). I'm sure other consumers depend on all this being sync as well. Because of that, I don't think it's a good idea to make addURI asynchronous.

> Not sure what to do about the shims. We could have a default IHistory impl or something... But do we really need to do anything, unless we really do change APIs here? Granted, embedders will have to provide the dom::Link stuff on an IHistory impl; I don't see a good way around that. > While the signature will not change, you could no longer do the following: OK, fair point. But does it matter? API stability for history is important for the callees, not for the callers. We've already broken all callers when the default history impl is in use, and plan to break them even more, no? > Given that docshell even uses isVisited to determine if it needs to notify > NS_LINK_VISITED_EVENT_TOPIC This patch removes that code. I hadn't commented on that because I assumed that the history-side changes made up for it; I figured I'd look at that once we get the rest of the docshell stuff sorted out. Certainly if add is async then this particular caller needs updating _somehow_ to do useful things. > I'm sure other consumers depend on all this being sync as well. Oh, are we continuing to have a sync implementation of it in our history impl, but just not calling it here? I'm pretty happy trying an async API with fallback to the sync one if history doesn't support the former, I think...

(In reply to comment #71) > We could have a default IHistory impl or something... But do we really need to > do anything, unless we really do change APIs here? Granted, embedders will > have to provide the dom::Link stuff on an IHistory impl; I don't see a good way > around that. We do have one; and Thunderbird uses it as I recall (they don't build places). This patch misses that because I think it's not in mozilla-central. Maybe we should move it? For what it is worth, bsmedberg had mentioned making places a requirement moving forward which would, in fact, implement this. > OK, fair point. But does it matter? API stability for history is important > for the callees, not for the callers. We've already broken all callers when > the default history impl is in use, and plan to break them even more, no? I think it's important for both. I'm sure add-ons use this API, and not keeping the same interface means they need clunky logic to change their behavior based on the version of the application instead of calling different APIs. > This patch removes that code. I hadn't commented on that because I assumed > that the history-side changes made up for it; I figured I'd look at that once > we get the rest of the docshell stuff sorted out. I don't believe it does, but this would be easy to fix. > Oh, are we continuing to have a sync implementation of it in our history impl, > but just not calling it here? Yes, I don't think we can get rid of it in the 1.9.3 timeframe either (it'd be a lot more work that we just don't have the resources for). > I'm pretty happy trying an async API with fallback to the sync one if history > doesn't support the former, I think... We could make the stub IHistory implementation call nsIGlobalHistory2/3 like it did before I suspect.

> We could make the stub IHistory implementation call nsIGlobalHistory2/3 like it > did before I suspect. That sounds like a perfectly good plan to me.

I'll have something that addresses these comments soon.

(In reply to comment #73) > That sounds like a perfectly good plan to me. W00t! I think we have consensus. We probably should have roped you in on the plans for this sooner bz; sorry about that.

> like it did before I suspect. I don't see any code like this, where is it?

(In reply to comment #76) > I don't see any code like this, where is it? I think it might actually be in the Thunderbird tree. You should ask Standard8 since I think he implemented it.

sdwilsh/bz - who exactly is the consumer of this shunt? Can we just move the callers over to places?

(In reply to comment #78) > sdwilsh/bz - who exactly is the consumer of this shunt? Can we just move the > callers over to places? Which shunt? Two have been mentioned so far.

sorry; my point mainly is if we are going to spend time on an adapter, it would be good to consider just fixing up the callers that we know about and just killing off the old code.

The consumer of this shunt would be an embeddor who wants to use their own history impl instead of places and has one working using frozen interfaces and whom we don't want to gratuitously break. If we have to, we have to, but I don't think we're in that situation here.

The IHistory stub isn't going to work. nsIGlobalHistory3 requires the channels for the redirect, but VisitURI doesn't get those. Additionally, nsIGlobalHistory2::AddURI always wants the referrer, but VisitURI only has the previous URI which could be a referrer or a redirecting site. We'll need to add this adapter code to docshell, like we had to do for redirects and nsGlobalHistory3. I think it's going to be confusing, because IHistory and nsIGlobalHistory have subtly different APIs. GH2 always wants the referrer. IHistory only wants the previous URI. Personally, I think this is reason enough to break away from GH2 API. Boris, if you disagree could we at least move the adapter code to a new bug so this bug can get landed? It's blocking important bugs in e10s and Firefox and Fennec should enjoy performance wins from landing it.

Fixes a crash and changes the API to use just one previous URI as a parameter.

> could we at least move the adapter code to a new bug so this bug can get > landed? Can we commit to fixing that new bug before this next alpha? I've had my share of letting code land when I think it's wrong on promises of it being fixed.... and it's not fixed yet, by and large. I'm very leery of letting that happen again. Especially because it seems like all that's involved in "fixing" this is not removing the existing code.... > Personally, I think this is reason enough to break away from GH2 API. If it were not our stable embedding API, sure. But it is. If we have a very good reason to break it, we can do it. But it's not clear to me that in this case we do.

And I'll be honest, "my code is important and should land now even if it breaks other people's code" is something else we've had issues with historically. Then people walk away and abandon both the code they landed and the people they broke. I don't want that happening here.

nsIGlobalHistory is frozen but deprecated. 2 is under review. 3 is not considered frozen. I haven't really looked at how we support embedders using places, but is it better/easier for them? Is this something that we can document well and offer source code examples? Or is this a matter of principle -- we aren't ready to remove deprecated APIs? I do not know what the risks of breaking this API are as I do not know who uses it and their cost of upgrading to a new API).

> 2 is under review. And de-facto frozen. > I haven't really looked at how we support embedders using places "Not very well" if they want to have a history model other than the Places one. > Or is this a matter of principle -- we aren't ready to remove deprecated APIs? nsIGlobalHistory2 is not deprecated... > as I do not know who uses it and their cost of upgrading to a new API Well, given the 1.9.2 target on this bug, does it matter what their costs are? We shouldn't be breaking the API there, period. Past that, Thunderbird doesn't use Places. Camino doesn't, last I checked. If we're seriously claiming that IHistory should be the new embedding API, then IHistory needs some changes (for example, anyone who wants to implement history in JS can't do it with IHistory, right?).

bz, I am not claiming anything about IHistory; just trying to understand the reasons to preserve nsIGlobalHistory.

(and, i only need this bug fixed because it will make our e10s work alot easier. work scheduled for 1.9.3)

> just trying to understand the reasons to preserve nsIGlobalHistory. Because it's our existing API for people who want to use a history impl of their own, and we have no proposal for a different one that really works yet.... I understand why we want this bug fixed. I'd just like us to not break things in the process of fixing it, unless we have to.

Camino 2.1 plans to move to Places, and I've heard the same from Thunderbird (3.2?) Also, this targets 1.9.3, not 1.9.2, if there is a 1.9.2 marked somewhere it's an error. Btw, I really think my solution is simpler, just check if IHistory is available, if it is use it, otherwise fallback to the old GH2/3.

> Btw, I really think my solution is simpler, just check if IHistory is > available, if it is use it, otherwise fallback to the old GH2/3. Right, that's what I said in comment 84.

(In reply to comment #91) > Btw, I really think my solution is simpler, just check if IHistory is > available, if it is use it, otherwise fallback to the old GH2/3. Except that that makes this overly complicated docshell code even more complicated, which is a losing situation. So, if we need to support nsIGlobalHistory3 still, we'll have to have special code to call it, but for nsIGlobalHistory2 we can simply use an adapter still. Do we need to worry about nsIGlobalHistory3?

(In reply to comment #93) > (In reply to comment #91) > > Btw, I really think my solution is simpler, just check if IHistory is > > available, if it is use it, otherwise fallback to the old GH2/3. > Except that that makes this overly complicated docshell code even more > complicated, which is a losing situation. So, if we need to support > nsIGlobalHistory3 still, we'll have to have special code to call it, but for > nsIGlobalHistory2 we can simply use an adapter still. Not really true. As I said above, an IHistory adapter only knows about the previous URI, so it doesn't always know what the referrer is. For instance, in A (referrer) -> B (redirect) -> C, during B->C VisitURI is called with just B and C. AddURI also needs A. The adapter code really belongs in docshell.

> Do we need to worry about nsIGlobalHistory3? Probably not.

Comment on attachment 447265 [details] [diff] [review] Patch v11 Discussed this in-person with Ben. He's going to get a new patch up.

afaic GH3 was added for Places.

(In reply to comment #97) > afaic GH3 was added for Places. Should we just nuke it then?

I would be fine with that.

Blocking based on this blocking e10s in Fennec

OK, here's the new docshell adapter code, with all the add visit stuff in one place. I added a lot of documentation to describe what is going on.

After working on the title patch, I found I needed a more generic way of having task queues in History as opposed to just VisitURI queues. With the more generic framework also comes a fail-safe way of ensuring that tasks complete (so that the queue never gets stuck by having a task that doesn't finish). There were also some other small changes: * visituri.js test removes an unused function and slightly changes the query to be like the CPP tests, with temp table first * a AddURI call in nsGlobalHistory no longer casts to bool, instead uses !!

Can you file (and patch) a new bug to remove nsIGlobalHistory3 per comment 95. It needs to depend on bug 568612. Not going to ask you to fix that here so we can keep this bug moving, but it will be simple to fix, so I'd like to do just fix it.

Comment on attachment 447819 [details] [diff] [review] Patch v12 For a review with more context, please see http://reviews.visophyte.org/r/447819/ on file: docshell/base/nsDocShell.cpp line 10111 > IHistory* history = nsContentUtils::GetHistory(); Can't actually use this in this file. However, we should add this to our global services since we'll need to also cache it here. See bug 516085 for how to do that (we should also nuke nsContentUtils version of it too, but that can be in a follow-up which you should file). on file: docshell/base/nsDocShell.cpp line 10146 > // This is the first redirect. Register visit 1=>2 and 2=>3. nit: elsewhere for transitions, you put spaces around =>, so "1 => 2 and 2 => 3". on file: docshell/base/nsDocShell.cpp line 10162 > // N-1=>N was handled by the previous redirect. > // Register visit N=>N+1. nit: elsewhere for transitions, you put spaces around =>, so "N-1 +> N" and "N => N+1". on file: docshell/base/nsDocShell.cpp line 10187 > // N=>N+1 was handled by a redirect that just happened a moment ago. nit: N => N+1 here on file: toolkit/components/places/src/History.h line 82 > void AppendTask(class Step* aTask); nit: drop "class" on file: toolkit/components/places/src/History.cpp line 263 > class UpdateFrecencyAndNotifyStep : public Step nit: I think you have two spaces after the colon here. on file: toolkit/components/places/src/History.cpp line 729 > aTask->AddRef(); NS_ADDREF(aTask); Also, when does this get released? on file: toolkit/components/places/src/History.cpp line 815 > nextTask->Callback(NULL); you don't check the result of Callback here on file: toolkit/components/places/src/History.cpp line 850 > if (spec.Equals(data->lastSpec)) { > // Do not save refresh-page visits. > return NS_OK; > } I actually don't recall seeing a test for this; is there? on file: toolkit/components/places/src/History.cpp line 888 > (data->transitionType == nsINavHistoryService::TRANSITION_FRAMED_LINK || > data->transitionType == nsINavHistoryService::TRANSITION_EMBED || > redirected) ? 1 : 0; nit: data and redirect should be inline with "data" on the first line here (not even with the opening paren) on file: toolkit/components/places/tests/browser/browser_visituri.js line 11 > function waitForObserve(name, callback) { nit: (commenting once) opening brace is supposed to be on a new line for function on file: toolkit/components/places/tests/browser/browser_visituri.js line 74 > subject = subject.QueryInterface(Ci.nsIURI); do let uri = subject.QueryInterface(Ci.nsIURI). Subject isn't clear as to what it actually is. Alternatively, rename subject to "aURI", and on the first line do |do_check_true(aURI instanceof Ci.nsIURI)| Should also probably check that the topic is what you expect for sanity's sake. on file: toolkit/components/places/tests/browser/browser_visituri.js line 83 > } else { nit: else after } (commenting once) on file: toolkit/components/places/tests/browser/visituri/begin.html line 6 > <a id="clickme" href="redirect_twice.sjs">Redirect twice</a> we generally like to have our test cases be valid html (with a head and body) unless we are testing invalid html specifically r=sdwilsh with these changes

> how to do that (we should also nuke nsContentUtils version of it too, but that > can be in a follow-up which you should file). > nit: drop "class" Can't. Step is defined in History.cpp. This is fine because it's only expected to be used by internal stuff. > Also, when does this get released? See right below. Dead task walking. Patch soon.

On vacation for the next week!

I'm using dougt patch queue And got this crash: http://pastebin.mozilla.org/729625 #5 Callback (this=0xacf6c360, aResultSet=0x0) at toolkit/components/places/src/History.cpp:764 #6 0xb6c0ca4c in mozilla::places::Step::HandleCompletion (this=0xacf6c360, aReason=0) at toolkit/components/places/src/History.cpp:151

Ah... and History::SetURITitle(nsIURI* aURI, const nsString& aTitle) from bug 566738 nsNavHistory::GetHistoryService() return NULL... Is something missing in my environment ?

Different bug Oleg. See 568925. I do not think that is in my queue now. If it isn't, please add it if you like.

Comment on attachment 448109 [details] [diff] [review] Patch v13 The mRedirectOccurred handling here is still broken. Nothing guarantees that OnRedirectStateChange will be followed by OnNewURI (e.g. it could be a redirect to a resource we don't render in a docshell at all). I'd really appreciate actually answering my question as to why this boolean is actually needed...

The flag is there because our VisitURI call works differently than before. VisitURI wants the order of visits in sequence. By the time OnNewURI is called it may or may not need to call VisitURI depending on whether a redirect has occurred. See the extensive comments here: https://bugzilla.mozilla.org/attachment.cgi?id=448109&action=diff#a/docshell/base/nsDocShell.cpp_sec8 If you have a better idea than a state flag, suggestions are welcome. Otherwise, I need help figuring out how to make sure this flag is reset after the request has been followed to its final destination.

Would the LOAD_REPLACE flag on the channel do what you want (i.e. indicate that it's the result of a redirect)? Especially when combined with a check for GetURI != GetOriginalURI? For HTTP it should. For multipart responses... well, what do you expect history to do in those cases anyway? Another option is to stash something in the channel's property bag to indicate that you already added the visit for it. Another option is to catch cases when the channel is removed from your loadgroup and reset the redirect flag then.

Looks promising! Will the flag on the channel already be set when docshell's OnRedirectStateChange is called? I need to know if it's the *first* redirect as well.

> Will the flag on the channel already be set when docshell's > OnRedirectStateChange is called? Yes. I guess the GetURI != GetOriginalURI thing might not be right in the case of redirects from proxied to unproxied and in cases of redirect to self, though... So explicitly flagging things via property bag might be better.

Why is the GetURI check necessary? It seems like LOAD_REPLACE is enough.

Uses LOAD_REPLACE instead of adding new flag.

> It seems like LOAD_REPLACE is enough. If it were, I would have just said to use it. But it's also set on various internal redirect results (especially the ones that accompany proxy autodetect and the like). That might be an HTTP bug, but unless we're planning to change that behavior we can't rely on just that flag being set. Hence comment 115.

In other words, your code will get bogus true values of aRedirectOccurred in AddURIVisit as called from OnNewURI. Is that acceptable? It looks to me like it will fail to call VisitURI at all in those situations (since aRedirectFromURI will be null), no? Oh, and as long as we're there please don't use optional arguments for AddURIVisit. Just pass the arguments at all callsites.

I guess the reason you need the redirect-occurred stuff is because a channel doesn't know where it was redirected _from_, so you can't just register redirect-related visits in OnNewURI and the redirect observer? Perhaps a better approach would in fact be to assume the property bag thing and just stash the URI the channel was redirected from on the property bag in the redirect observer? Then the redirect observer could add the visit for the pre-redirect URI and OnNewURI could add it for the post-redirect one. That would incidentally not flag the redirect target as visited if the site can't be loaded (e.g. DNS error), right? Which would be consistent with how the non-redirect case is handled...

> If it were, I would have just said to use it. But it's also set on various > internal redirect results (especially the ones that accompany proxy autodetect > and the like). That might be an HTTP bug, but unless we're planning to change > that behavior we can't rely on just that flag being set. If I understand correctly, VisitURI doesn't see any of the internal redirect stuff. See this check in OnRedirectStateChanged: > if (!(aRedirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL)) {

(In reply to comment #121) > If I understand correctly, VisitURI doesn't see any of the internal redirect > stuff. See this check in OnRedirectStateChanged: It certainly isn't meant to (I don't see why any history implementation would ever care about redirects that only matter to the channel implementation).

> If I understand correctly, VisitURI doesn't see any of the internal redirect Right, precisely. So if you do a channel loading A which does an internal redirect to B (same url, for http, fwiw), then the latest patch wouldn't record the visit at all. See comment 119 paragraph one.

Uses property bag to propagate URIs and redirect flags. I found this a little challenging to describe (especially during redirect), so please read the comments and see if you follow. Also, redirects now correctly checks and ignores POSTs.

--> beta2+, e10s-in-fennec isn't a hard beta1+ blocker

Comment on attachment 449959 [details] [diff] [review] Patch v15 Instead of having (inconsistent) |previousFlags ? :| stuff at AddURIVisit callsites, can you just push that down into AddURIVisit? You only need nsIPropertyBag2 in ExtractLastVisit, not nsIWritablePropertyBag2. The code in ExtractLastVisit doesn't make sense to me toward the end. How can *aURI be null there? I don't see any places where you write null to the property bag while also writing nonzero flags. Am I missing something? And if nothing is in the property bag, then GetPropertyAsInterface has already returned an error. Is this code tested? What's it trying to do?

> Instead of having (inconsistent) |previousFlags ? :| stuff at AddURIVisit > callsites, can you just push that down into AddURIVisit? Can do. > The code in ExtractLastVisit doesn't make sense to me toward the end. How can > *aURI be null there? I don't see any places where you write null to the > property bag while also writing nonzero flags. Am I missing something? And if > nothing is in the property bag, then GetPropertyAsInterface has already > returned an error. Is this code tested? What's it trying to do? In fact, that is a bug in ExtractLastVisit. The tests pass because AddURIVisit is always given the referrer from the channel, not the previous URI from the property bags.

> In fact, that is a bug in ExtractLastVisit. Does that mean we can remove the !*aURI block?

> Does that mean we can remove the !*aURI block? Yes. I am moving it to the first NS_FAILED block as we speak.

OK! Comments addressed! I also added a warning in extract last visit in case previousFlags could not be found in the property bag.

Comment on attachment 454140 [details] [diff] [review] Patch v16 sr=bzbarsky

Occasionally a rogue visit will still be in the queue when shutting down (this seems to happen a lot with leak detection builds on tinderbox). We had to back out the patch. This new patch does the responsible thing and removes all references to visit steps that it owns. It also guarantees that no new tasks are begun (this is necessary because when tasks are destroyed, they try to start the next task!).

Missed a check in CurrentTaskFinished. Since it does not peek at the front of the queue when it pops (the task that has finished has just been popped so it may be empty), it needed a check. I also added an assertion to make sure there is something in the queue. CurrentTaskFinished under normal circumstances should *never* be called when empty.

imo, this is still shutting down too late, Places shuts down at profile-before-change and fires a "places-shutdown" notification, then clear history on shutdown could run. This piece of code instead keeps going on till the object is destroyed, that means far later and it could add visits after we run clear history on shutdown. It should not wait destruction, rather observe places-shutdown notification, and cleanup there.

Use places-shutdown event to clean up and assert that destructor is only called after cleanup.

Comment on attachment 455174 [details] [diff] [review] Leak fix v3 >+++ b/toolkit/components/places/src/History.cpp > History::History() >+: mShuttingDown(false) > { > NS_ASSERTION(!gService, "Ruh-roh! This service has already been created!"); > gService = this; >+ >+ nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService(); >+ NS_ASSERTION(os, "Observer service was not found!"); >+ (void)os->AddObserver(this, TOPIC_PLACES_SHUTDOWN, PR_FALSE); We should be safer here and do this inside an if block like we do in nsNavHistory. > History::~History() > { > gService = NULL; >+ > #ifdef DEBUG > if (mObservers.IsInitialized()) { > NS_ASSERTION(mObservers.Count() == 0, > "Not all Links were removed before we disappear!"); > } > #endif >+ >+ NS_ASSERTION(mShuttingDown, "Received places shutdown event"); Move this inside the ifdef DEBUG block, and also assert that our deque size (via GetSize) is zero at this point like we do with mObservers. >+void >+History::AppendTask(Step* aTask) >+{ Shouldn't we be checking mShuttingDown here too? >+NS_IMETHODIMP >+History::Observe(nsISupports* aSubject, const char* aTopic, >+ const PRUnichar* aData) nit: style is to have each argument on its own line. >+{ >+ if (strcmp(aTopic, TOPIC_PLACES_SHUTDOWN) == 0) { >+ mShuttingDown = true; >+ >+ NS_WARN_IF_FALSE(!mPendingVisits.PeekFront(), "Tasks were not completed :("); Looks like based on your message you were going to say more but you don't. Either say more, or fix the message. This also needs a test that adds something to history, and then QI's this class to nsIObserver and sends the places shutdown topic. The failure condition is that we'd leak, success is that we don't. You can post the test up in a separate patch and mak or I can review it. r=sdwilsh

Comment on attachment 455174 [details] [diff] [review] Leak fix v3 > History::~History() > { > gService = NULL; >+ > #ifdef DEBUG > if (mObservers.IsInitialized()) { > NS_ASSERTION(mObservers.Count() == 0, > "Not all Links were removed before we disappear!"); > } > #endif >+ >+ NS_ASSERTION(mShuttingDown, "Received places shutdown event"); the message should be opposite, since it's the message printed if we did not receive the topic. something like "Going away before Places shutdown notification?" sounds better. >+void >+History::StartNextTask() >+{ >+ if (mShuttingDown) { >+ // Release tasks only, don't start new ones. >+ return; >+ } the comment is a bit off-topic since we are not releasing anything here it's just an early return, I'd vote for a better comment. > NS_IMETHODIMP >+History::VisitURI(nsIURI* aURI, >+ nsIURI* aLastVisitedURI, >+ PRUint32 aFlags) >+{ >+ NS_PRECONDITION(aURI, "URI should not be NULL."); >+ NS_PRECONDITION(!mShuttingDown, "Shutting down but adding visits"); hm, actually this mShuttingDown assertion could give you and us headaches, it is possible that Docshell will call VisitURI after Places shutdown, since it's independent from history implementation. This will often happen in tests that add visit, check something and close in a bunch of milliseconds, and we don't want oranges due to this assertion. I think you should just bail out and refuse to add the visit. Not sure if docshell is ready to get a failure from VisitURI, but it should be. A return NS_ERROR_NOT_AVAILABLE sounds like the best solution to me. >+ NS_WARN_IF_FALSE(!mPendingVisits.PeekFront(), "Tasks were not completed :("); "History tasks were..." r- just for the possibility the assertion could cause oranges.

You will also probably need to handle mShuttingDown in setURITitle patch.

Comment on attachment 455253 [details] [diff] [review] Review comments addressed > History::~History() > { > gService = NULL; >+ > #ifdef DEBUG > if (mObservers.IsInitialized()) { > NS_ASSERTION(mObservers.Count() == 0, > "Not all Links were removed before we disappear!"); > } >+ >+ NS_ASSERTION(mShuttingDown, "Did not receiv places shutdown event"); nit: typo "receiv" and Places is an uppercase name > NS_IMETHODIMP >+History::VisitURI(nsIURI* aURI, >+ nsIURI* aLastVisitedURI, >+ PRUint32 aFlags) >+{ >+ NS_PRECONDITION(aURI, "URI should not be NULL."); >+ if (mShuttingDown) { >+ return; >+ } does this really compile? it should expect a nsresult return value. > //////////////////////////////////////////////////////////////////////////////// >+//// nsIObserver >+ >+NS_IMETHODIMP >+History::Observe(nsISupports* aSubject, const char* aTopic, >+ const PRUnichar* aData) >+{ >+ if (strcmp(aTopic, TOPIC_PLACES_SHUTDOWN) == 0) { >+ mShuttingDown = true; >+ >+ >+ >+ // History is going away, so abandon tasks. what is all of that empty space?

http://hg.mozilla.org/mozilla-central/rev/f9a700607b86 This is a small changeset fixing assertions firing when a data object is created but we bail out before adding a step for it. Also the assertion on shutdown can fire when the process is initially started and restarted (this happens in b-c tests for example). http://hg.mozilla.org/mozilla-central/rev/f666976446db I prioritized the fact we needed this on central because it was blocking mobile and to check all functionality is fine, we can cleanup and fix minor issues in code at any time starting from that, feel free to file follow-ups bugs if you notice something we should revise.

Is this patch responsible for the huge (800%, 900%, etc) Ts Shutdown performance regressions?

(In reply to comment #144) > Is this patch responsible for the huge (800%, 900%, etc) Ts Shutdown > performance regressions? I have filed bug 576590. I think it could be related, yes.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:2.0b2pre) Gecko/20100702 Minefield/4.0b2pre ID:20100702111754 maybe after this checkin, uncheck "Options>Privacy>>Remember my browsing history", but browsing history is listed under "History" menu. before this checkin, no history is listed under the menu. intended ?

(In reply to comment #143) > http://hg.mozilla.org/mozilla-central/rev/f9a700607b86 > http://hg.mozilla.org/mozilla-central/rev/f666976446db I've backed these two out (along with bug 566738) due to one of them likely being the cause of bug 576684

Backouts: http://hg.mozilla.org/mozilla-central/rev/4497942b0e3d http://hg.mozilla.org/mozilla-central/rev/920385221b56

Mozilla/5.0 (Windows; Windows NT 6.1; en-US; rv:2.0b2pre) Gecko/20100716 Minefield/4.0b2pre ID:20100716040830 (In reply to comment #146) > Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:2.0b2pre) Gecko/20100702 > Minefield/4.0b2pre ID:20100702111754 > > maybe after this checkin, > uncheck "Options>Privacy>>Remember my browsing history", but browsing history > is listed under "History" menu. > before this checkin, no history is listed under the menu. > intended ? again. intended or not ??

Existing history should be shown in the menu, new visits should not be added imo, no reasons to give a false sense of "we removed all of your history" since it's not true. You should have filed a bug, comments are most likely to be forgotten or to go unnoticed. Actually I can't figure out the relation between history menu and this bug though.

(In reply to comment #153) > Existing history should be shown in the menu, new visits should not be added > imo, no reasons to give a false sense of "we removed all of your history" since > it's not true. > You should have filed a bug, comments are most likely to be forgotten or to go > unnoticed. > Actually I can't figure out the relation between history menu and this bug > though. new visits are added. before this checkin, no history is listed. after this checkin, history is listed. so I think there is a relation.

So, if you uncheck "Remember my browsing history" and visit a page, a new visit is added to history menu? please file a regression bug if so. If _old_ history is shown in the menu I don't care much, that data is not removed so it's fine to show it.

at first glance we should just make canAddURI check IsHistoryDisabled, and remove duped checks around code.

(In reply to comment #155) > So, if you uncheck "Remember my browsing history" and visit a page, a new visit > is added to history menu? yes. no history, (Changeset:8a745dc12044) http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1279227245/ history listed, (Changeset:92339b84d089) http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1279230855/

ok I filed Bug 579334

(In reply to comment #158) > ok I filed Bug 579334 thanks.