402880 - figure out pref UI for history expiration / limiting visits

Status: VERIFIED FIXED

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

Description

[(not reading, please use [e-mail] instead)]

figure out pref UI for history expiration / limiting visits

for bug #332748, a new pref was added: browser.history_expire_visits


for b2, should we fix the pref ui (under Tools | Privacy | History) to allow the user to specify visits (like opera does?)

or should we change the text to be "remember visited pages for the last [x] days" to be something like "remember visited pages for at most [x] days"

Comment 1

[Tony Mechelynck [:tonymec]]

(In reply to comment #0)
> figure out pref UI for history expiration / limiting visits
> 
> for bug #332748, a new pref was added: browser.history_expire_visits
> 
> 
> for b2, should we fix the pref ui (under Tools | Privacy | History) to allow
> the user to specify visits (like opera does?)
> 
> or should we change the text to be "remember visited pages for the last [x]
> days" to be something like "remember visited pages for at most [x] days"
> 
I don't use Opera.

Maybe we could use something like:
    Remember the latest [___] visited pages in the last [___] days
?

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: screen shot of opera prefs

Comment 3

[(not reading, please use [e-mail] instead)]

Tony, thanks for suggesting a wording.  

I'll leave it to the ui/ux/wording gurus to decide what we should do here.

Comment 4

[(not reading, please use [e-mail] instead)]

How about:

Remember the last [x] visits from the past [y] days.

We should probably get this new string in ASAP, before the l10n freeze.

Comment 5

[(not reading, please use [e-mail] instead)]

when we fix this, it might be a good time to fix polish bug #403954, too.

Comment 6

[(not reading, please use [e-mail] instead)]

over email, beltzner writes:

I worry a little about the added complication here. Users must now think about the total number of visits in combination with the day limit. 

I think my more fundamental worry is that users don't. have a good sense of how many pages they visit, so I'm not sure they could really answer that question.

Could we instead do something like:

Remember my history for at least [n] days

And ensure that we don't expire visits within that timeframe - perhaps bad for users who set it high and then visit tons of sites, but shouldn't affect too many people if we get the value of "n" right.  

Comment 7

[(not reading, please use [e-mail] instead)]

> Remember my history for at least [n] days
> And ensure that we don't expire visits within that timeframe

Actually, I think what we're really doing under the hood is:

"Remember my history for at most [n] days"

If you have too many visits in that time, we'll cull them.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

    I morphed bug 403572 a little bit, and now it basically means "make sure we're
    not removing history entries when the user doesn't expect us to"

    Connor and I just brainstormed two potential UIs:

    1. The one I proposed:

    Remember my history for at least [N] days

     - we will use browser.history_expired_visits to keep the list down for perf,
     - BUT we will only remove history entries when they are older than N days

    2. A slider which ranges from:

       About 20 days                           About 180 days
         |---------------------------------------------|
       (10,000 entries)                        (180,000 entries)

    In either case, we should consider giving the user proper warning when they
    specify a value that could adversely affect performance.

    Our default should be a good mix of averages.

    (retaining uiwanted for now, upping to P1)

Comment 9

[(not reading, please use [e-mail] instead)]

mike, thanks for explaining.  at least now I get your proposal!

some problems:

1) "Remember my history for at least [N] days"

I choose zero, because I want to disable history.  but "Remember my history for a least 0 days" seems to indicate that we might still remember

2)  "A slider which ranges from x to y"

We should either have "0" as the starting point, right?

Comment 10

[Dietrich Ayala (:dietrich)]

per https://bugzilla.mozilla.org/show_bug.cgi?id=403572#c5, moving implementation discussion over here.

Regarding https://bugzilla.mozilla.org/show_bug.cgi?id=403572#c1:

> 1) raise the limit to more like 40k visits (200+ real vists per day for 180
> months)
> 2) lower the expiry time for embedded link visits to one day
> 3) when we get to within 500 visits of the cap, make sure we aggressively clear
> the embedded visits before any "real" visits get lost.

