Closed Bug 416313 Opened 16 years ago Closed 16 years ago

Define the identity of the visit_count column

Categories

(Firefox :: Bookmarks & History, defect, P2)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 3

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 6 obsolete files)

patch
44.82 KB, patch
Details | Diff | Splinter Review 
In moz_places we have a visit_count column, that *should* be used to have the count of valid visits (eg visits NOT IN (0,4))

Problem there is that visit_count is not in synch with moz_historyvisits real count, and we don't clear visit_count on clear private data because we use it for frecency recalculation on idle. 
The same frecency recalculation can't rely on visit_count because it does not contain all visits.
So we can't rely on that value since it's not resetted, and that could be a privacy complain for some user (why is my visit_count not resetted on clear private data?) even if that's really affect those pages that are bookmarks or have some expire_never annotation.
Moreover we also have query option maxVisits and minVisits that rely on visit_count, but again that is not always guaranteed to be in synch (we also have some glitches there since user expect it to contain all visits, but that's not true, see Bug 415716)
Moreover we use visit_count to get fast most_visited query, but bookmarks sites always appear in the query even if unvisited (after a cpd).

So what we should do is take a decision on the contents of visit_count.
We should state if it should contain all visits (also embedded) to be used in frecency calc and in maxVisits query.
We should check that it is mantained in synch when we delete any visit (through trigger or by backend code).

for frecency recalculation in idle i suggest moving to a different approach, if i'm not wrong now we are using visit count to see what frecency should be calculated before and we use -1 to identify a non valid frecency. We could instead set frecency = -visit_count, and reset visit_count = 0, then an invalid visit_count will be <0 while to decide on which frecency to rebuild first we could sort by frecency <0 ASC
for future reference, fixing visit_count could change most visited query (see Bug 416324)
Blocks: 415716
my suggestion is to attach a trigger to moz_historyvisits, and let the trigger mantain visit_count in sync, at the same time we should do a one time recalculation of all visit_count to start from a reliable value.

Frecency should detach from visit_count to do recalculation on idle, and use instead the purposed method (frec=-visit_count)

about having all visits or only valid visits in visit_count it's a choice that in both cases will create some pro and some cons, so i'd go for the actual behaviour of not counting 0, 4(embed), 7(download)? (bug 412217)
cases in which most probably visit_count will go out of sync:
- clear private data
- batch delete of visits (so delete in sidebar for example)
Blocks: 394133
This also breaks link coloring: nsNavHistory::IsVisited() uses moz_places.visit_count, which means that because that isn't cleared when history is cleared, unvisited+bookmarked links in content are colored as visited.
Drivers: see comment #4. At the very least we need to fix consumers of moz_places.visit_count who believe it to be real, to go to the moz_historyvisits table instead.

For the proper fix, Marco's solution in comment #2 sounds fine.
Flags: blocking-firefox3?
Priority: -- → P2
Target Milestone: --- → Firefox 3 beta4
Assignee: nobody → mak77
Flags: blocking-firefox3? → blocking-firefox3+
Priority: P2 → P3
When you make the change, be sure to add dev-doc-needed
Target Milestone: Firefox 3 beta4 → Firefox 3
Blocks: 419957
Status: NEW → ASSIGNED
Attached patch wip v3 (obsolete) — — Splinter Review 
this compiles and execute, still failing on test_000_frecency, but i still need to check tests (they are using setpagedetails that should not really touch the visit_count) and revisit all the code i've changed (and i should write a dedicated test that will take in count all our insert / delete / batch remove functions).

While we are here it's the right time to talk about what we can do with these triggers. I'm actually creating 2 triggers on moz_historyvisits, AFTER INSERT and AFTER DELETE. If we need/want to put some new rule there we could/should do that now, because in future we could update them, but will be more difficult than doing that now (i'm however calling them TABLENAME_TRIGGERACTION_VERSION_trigger so we will always be able to recognize and replace them).

SO, if we want to:

- add a last_visit_date column to remove the actual MAX fragment (we will not gain much since actual fragment is quite fast, but still would help readability)
- add a full_visit_count column to count other visits (could be used by frecency or by third party if they want full visit count)
- invalidate | set frecency dinamically for every visit add/removal (i don't know the full algo, probably is not so easy, are there fixed paths that are pure sql and must be repeated for every visit removal?)
- add a lastModified column to moz_places (could be used for sync or to detect all places modified by a batch query on visits, so you set a starting time, do a batch removal of history and with a query you know which places have been modified)
- other jobs based on INSERT/DELETE in moz_historyvisits (ideas?)
Priority: P3 → P4
(In reply to comment #7)

> While we are here it's the right time to talk about what we can do with these
> triggers. I'm actually creating 2 triggers on moz_historyvisits, AFTER INSERT
> and AFTER DELETE. If we need/want to put some new rule there we could/should do
> that now, because in future we could update them, but will be more difficult
> than doing that now (i'm however calling them
> TABLENAME_TRIGGERACTION_VERSION_trigger so we will always be able to recognize
> and replace them).

hard, but not very hard. even so, for post-firefox3 it's a high priority that we get a decent api for database versioning, to make migrations such as these easier.

regarding the list below: this late in the dev cycle, anything not absolutely required to fix P1 issues is a very low priority.

> - invalidate | set frecency dinamically for every visit add/removal (i don't
> know the full algo, probably is not so easy, are there fixed paths that are
> pure sql and must be repeated for every visit removal?)

hm, could call out to user function here.

> - other jobs based on INSERT/DELETE in moz_historyvisits (ideas?)
> 

much of the expiration work is pure sql, could be applicable.

Comment on attachment 306411 [details] [diff] [review]
wip v3


>+  rv = CreateTriggers();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  

nit: whitespace

>+// nsNavHistory::CreateTriggers
>+//
>+//    this creates our triggers
>+
>+nsresult
>+nsNavHistory::CreateTriggers()
>+{
>+  nsresult rv;
>+  mozStorageTransaction createTriggersTransaction(mDBConn, PR_FALSE);

why not open and commit the transaction inside the |if| block?

>+
>+  // we are creating 2 triggers on moz_historyvisits to mantain

s/mantain/maintain/

>+  // moz_places.visit_count in sync with moz_historyvisits, for this
>+  // to work we must ensure that all visit_count values are correct
>+  // See bug 416313 for details
>+  nsCOMPtr<mozIStorageStatement> detectVisitCountTrigger;
>+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+      "SELECT name FROM sqlite_master WHERE type = 'trigger' AND "
>+      "name = 'moz_historyvisits_afterinsert_v1_trigger'"),
>+    getter_AddRefs(detectVisitCountTrigger));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool hasTrigger;
>+  rv = detectVisitCountTrigger->ExecuteStep(&hasTrigger);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = detectVisitCountTrigger->Reset();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!hasTrigger) {
>+    // do a one-time reset of all the visit_count values
>+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+      "UPDATE moz_places SET visit_count = "
>+      "(SELECT count(*) FROM moz_historyvisits WHERE place_id = moz_places.id)"));
>+    NS_ENSURE_SUCCESS(rv, rv);

why not filtering on known not-wanted visit types?

>+
>+    // moz_historyvisits_afterinsert_v1_trigger
>+    // increment visit_count by 1 for each inserted visit
>+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+      "CREATE TRIGGER IF NOT EXISTS moz_historyvisits_afterinsert_v1_trigger "
>+      "AFTER INSERT ON moz_historyvisits FOR EACH ROW "
>+      "WHEN NEW.visit_type NOT IN (0,4,7) "

hm, maybe the visit types filtered should be defined outside of the query, in case we want to tweak them. 

>@@ -989,28 +1057,30 @@ nsNavHistory::InitStatements()
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>       "SELECT id, visit_count, typed, hidden "
>       "FROM moz_places "
>       "WHERE url = ?1"),
>     getter_AddRefs(mDBGetPageVisitStats));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // mDBUpdatePageVisitStats (see InternalAdd)
>+  // we don't need to update visit_count since it's mantained