I think your #2 obviates the need to do this. A user would need 39,500 visits in less than 24 hours for this to have any effect.

Reed running mochitest repeatedly in a background window would trigger this. However, in his case we'd blow past the cap pretty quickly so it wouldn't really make a difference.

Also, I've implemented "don't add new visits for refreshes", which would should mitigate the "scripts gone wild" and "aggressive ad refreshing" scenarios.

A scenario still not covered is if the gone-wild script is a spidering script, causing a high volume of visits to unique URLs. In this case, we could implement a time-based volume restriction to cover this, so that if say N unique visits are added in short-period X, and the oldest visit in the users history is over Y days old (ie: if their oldest visit is relatively recent, they must just be an unbelievably high-volume browser, so this is normal behavior), we call bullshit on the new visits and throw them away.

> 4) continue to purge moz_places records with zero visits.
> 5) figure out how to present these caps sanely for users in the UI...
> 

Comment 11

[Dietrich Ayala (:dietrich)]

(In reply to comment #9)
> 
> 1) "Remember my history for at least [N] days"
> 
> I choose zero, because I want to disable history.  but "Remember my history for
> a least 0 days" seems to indicate that we might still remember

We do have the checkbox to disable history. However, if you set the value to 0, the checkbox is not immediately un-checked (it is unchecked if you close and then re-open the prefs UI). If we immediately un-checked it, it'd be a better visual cue that 0 days == disabling history entirely.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: wip

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

Summary of the changes in this patch:
- Change the default date limit to 90 days
- Change the visit cap to a site cap
- Set the default site cap to 40k
- Expire embedded visits after a day
- UI: Implement idea #1 in comment #8
- UI: Implement the improvement in comment #11

Summary of the approach in this patch:

"Remember my history for at least {browser.history_expire_days} days. For history over {browser.history_expire_days} days old, remember up to {browser.history_expire_sites} sites."

This allows for 3 possible states of history, listed below (and the resulting action taken in case):

0 records over 90 days old: do nothing
>0 records over 90 days old, but <40k total records: do nothing
>0 records over 90 days old and >40k total records: expire the oldest records over 40k

I've lowered the date range, since we're not enforcing the deterministic limit until we've blown past the date limit (bug 403572). I've increased the deterministic cap, as it's clearly too low for power users. I've changed the definition of the cap to be "sites" instead of visits, meaning it'll effectively cap moz_places (history records only, no cap on records due to bookmarks, etc). How does this sound to everyone?

I'll ask for review once we settle on the approach and numbers.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 289375 [details] [diff] [review]
fix v1

Marco, can you please review the changes in FindVisits? I've modified the LIMIT/OFFSET to apply to moz_places instead of moz_historyvisits, in effect capping moz_places.

Still to-do is to rename the visits pref to reflect that it applies to sites, not visits.

Comment 15

[Dietrich Ayala (:dietrich)]

Sorry, another change I made in this patch is that refreshes are no longer counted as new visits.

If a new visit occurs, and the referrer URI is the same as the new URI, the visit is not added to the database.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 289375 [details] [diff] [review]
fix v1

i'm a little confused on this, since i can't clearly see the target...

if aNumToExpire == 0 then FindVisits will select ALL visits (clear history), that will be slow, will this cause calling a delete on every visit? i can't remember atm if there is a different approach on clear history with a single delete query.

the following case is FALSE:
"0 records over 90 days old and >40k total records: expire the oldest records over 40k"
that is because WHERE visit_date < 90_days_before_today is imperative if aNumToExpire !=0, so if there are no items < 90_days_before, it will not find anything even if we are over the 40000 visit cap. 
what i see here is:
"0 records over 90 days old and >40k total records: will expire nothing"
"some record over 90 days old and >40k total records: will expire some record"

i don't see the change in selecting places instead of visits, since changing the order of the joins will not change the resultset, so you will end up with the same visit results, reordering the joins is matter of the sqlite optimizer...

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