nit: spelling s/mantained/maintained/

>@@ -1091,17 +1161,17 @@ nsNavHistory::InitStatements()
>       "ON r.id = v.from_visit AND v.visit_type IN ") +
>       nsPrintfCString("(%d,%d) ", TRANSITION_REDIRECT_PERMANENT,
>       TRANSITION_REDIRECT_TEMPORARY) + NS_LITERAL_CSTRING(
>     "WHERE v.place_id = ?1 ORDER BY date DESC LIMIT ") +
>      nsPrintfCString("%d", mNumVisitsForFrecency),
>     getter_AddRefs(mDBVisitsForFrecency));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  // find places with invalid frecencies (frecency = -1)
>+  // find places with invalid frecencies (frecency < 0)

why this change?

>@@ -1628,34 +1690,29 @@ nsNavHistory::InternalAddNewPage(nsIURI*
>   // hidden
>   rv = mDBAddNewPage->BindInt32Parameter(3, aHidden);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // typed
>   rv = mDBAddNewPage->BindInt32Parameter(4, aTyped);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  // visit count
>-  rv = mDBAddNewPage->BindInt32Parameter(5, aVisitCount);
>-  NS_ENSURE_SUCCESS(rv, rv);

can you check the path of history import from branch, and confirm that it doesn't need to set visit count before changing these apis?

>Index: toolkit/components/places/src/nsNavHistoryExpire.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v
>retrieving revision 1.42
>diff -u -8 -p -r1.42 nsNavHistoryExpire.cpp
>--- toolkit/components/places/src/nsNavHistoryExpire.cpp	25 Feb 2008 21:35:57 -0000	1.42
>+++ toolkit/components/places/src/nsNavHistoryExpire.cpp	29 Feb 2008 00:42:37 -0000
>@@ -239,19 +239,29 @@ nsNavHistoryExpire::OnQuit()
> //    for clear history cases. However, my initial tests show that the
> //    notifications are not a significant part of clear history time.
> 
> nsresult
> nsNavHistoryExpire::ClearHistory()
> {
>   mozIStorageConnection* connection = mHistory->GetStorageConnection();
>   NS_ENSURE_TRUE(connection, NS_ERROR_OUT_OF_MEMORY);
>+  nsresult rv;
>+
>+  // reset frecency for all items that will not be deleted
>+  // Note, we set frecency to -visit_count since we use that value in our
>+  // idle query to figure out which places to recalcuate frecency first.
>+  // We must do this before deleting visits
>+  rv = connection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+    "UPDATE moz_places SET frecency = -visit_count WHERE "));
>+  if (NS_FAILED(rv))
>+    NS_WARNING("failed to recent frecency");