please ignore my last point (my mind was elsewhere), however, what's the advantage in finding places instead of visits, the historyvisits table should be the most populated

Comment 18

[Dietrich Ayala (:dietrich)]

(In reply to comment #16)
> (From update of attachment 289375 [details] [diff] [review])
> i'm a little confused on this, since i can't clearly see the target...
> 
> if aNumToExpire == 0 then FindVisits will select ALL visits (clear history),
> that will be slow, will this cause calling a delete on every visit? i can't
> remember atm if there is a different approach on clear history with a single
> delete query.

Yes, this really should be optimized to use a single query.

> 
> the following case is FALSE:
> "0 records over 90 days old and >40k total records: expire the oldest records
> over 40k"
> that is because WHERE visit_date < 90_days_before_today is imperative if
> aNumToExpire !=0, so if there are no items < 90_days_before, it will not find
> anything even if we are over the 40000 visit cap. 
> what i see here is:
> "0 records over 90 days old and >40k total records: will expire nothing"
> "some record over 90 days old and >40k total records: will expire some record"
> 

Sorry, the second and third lines had the ">0", which when posted here looked like a response to a previous comment, but were meant as "greater than zero".

(In reply to comment #17)
> please ignore my last point (my mind was elsewhere), however, what's the
> advantage in finding places instead of visits, the historyvisits table should
> be the most populated
> 

1. Capping the number of "sites" vs "visits" is easier for users to understand. 

2. Because of revisitation, the visits table will always be larger than the moz_places table, so capping on it will have a magnified effect on moz_places.

3. In most of the big places.sqlite files I've tested, moz_historyvisits+indexes comprises a very small amount of space, while moz_places+indexes takes 75% or more. Capping moz_places will have a bigger effect on db size and performance.

Comment 19

[bomfog]

(In reply to comment #11)
> (In reply to comment #9)
> > 
> > 1) "Remember my history for at least [N] days"
> > 
> > I choose zero, because I want to disable history.  but "Remember my history for
> > a least 0 days" seems to indicate that we might still remember
> 
> We do have the checkbox to disable history. However, if you set the value to 0,
> the checkbox is not immediately un-checked (it is unchecked if you close and
> then re-open the prefs UI). If we immediately un-checked it, it'd be a better
> visual cue that 0 days == disabling history entirely.
> 

If it's not too late, and this is the right bug, I'd like to put in a plea for a way for the user to set a non-zero hard cap by date. I'd like my link coloring to reset after x number of days. As it is now, unless I missed a change, the behavior, for my purposes, of an x day setting seems pretty undefined, to the user ignorant of visit caps and such.

Stop me, before I comma again....

Comment 20

[Dietrich Ayala (:dietrich)]

Attachment: fix v2 (wip)

- renames the visits pref to history_expire_sites, since it's a site limit now
- adds browser.history_expire_days_min, and exposes that in the UI instead of expire_days (the max-age cap)
- changes ClearHistory to use a single delete query (for perf and code simplification)
- fixes ExpireAnnotationsParanoid to remove annotations on URIs that have zero visits (and are not EXPIRE_NEVER)
- updates tests to account for max-age as well

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Removing ui-wanted as the design seems pretty set.

>+<!ENTITY  rememberDaysBefore.label      "Remember my history for at least">
>+<!ENTITY  rememberDaysBefore.accesskey  "v">
>+<!ENTITY  rememberDaysAfter.label       "days.">

The wording here might be tricky. The above seems OK for now, and I'd rather see it get in sooner than later, but potential alternatives are (in order of my preference):

  * Keep my history for at least ___ days
  * Remember the pages I've been to for at least ___ days
  * Remember my browsing history for at least ___ days
  * Keep track of at least the last ___ days of browsing history
  * Remember at least ___ days of browsing history
  * Remember the pages I've been to for at least the past ___ days

Comment 22

[Ian Pottinger]

Remember all the pages I've visited within the last ___ days

Comment 23

[Dietrich Ayala (:dietrich)]

(In reply to comment #22)
> Remember all the pages I've visited within the last ___ days
> 

I like Beltzner's suggestion, due to brevity:

Keep my history for at least ___ days
Remember all the pages I've visited within the last ___ days


And while "history" is somewhat ambiguous, it is familiar in this context.

I don't like "all" because it implies that it's possible to only remember specific subsets of history, which is not the case.

Comment 24

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

- makes beltzner's text change
- removes the change to clear history in one query, until we figure out the notification issues. will file a new bug for optimizating that.

Comment 25

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

removed sessionstore changes from the patch

Comment 26

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

removed extraneous profiling line.

Comment 27

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 290571 [details] [diff] [review]
fix v3

 // nsNavHistoryExpire::FindVisits
...
+//    * With a visit date greater than the minimum, and less than the maximum,
+//      and over the visit cap of browser.history_expire_visits. 

should be browser.history_expire_sites


+  // Select moz_places records that have a visit
   nsCAutoString sqlBase;
   sqlBase.AssignLiteral(
     "SELECT v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, b.fk "
-    "FROM moz_historyvisits v LEFT JOIN moz_places h ON v.place_id = h.id "
-    "LEFT OUTER JOIN moz_bookmarks b on v.place_id = b.fk ");
+    "FROM moz_places h LEFT JOIN moz_historyvisits v ON h.id = v.place_id "
+    "LEFT OUTER JOIN moz_bookmarks b on h.id = b.fk ");

since this is a left join on moz_historyvisits, will find also sites that does not have a visit, so i don't understand the comment... if a site has no visit visit_date will be NULL, NULL < max_age will be NULL, this should not be a problem, but it's not FALSE

also in sqlite LEFT JOIN and LEFT OUTER JOIN are the same thing, don't need to mix them up, LEFT JOIN is simply a short form of LEFT OUTER JOIN



in expireAnnotationsParanoid

+      "WHERE p.id IS NULL "
+      "OR (v.id IS NULL AND a.expiration != ") +
+      nsPrintfCString("%d", nsIAnnotationService::EXPIRE_NEVER) +

this will expire annotation also for unvisited bookmarks (if they have not an expire_never annotation), is this correct?

Comment 28

[(not reading, please use [e-mail] instead)]

Comment on attachment 290571 [details] [diff] [review]
fix v3

clearing request, I believe dietrich is working on a "v4"

Comment 29

[Dietrich Ayala (:dietrich)]

Attachment: fix v4

Comment 30

[Dietrich Ayala (:dietrich)]

(In reply to comment #27)
> 
> should be browser.history_expire_sites

fixed
> 
> 
> +  // Select moz_places records that have a visit
>    nsCAutoString sqlBase;
>    sqlBase.AssignLiteral(
>      "SELECT v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden,
> b.fk "
> -    "FROM moz_historyvisits v LEFT JOIN moz_places h ON v.place_id = h.id "
> -    "LEFT OUTER JOIN moz_bookmarks b on v.place_id = b.fk ");
> +    "FROM moz_places h LEFT JOIN moz_historyvisits v ON h.id = v.place_id "
> +    "LEFT OUTER JOIN moz_bookmarks b on h.id = b.fk ");
> 
> since this is a left join on moz_historyvisits, will find also sites that does
> not have a visit, so i don't understand the comment... if a site has no visit
> visit_date will be NULL, NULL < max_age will be NULL, this should not be a
> problem, but it's not FALSE
> 
> also in sqlite LEFT JOIN and LEFT OUTER JOIN are the same thing, don't need to
> mix them up, LEFT JOIN is simply a short form of LEFT OUTER JOIN
> 
> 

fixed, fixed

> 
> in expireAnnotationsParanoid
> 
> +      "WHERE p.id IS NULL "
> +      "OR (v.id IS NULL AND a.expiration != ") +
> +      nsPrintfCString("%d", nsIAnnotationService::EXPIRE_NEVER) +
> 
> this will expire annotation also for unvisited bookmarks (if they have not an
> expire_never annotation), is this correct?
> 

yes, this is in-line w/ our expiration of date-dependent annotations in EraseAnnotations.

Comment 31

[(not reading, please use [e-mail] instead)]

review comments:

1) in comment #19, Michael Warner wrote:

If it's not too late, and this is the right bug, I'd like to put in a plea for
a way for the user to set a non-zero hard cap by date. I'd like my link
coloring to reset after x number of days. As it is now, unless I missed a
change, the behavior, for my purposes, of an x day setting seems pretty
undefined, to the user ignorant of visit caps and such.

over irc, dietrich says "that's implemented in the patch"

but here's my question:  this is controlled by the browser.history_expire_day pref, which used to be visible in the ui, but is not longer visible.  

if they never changed it in fx 2, it would have been migrated from 9 days to 180, no problem, as 9 was the default.

but if they did change it (to say 0), they would need to use about:config or manually edit prefs re-enable history.

Does this concern make sense?

2)

On a related note, how does a user disable history completely from the UI now?

3)

+   * browser.history_expire_days_min.mirror
    * - a preference whose value mirrors that of browser.history_expire_days, to
    *   make the "days of history" checkbox easier to code

should be:

    * - a preference whose value mirrors that of browser.history_expire_days_min, to

4)

-<!ENTITY  rememberBefore.label          "Remember visited pages for the last">
+<!ENTITY  rememberDaysBefore.label      "Keep my history for at least">

<!ENTITY  rememberBefore.accesskey      "v">
<!ENTITY  rememberDaysBefore.accesskey  "v">