missing a word here?

>   // Reset the frecencies for the places that don't have any more visits after
>   // we deleted them and make sure they aren't bookmarked either. This means we
>   // keep the old frecencies when possible as an estimate for the new frecency
>   // unless we know it has to be -1.
>-  rv = aConnection->ExecuteSimpleSQL(
>+  // We must do this before deleting visits
>+  nsresult rv = aConnection->ExecuteSimpleSQL(
>     NS_LITERAL_CSTRING(
>       "UPDATE moz_places "
>-      "SET frecency = -1 "
>+      "SET frecency = -visit_count "
>       "WHERE id IN ("
>         "SELECT h.id FROM moz_places h "
>         "LEFT OUTER JOIN moz_historyvisits v ON v.place_id = h.id "
>         "LEFT OUTER JOIN moz_bookmarks b ON b.fk = h.id "
>         "WHERE v.id IS NULL AND b.id IS NULL AND h.id IN (") +
>     placeIds +
>     NS_LITERAL_CSTRING("))"));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  rv = aConnection->ExecuteSimpleSQL(
>+    NS_LITERAL_CSTRING("DELETE FROM moz_historyvisits WHERE id IN (") +
>+    deletedVisitIds +
>+    NS_LITERAL_CSTRING(")"));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (placeIds.IsEmpty())
>+    return NS_OK;
>+
>   return NS_OK;

hm?
Priority: P4 → P3
(In reply to comment #9)
> >+nsresult
> >+nsNavHistory::CreateTriggers()
> >+{
> >+  nsresult rv;
> >+  mozStorageTransaction createTriggersTransaction(mDBConn, PR_FALSE);
> 
> why not open and commit the transaction inside the |if| block?

i thought to this function as a general trigger creation one, so having the transaction starting first, and closing last. But probably it would be better to have one transaction per creation.

> >+    // do a one-time reset of all the visit_count values
> >+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> >+      "UPDATE moz_places SET visit_count = "
> >+      "(SELECT count(*) FROM moz_historyvisits WHERE place_id = moz_places.id)"));
> >+    NS_ENSURE_SUCCESS(rv, rv);
> 
> why not filtering on known not-wanted visit types?

yeah, should do that!

> >+    // moz_historyvisits_afterinsert_v1_trigger
> >+    // increment visit_count by 1 for each inserted visit
> >+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> >+      "CREATE TRIGGER IF NOT EXISTS moz_historyvisits_afterinsert_v1_trigger "
> >+      "AFTER INSERT ON moz_historyvisits FOR EACH ROW "
> >+      "WHEN NEW.visit_type NOT IN (0,4,7) "
> 
> hm, maybe the visit types filtered should be defined outside of the query, in
> case we want to tweak them. 

could be, but it will make no difference, once the trigger is created the only way to tweak that is removing trigger, recalculate everything and create a new one. there's not easier path

> >@@ -1091,17 +1161,17 @@ nsNavHistory::InitStatements()
> >       "ON r.id = v.from_visit AND v.visit_type IN ") +
> >       nsPrintfCString("(%d,%d) ", TRANSITION_REDIRECT_PERMANENT,
> >       TRANSITION_REDIRECT_TEMPORARY) + NS_LITERAL_CSTRING(
> >     "WHERE v.place_id = ?1 ORDER BY date DESC LIMIT ") +
> >      nsPrintfCString("%d", mNumVisitsForFrecency),
> >     getter_AddRefs(mDBVisitsForFrecency));
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >-  // find places with invalid frecencies (frecency = -1)
> >+  // find places with invalid frecencies (frecency < 0)
> 
> why this change?

because when invalidating frecency we are setting it to -visit_count

> >@@ -1628,34 +1690,29 @@ nsNavHistory::InternalAddNewPage(nsIURI*
> >   // hidden
> >   rv = mDBAddNewPage->BindInt32Parameter(3, aHidden);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >   // typed
> >   rv = mDBAddNewPage->BindInt32Parameter(4, aTyped);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >-  // visit count
> >-  rv = mDBAddNewPage->BindInt32Parameter(5, aVisitCount);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> 
> can you check the path of history import from branch, and confirm that it
> doesn't need to set visit count before changing these apis?

will do

> >+  // reset frecency for all items that will not be deleted
> >+  // Note, we set frecency to -visit_count since we use that value in our
> >+  // idle query to figure out which places to recalcuate frecency first.
> >+  // We must do this before deleting visits
> >+  rv = connection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> >+    "UPDATE moz_places SET frecency = -visit_count WHERE "));
> >+  if (NS_FAILED(rv))
> >+    NS_WARNING("failed to recent frecency");
> 
> missing a word here?

MIA :(

> >+  if (placeIds.IsEmpty())
> >+    return NS_OK;
> >+
> >   return NS_OK;
> 
> hm?

that's a wip :)

thank you for the temp review!
Attached patch wip v4 (obsolete) — — Splinter Review 
Mardak, this is the first working version, if you could take a look at the frecency changes, that would be great
Attachment #306411 - Attachment is obsolete: true
Attachment #307488 - Flags: review?(edilee)
Comment on attachment 307488 [details] [diff] [review]
wip v4

>+  // we are creating 2 triggers on moz_historyvisits to maintain
>+  // moz_places.visit_count in sync with moz_historyvisits, for this
>+  // to work we must ensure that all visit_count values are correct
nit: wrap at 80

>+  PRBool hasTrigger;
>+  rv = detectVisitCountTrigger->ExecuteStep(&hasTrigger);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = detectVisitCountTrigger->Reset();
>+  NS_ENSURE_SUCCESS(rv, rv);
The reset shouldn't be necessary, it'll reset on destruct. But EIBTI.

>+      "CREATE TRIGGER IF NOT EXISTS moz_historyvisits_afterinsert_v1_trigger "
Shouldn't need the IF NOT EXISTS, but we can be safe.
..
>+      "CREATE TRIGGER IF NOT EXISTS moz_historyvisits_afterdelete_v1_trigger "
Same.. well.. I suppose someone could manually delete the trigger..

>+    rv = createTriggersTransaction.Commit();
This should automatically commit on exiting the if's block.
>+  }
>+
>+  NS_ENSURE_SUCCESS(rv, rv);
Either put this inside the if with the Commit or get rid of it.

>-  // for every moz_place that has an invalid frecency (-1) and
>+  // for every moz_place that has an invalid frecency (<0) and
nit: < 0

> nsNavHistory::RemovePagesInternal(const nsCString& aPlaceIdsQueryString)
..
>+  nsresult rv;
nit: define nsresult rv with the ExecuteSimpleSQL below
..
>+  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+      "UPDATE moz_places SET frecency = -MAX(visit_count, 1) WHERE id IN(") +
>+        aPlaceIdsQueryString +
>+        NS_LITERAL_CSTRING(")"));
nit: Could put the ending ")" on the same line as aPlacesIdsQueryString

But then potentially we should be doing this (like the statement right below this one)

>         NS_LITERAL_CSTRING(") AND "
>         "NOT EXISTS (SELECT b.id FROM moz_bookmarks b WHERE b.fk = h.id) AND "
>         "NOT EXISTS (SELECT a.id FROM moz_annos a WHERE a.place_id = h.id))"));
So that it doesn't unnecessarily update frecencies for pages that will be removed.

Should we just do frecency = -visit_count - 1? It's an approximation but saves on the MAX, but either way should be fine.

> nsresult
> nsNavHistory::AddPageWithVisit(nsIURI *aURI,
>                                const nsString &aTitle,
>-                               PRBool aHidden, PRBool aTyped,
>+                               PRBool aHidden,
>+                               PRBool aTyped,
>                                PRInt32 aVisitCount,
>                                PRInt32 aLastVisitTransition,
>                                PRTime aLastVisitDate)
So what should we do with the aVisitCount when importing history? Just trust it and removing all history visits might not drop visit_count to 0? Similarly, what should be done with RemoveDuplicateURIs which combine visit counts..

>+++ toolkit/components/places/src/nsNavHistoryAutoComplete.cpp	5 Mar 2008 15:58:41 -0000
>- * But places with frecency (-1) are included, as that means that these items
>+ * But places with frecency <0 are included, as that means that these items
nit: < 0

>+  nsresult rv;
nit: combine
>+  rv = connection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+    "UPDATE moz_places SET frecency = -visit_count WHERE id IN("
>+      "SELECT h.id FROM moz_places h WHERE "
>+      "EXISTS (SELECT id FROM moz_bookmarks WHERE fk = h.id) OR "
>+      "EXISTS (SELECT id FROM moz_annos WHERE place_id = h.id AND expiration = ") +
>+      nsPrintfCString("%d", nsIAnnotationService::EXPIRE_NEVER) +
>+      NS_LITERAL_CSTRING(")"));
>+  if (NS_FAILED(rv))
>+    NS_WARNING("failed to recent frecency");
frecency = -visit_count - 1 ?