we need a different accesskey value, as there is no "v" in "Keep my history for at least".

5)

nsNavHistoryExpire::FindVisits(PRTime aExpireThreshold, PRUint32 aNumToExpire,
...
rv = selectStatement->BindInt64Parameter(1, aNumToExpire);

shouldn't that be BindInt32Parameter()?

(previously, we had rv = visitsStatement->BindInt32Parameter(1, mHistory->mExpireVisits);

we do this in a couple spots.

6)

can you take a screen shot of the pref UI?

Comment 32

[Dietrich Ayala (:dietrich)]

Attachment: fix v5

address sspitzer review comments

Comment 33

[Dietrich Ayala (:dietrich)]

Attachment: screenshot

Comment 34

[(not reading, please use [e-mail] instead)]

Comment on attachment 291619 [details] [diff] [review]
fix v5

r=sspitzer

about Date.now() * 1000, we must be making this mistake in other tests, right?  (hopefully not in our code as well)

PR_Now() and Date.now() use different units, right?

can you log a bug about fixing the other tests?

Comment 35

[Dietrich Ayala (:dietrich)]

filed bug 406937 for the Date.now()/PR_Now() mismatch.

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.237; previous revision: 1.236
done
Checking in browser/components/preferences/privacy.js;
/cvsroot/mozilla/browser/components/preferences/privacy.js,v  <--  privacy.js
new revision: 1.21; previous revision: 1.20
done
Checking in browser/components/preferences/privacy.xul;
/cvsroot/mozilla/browser/components/preferences/privacy.xul,v  <--  privacy.xul
new revision: 1.22; previous revision: 1.21
done
Checking in browser/locales/en-US/chrome/browser/preferences/privacy.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/privacy.dtd,v  <--  privacy.dtd
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.207; previous revision: 1.206
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.115; previous revision: 1.114
done
Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/components/places/src/nsNavHistoryExpire.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.h,v  <--  nsNavHistoryExpire.h
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/components/places/tests/unit/test_expiration.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_expiration.js,v  <--  test_expiration.js
new revision: 1.9; previous revision: 1.8
done

Comment 36

[(not reading, please use [e-mail] instead)]

dietrich, can you attach v6 (what you checked in?)

Comment 37

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007120504 Minefield/3.0b2pre

Comment 38

[Dietrich Ayala (:dietrich)]

Attachment: v6 - as checked in

Comment 39

[Gervase Markham [:gerv]]

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