>@@ -552,42 +558,40 @@ nsNavHistoryExpire::EraseVisits(mozIStor
..
>+  nsresult rv = aConnection->ExecuteSimpleSQL(
>     NS_LITERAL_CSTRING(
>       "UPDATE moz_places "
>-      "SET frecency = -1 "
>+      "SET frecency = -visit_count "
frecency = -visit_count - 1 ?
>       "WHERE id IN ("
>         "SELECT h.id FROM moz_places h "
>         "LEFT OUTER JOIN moz_historyvisits v ON v.place_id = h.id "
>         "LEFT OUTER JOIN moz_bookmarks b ON b.fk = h.id "
>         "WHERE v.id IS NULL AND b.id IS NULL AND h.id IN (") +
>     placeIds +
>     NS_LITERAL_CSTRING("))"));
>   NS_ENSURE_SUCCESS(rv, rv);

>+++ toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js	5 Mar 2008 15:58:44 -0000
..
>-  histsvc.setPageDetails(uri1, "foo title", 0, false, true);
>+  histsvc.setPageDetails(uri1, "foo title", false, true);
>   histsvc.addVisit(uri2, Date.now() * 1000, null, histsvc.TRANSITION_TYPED, false, 0);
>   histsvc.setPageDetails(uri2, "bar title", 0, false, true);
How about the second setPageDetails?
Whiteboard: [swag: 0.5d]
Attached patch v5 (obsolete) — — Splinter Review 
(In reply to comment #12)
> (From update of attachment 307488 [details] [diff] [review])
> >+  // we are creating 2 triggers on moz_historyvisits to maintain
> >+  // moz_places.visit_count in sync with moz_historyvisits, for this
> >+  // to work we must ensure that all visit_count values are correct
> nit: wrap at 80

hum, that are 68...

> >+  PRBool hasTrigger;
> >+  rv = detectVisitCountTrigger->ExecuteStep(&hasTrigger);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  rv = detectVisitCountTrigger->Reset();
> >+  NS_ENSURE_SUCCESS(rv, rv);
> The reset shouldn't be necessary, it'll reset on destruct. But EIBTI.

yes i wanted to free the resultset

> >+      "CREATE TRIGGER IF NOT EXISTS moz_historyvisits_afterinsert_v1_trigger "
> Shouldn't need the IF NOT EXISTS, but we can be safe.
> ..
> >+      "CREATE TRIGGER IF NOT EXISTS moz_historyvisits_afterdelete_v1_trigger "
> Same.. well.. I suppose someone could manually delete the trigger..

i always prefer to leave that since it comes at almost no cost, and i'm checking the existance of the first to create both

> >+    rv = createTriggersTransaction.Commit();
> This should automatically commit on exiting the if's block.
> >+  }
> >+
> >+  NS_ENSURE_SUCCESS(rv, rv);
> Either put this inside the if with the Commit or get rid of it.
> 
> >-  // for every moz_place that has an invalid frecency (-1) and
> >+  // for every moz_place that has an invalid frecency (<0) and
> nit: < 0

yes

> > nsNavHistory::RemovePagesInternal(const nsCString& aPlaceIdsQueryString)
> ..
> >+  nsresult rv;
> nit: define nsresult rv with the ExecuteSimpleSQL below
> ..
> >+  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> >+      "UPDATE moz_places SET frecency = -MAX(visit_count, 1) WHERE id IN(") +
> >+        aPlaceIdsQueryString +
> >+        NS_LITERAL_CSTRING(")"));
> nit: Could put the ending ")" on the same line as aPlacesIdsQueryString

i find more readable having the variable so visible inside the query, we are doing the same in other places

> >         NS_LITERAL_CSTRING(") AND "
> >         "NOT EXISTS (SELECT b.id FROM moz_bookmarks b WHERE b.fk = h.id) AND "
> >         "NOT EXISTS (SELECT a.id FROM moz_annos a WHERE a.place_id = h.id))"));
> So that it doesn't unnecessarily update frecencies for pages that will be
> removed.

yes, good point

> Should we just do frecency = -visit_count - 1? It's an approximation but saves
> on the MAX, but either way should be fine.

i doubt that a max with a fixed value has a bigger cost, and is more precise (-2 should still be more important than -1, even if will not make a big difference)


> > nsresult
> > nsNavHistory::AddPageWithVisit(nsIURI *aURI,
> >                                const nsString &aTitle,
> >-                               PRBool aHidden, PRBool aTyped,
> >+                               PRBool aHidden,
> >+                               PRBool aTyped,
> >                                PRInt32 aVisitCount,
> >                                PRInt32 aLastVisitTransition,
> >                                PRTime aLastVisitDate)
> So what should we do with the aVisitCount when importing history? Just trust it
> and removing all history visits might not drop visit_count to 0? Similarly,
> what should be done with RemoveDuplicateURIs which combine visit counts..

when importing history we are creating visits, that will increase the visit_count by itself.

RemoveDuplicateURIs has been used in the past to move on to an updated schema, will never be used again since we have a unique index on url.

> >+++ toolkit/components/places/src/nsNavHistoryAutoComplete.cpp	5 Mar 2008 15:58:41 -0000
> >- * But places with frecency (-1) are included, as that means that these items
> >+ * But places with frecency <0 are included, as that means that these items
> nit: < 0

y

> >+  nsresult rv;
> nit: combine

y

> >+  rv = connection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> >+    "UPDATE moz_places SET frecency = -visit_count WHERE id IN("
> >+      "SELECT h.id FROM moz_places h WHERE "
> >+      "EXISTS (SELECT id FROM moz_bookmarks WHERE fk = h.id) OR "
> >+      "EXISTS (SELECT id FROM moz_annos WHERE place_id = h.id AND expiration = ") +
> >+      nsPrintfCString("%d", nsIAnnotationService::EXPIRE_NEVER) +
> >+      NS_LITERAL_CSTRING(")"));
> >+  if (NS_FAILED(rv))
> >+    NS_WARNING("failed to recent frecency");
> frecency = -visit_count - 1 ?

using MAX (as before)

> 
> >+++ toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles.js	5 Mar 2008 15:58:44 -0000
> ..
> >-  histsvc.setPageDetails(uri1, "foo title", 0, false, true);
> >+  histsvc.setPageDetails(uri1, "foo title", false, true);
> >   histsvc.addVisit(uri2, Date.now() * 1000, null, histsvc.TRANSITION_TYPED, false, 0);
> >   histsvc.setPageDetails(uri2, "bar title", 0, false, true);
> How about the second setPageDetails?

fixed
Attachment #307488 - Attachment is obsolete: true
Attachment #307488 - Flags: review?(edilee)
Attachment #307722 - Flags: review?(dietrich)
Comment on attachment 307722 [details] [diff] [review]
v5

>@@ -6116,25 +6166,31 @@ nsNavHistory::CalculateFrecencyInternal(
..
>-        if (NS_SUCCEEDED(rv) && trueVisitCount)
>-          *aFrecency = -1;
>-        else
>-          *aFrecency = 0;
..
>+        PRInt32 visitCount = 0;
..
>+        if (NS_SUCCEEDED(mDBGetIdPageInfo->ExecuteStep(&hasVisits)) && hasVisits) {
>+          rv = mDBGetIdPageInfo->GetInt32(nsNavHistory::kGetInfoIndex_VisitCount,
>+                                          &visitCount);
..
>+        *aFrecency = -visitCount;
Should this be *aFrecency = -(visitCount ? visitCount : 1);


>@@ -552,42 +557,40 @@ nsNavHistoryExpire::EraseVisits(mozIStor
..
>   // Reset the frecencies for the places that don't have any more visits after
>   // we deleted them and make sure they aren't bookmarked either. This means we
>   // keep the old frecencies when possible as an estimate for the new frecency
>   // unless we know it has to be -1.
nit: -1 comment
>-  rv = aConnection->ExecuteSimpleSQL(
>+  // We must do this before deleting visits
>+  nsresult rv = aConnection->ExecuteSimpleSQL(
>     NS_LITERAL_CSTRING(
>       "UPDATE moz_places "
>-      "SET frecency = -1 "
>+      "SET frecency = -visit_count "
>       "WHERE id IN ("
>         "SELECT h.id FROM moz_places h "
>         "LEFT OUTER JOIN moz_historyvisits v ON v.place_id = h.id "
>         "LEFT OUTER JOIN moz_bookmarks b ON b.fk = h.id "
>         "WHERE v.id IS NULL AND b.id IS NULL AND h.id IN (") +
>     placeIds +
>     NS_LITERAL_CSTRING("))"));
>   NS_ENSURE_SUCCESS(rv, rv);
You didn't comment on if that also be -MAX(visit_count, 1)?
(In reply to comment #14)
> Should this be *aFrecency = -(visitCount ? visitCount : 1);
Nevermind. What you had should be fine. If we have visits, it'll be at minimum 1, otherwise 0.
Blocks: 421180
(In reply to comment #14)
> >-  rv = aConnection->ExecuteSimpleSQL(
> >+  // We must do this before deleting visits
> >+  nsresult rv = aConnection->ExecuteSimpleSQL(
> >     NS_LITERAL_CSTRING(
> >       "UPDATE moz_places "
> >-      "SET frecency = -1 "
> >+      "SET frecency = -visit_count "
> >       "WHERE id IN ("
> >         "SELECT h.id FROM moz_places h "
> >         "LEFT OUTER JOIN moz_historyvisits v ON v.place_id = h.id "
> >         "LEFT OUTER JOIN moz_bookmarks b ON b.fk = h.id "
> >         "WHERE v.id IS NULL AND b.id IS NULL AND h.id IN (") +
> >     placeIds +
> >     NS_LITERAL_CSTRING("))"));
> >   NS_ENSURE_SUCCESS(rv, rv);
>
> You didn't comment on if that also be -MAX(visit_count, 1)?

yes, but i also need to change the query, "WHERE v.id IS NULL" does not apply since i've not yet deleted history!!! doh!

will add this fix to dietrich's review comments
Attachment #307722 - Flags: review?(dietrich)
in Bug 419749 we are probably going to remove at all SetPageDetails, so i need to wait for that to unbitrot.
Attached patch unbitrot, fixed comments (obsolete) — — Splinter Review 
i should have addressed all comments so far.

Will most probably bitrot due to Bug 419170, but i prefer landing that before (has also an higher priority)

After fixing this we will add a new trigger to cleanup keywords on bookmark delete (Bug 421180)
Attachment #307722 - Attachment is obsolete: true
Attachment #309438 - Flags: review?(edilee)
Comment on attachment 309438 [details] [diff] [review]
unbitrot, fixed comments

r=Mardak. Hehe, getting rid of setPageDetails cleaned it up a bit. ;)

>+nsNavHistory::CreateTriggers()
..
>+  nsresult rv;
..
>+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
nit: combine nsresult rv = ..

> nsNavHistory::RemovePagesInternal(const nsCString& aPlaceIdsQueryString)
..
>+  nsresult rv;
..
>+  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
nit: combine

>+      "UPDATE moz_places SET frecency = -MAX(visit_count, 1) WHERE id IN(") +
>+        aPlaceIdsQueryString +
>+        NS_LITERAL_CSTRING(") AND ("
>+        "EXISTS (SELECT b.id FROM moz_bookmarks b WHERE b.fk = moz_places.id) "
>+        "OR EXISTS "
>+        "(SELECT a.id FROM moz_annos a WHERE a.place_id = moz_places.id))"));
Can you shift the content inside "AND (" right 2 spaces (exists .. or exists) and break it after the first ).. So something like

"UPDATE .. "
"SET .. "
"WHERE .. IN(" +
  aPlace.. +
  NS_(") AND ("
    "EXISTS .. "
    "OR EXISTS "
      "(SELECT .. )"
  ")"));

> nsNavHistoryExpire::EraseVisits(mozIStorageConnection* aConnection,
..
>+  // Reset the frecencies for the places that won't have any visits after
>+  // we delete them and make sure they aren't bookmarked either. This means we
>   // keep the old frecencies when possible as an estimate for the new frecency
>+  // unless we know it has to be invalidated.
>+  // We must do this before deleting visits
>+  nsresult rv = aConnection->ExecuteSimpleSQL(
>     NS_LITERAL_CSTRING(
>       "UPDATE moz_places "
>+      "SET frecency = -MAX(visit_count, 1) "
>       "WHERE id IN ("
>         "SELECT h.id FROM moz_places h "
>+        "WHERE NOT EXISTS (SELECT b.id FROM moz_bookmarks b WHERE b.fk = h.id) "
>+        "AND NOT EXISTS (SELECT v.id FROM moz_historyvisits v WHERE "
>+          "v.place_id = h.id AND v.id NOT IN (") + deletedVisitIds +
>+          NS_LITERAL_CSTRING(")) AND h.id IN (") + placeIds +
>     NS_LITERAL_CSTRING("))"));
Could you clean that up? Especially putting "h.id IN (placesIds)" on its own line.
UPDATE ..
SET ..
WHERE .. IN (
  SELECT .. FROM ..
  WHERE NOT EXISTS .. AND
    NOT EXISTS .. AND
    h.id IN (placesIds)
)
Attachment #309438 - Flags: review?(edilee) → review+
I'm not sure what style we want for the AND/OR in sql.. but at least for + in the c++, we have it at the end of the previous line.
Blocks: 423391
Blocks: 419170
Whiteboard: [swag: 0.5d] → [waiting for higher priority bug 419170 to unbitrot]
this should be bumped to P2.
Attached patch patch (obsolete) — — Splinter Review 
unbitrot, some cleanup, some new comment, fixed an operator precedence in FixInvalidFrecenciesForExcludedPlaces query (Mardak if this is wrong please annotate here)
Attachment #309438 - Attachment is obsolete: true
Whiteboard: [waiting for higher priority bug 419170 to unbitrot] → [has patch]
Priority: P3 → P2
We want FixInvalidFrecenciesForExcludedPlaces to be 0 for all place: uris, so the original logic was right in its operators but the comment is wrong.
Comment on attachment 310749 [details] [diff] [review]
patch

>+++ toolkit/components/places/src/nsNavHistoryExpire.cpp	20 Mar 2008 13:30:56 -0000
> nsNavHistoryExpire::ClearHistory()
..
>-  // expire visits, then let the paranoid functions do the cleanup for us
>+  // reset frecency for all items that will _not_ be deleted
>+  // Note, we set frecency to -visit_count since we use that value in our
>+  // idle query to figure out which places to recalcuate frecency first.
>+  // We must do this before deleting visits
>   nsresult rv = connection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+    "UPDATE moz_places SET frecency = -MAX(visit_count, 1) "
>+    "WHERE id IN("
>+      "SELECT h.id FROM moz_places h WHERE "
>+        "EXISTS (SELECT id FROM moz_bookmarks WHERE fk = h.id) "
>+        "OR EXISTS "
>+        "(SELECT id FROM moz_annos WHERE place_id = h.id AND expiration = ") +
>+      nsPrintfCString("%d", nsIAnnotationService::EXPIRE_NEVER) +
>+      NS_LITERAL_CSTRING(")"));
How about
UPDATE moz_places
SET frecency = -MAX(visit_count, frecency != 0)

> nsNavHistoryExpire::EraseVisits(mozIStorageConnection* aConnection,
..
>-  // Reset the frecencies for the places that don't have any more visits after
>-  // we deleted them and make sure they aren't bookmarked either. This means we
>+  // Reset the frecencies for the places that won't have any visits after
>+  // we delete them and make sure they aren't bookmarked either. This means we
>   // keep the old frecencies when possible as an estimate for the new frecency
>-  // unless we know it has to be -1.
>-  rv = aConnection->ExecuteSimpleSQL(
>+  // unless we know it has to be invalidated.
>+  // We must do this before deleting visits
>+  nsresult rv = aConnection->ExecuteSimpleSQL(
>     NS_LITERAL_CSTRING(
>       "UPDATE moz_places "
>-      "SET frecency = -1 "
>+      "SET frecency = -MAX(visit_count, 1) "
>       "WHERE id IN ("
>         "SELECT h.id FROM moz_places h "
>-        "LEFT OUTER JOIN moz_historyvisits v ON v.place_id = h.id "
>-        "LEFT OUTER JOIN moz_bookmarks b ON b.fk = h.id "
>-        "WHERE v.id IS NULL AND b.id IS NULL AND h.id IN (") +
>-    placeIds +
>+        "WHERE NOT EXISTS (SELECT b.id FROM moz_bookmarks b WHERE b.fk = h.id) "
>+          "AND NOT EXISTS "
>+            "(SELECT v.id FROM moz_historyvisits v WHERE v.place_id = h.id AND "
>+              "v.id NOT IN (") + deletedVisitIds +
>+              NS_LITERAL_CSTRING(")) AND "
>+              "h.id IN (") + placeIds +
>     NS_LITERAL_CSTRING("))"));
Same here.
Comment on attachment 309438 [details] [diff] [review]
unbitrot, fixed comments

> nsNavHistory::FixInvalidFrecenciesForExcludedPlaces()
> {
>+  // for every moz_place that has an invalid frecency (< 0) and
>   // begins with "place:" or is an unvisited child of a livemark feed, 
>   // set frecency to 0 so that it is excluded from url bar autocomplete.
>   nsCOMPtr<mozIStorageStatement> dbUpdateStatement;
>   nsresult rv = mDBConn->CreateStatement(
>     NS_LITERAL_CSTRING("UPDATE moz_places SET frecency = 0 WHERE id IN ("
>       "SELECT h.id FROM moz_places h JOIN moz_bookmarks b ON h.id = b.fk "
Make this "LEFT OUTER JOIN" so that we reset place: uris that aren't bookmarked.
>-      "WHERE frecency = -1 "
>+      "WHERE frecency < 0 "
>         // place is not a livemark feed item
>         "AND (b.parent IN ("
>           "SELECT annos.item_id FROM moz_anno_attributes attrs "
>           "JOIN moz_items_annos annos ON attrs.id = annos.anno_attribute_id "
>           "WHERE attrs.name = ?1) "
>         // place has no visits (that are not invalid or embedded) 
>-        "AND (SELECT visit_date FROM moz_historyvisits "
>-          "WHERE place_id = h.id AND visit_type NOT IN (0,4) LIMIT 1) is null) "
>+        "AND " SQL_STR_FRAGMENT_MAX_VISIT_DATE ( "h.id ") " is null) "
>       "OR SUBSTR(h.url,0,6) = 'place:')"),
If we fix the "JOIN" -> "LEFT OUTER JOIN" for FixInvalidFrecenciesForExcludedPlaces, we won't need to do the "-MAX(visit_count, 1)" -> "-MAX(visit_count, frecency != 0)" change because all the place: uris will be correctly reset to 0 afterwords.
Attached patch patch (obsolete) — — Splinter Review 
reverted change to query (fixed comment), fixed invalid frecencies
Attachment #310749 - Attachment is obsolete: true
Attachment #310801 - Flags: review?(dietrich)
Comment on attachment 310801 [details] [diff] [review]
patch


>+nsresult
>+nsNavHistory::CreateTriggers()
>+{
>+  // we are creating 2 triggers on moz_historyvisits to maintain
>+  // moz_places.visit_count in sync with moz_historyvisits, for this
>+  // to work we must ensure that all visit_count values are correct
>+  // See bug 416313 for details
>+  nsCOMPtr<mozIStorageStatement> detectVisitCountTrigger;
>+  nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+      "SELECT name FROM sqlite_master WHERE type = 'trigger' AND "
>+      "name = 'moz_historyvisits_afterinsert_v1_trigger'"),
>+    getter_AddRefs(detectVisitCountTrigger));
>+  NS_ENSURE_SUCCESS(rv, rv);

please file a followup for adding triggerExists() to mozStorage

>+
>+  PRBool hasTrigger;
>+  rv = detectVisitCountTrigger->ExecuteStep(&hasTrigger);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = detectVisitCountTrigger->Reset();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!hasTrigger) {
>+    mozStorageTransaction createTriggersTransaction(mDBConn, PR_FALSE);
>+
>+    // do a one-time reset of all the visit_count values
>+    rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+      "UPDATE moz_places SET visit_count = "
>+      "(SELECT count(*) FROM moz_historyvisits "
>+      "WHERE place_id = moz_places.id AND visit_type NOT IN (0,4,7))"));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // moz_historyvisits_afterinsert_v1_trigger
>+    // increment visit_count by 1 for each inserted visit
>+    // exluding invalid, embed, download visits

s/exluding/excluding/

>@@ -988,28 +1064,30 @@ nsNavHistory::InitStatements()
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>       "SELECT id, visit_count, typed, hidden "
>       "FROM moz_places "
>       "WHERE url = ?1"),
>     getter_AddRefs(mDBGetPageVisitStats));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // mDBUpdatePageVisitStats (see InternalAdd)
>+  // we don't need to update visit_count since it's maintained
>+  // in sync by triggers, and we should NEVER touch it

s/should/must/

r=me otherwise. thanks ed for doing first review.
Attachment #310801 - Flags: review?(dietrich) → review+
(In reply to comment #28)
> please file a followup for adding triggerExists() to mozStorage

filled bug 424972
Attached patch patch — — Splinter Review 
fixed review comments
Attachment #310801 - Attachment is obsolete: true
Whiteboard: [has patch] → [has patch][check-in after beta 5 freeze]
Keywords: checkin-needed
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.282; previous revision: 1.281
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.153; previous revision: 1.152
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.53; previous revision: 1.52
done
Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.44; previous revision: 1.43
done
Checking in toolkit/components/places/tests/unit/test_000_frecency.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_000_frecency.js,v  <--  test_000_frecency.js
new revision: 1.8; previous revision: 1.7
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][check-in after beta 5 freeze]
Depends on: 425563
As comment 6 states do we need dev-doc-needed here?

Marko, what's the easiest way to verify this fix? 
(In reply to comment #33)
> As comment 6 states do we need dev-doc-needed here?
> 
> Marko, what's the easiest way to verify this fix? 

humm you should visit sites and take a look at the visit_count column in sqlite manager. should increase after every visit, and should decrease if you delete visits (fox example in the sidebar). should go to 0 when you clear history.
Or you could create a test using maxVisits and minVisits.

PS: visit_count does not count embed visits (images for example) and downloads


you should also verify that on clear history the frecency column is setup to -visit_count before visit_count is reset
Blocks: 487809
Shouldn't we add a test for this at some time?
Flags: in-testsuite?
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: