figure out pref UI for history expiration / limiting visits

VERIFIED FIXED in Firefox 3 beta2

Status

()

P1
normal
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: moco, Assigned: dietrich)

Tracking

Trunk
Firefox 3 beta2
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

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"
Flags: blocking-firefox3?
(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
?
Tony, thanks for suggesting a wording.  

I'll leave it to the ui/ux/wording gurus to decide what we should do here.
Whiteboard: [need ui/ux/wording help]
(Assignee)

Updated

12 years ago
Keywords: uiwanted
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
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.
when we fix this, it might be a good time to fix polish bug #403954, too.
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.  
> 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.
(Assignee)

Updated

12 years ago
Depends on: 403572
Target Milestone: --- → Firefox 3 M10
Blocks: 403572
No longer depends on: 403572
    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)

Priority: P2 → P1
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?
(Assignee)

Comment 10

12 years ago
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...
> 
(Assignee)

Comment 11

12 years ago
(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.
(Assignee)

Updated

12 years ago
Assignee: nobody → dietrich
(Assignee)

Comment 12

12 years ago
Posted patch wip (obsolete) — Splinter Review
(Assignee)

Comment 13

12 years ago
Posted patch fix v1 (obsolete) — Splinter Review
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.
Attachment #289351 - Attachment is obsolete: true
(Assignee)

Comment 14

12 years ago
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.
Attachment #289375 - Flags: review?(mak77)
(Assignee)

Comment 15

12 years ago
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 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...
Attachment #289375 - Flags: review?(mak77)
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
(Assignee)

Comment 18

12 years ago
(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.

Status: NEW → ASSIGNED

Comment 19

12 years ago
(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....
(Assignee)

Comment 20

12 years ago
Posted patch fix v2 (wip) (obsolete) — Splinter Review
- 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
Attachment #289375 - Attachment is obsolete: true
Attachment #289753 - Attachment is patch: true
Attachment #289753 - Attachment mime type: application/octet-stream → text/plain
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
Keywords: uiwanted
Whiteboard: [need ui/ux/wording help]

Comment 22

12 years ago
Remember all the pages I've visited within the last ___ days
(Assignee)

Comment 23

12 years ago
(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.
(Assignee)

Comment 24

12 years ago
Posted patch fix v3 (obsolete) — Splinter Review
- 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.
Attachment #289753 - Attachment is obsolete: true
Attachment #290568 - Flags: review?(mak77)
(Assignee)

Comment 25

12 years ago
Posted patch fix v3 (obsolete) — Splinter Review
removed sessionstore changes from the patch
Attachment #290568 - Attachment is obsolete: true
Attachment #290569 - Flags: review?(mak77)
Attachment #290568 - Flags: review?(mak77)
(Assignee)

Comment 26

12 years ago
Posted patch fix v3 (obsolete) — Splinter Review
removed extraneous profiling line.
Attachment #290569 - Attachment is obsolete: true
Attachment #290571 - Flags: review?(mak77)
Attachment #290569 - Flags: review?(mak77)
(Assignee)

Updated

12 years ago
Attachment #290571 - Flags: review?(sspitzer)
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?
Attachment #290571 - Flags: review?(mak77) → review+
Comment on attachment 290571 [details] [diff] [review]
fix v3

clearing request, I believe dietrich is working on a "v4"
Attachment #290571 - Flags: review?(sspitzer)
(Assignee)

Comment 29

12 years ago
Posted patch fix v4 (obsolete) — Splinter Review
Attachment #290571 - Attachment is obsolete: true
Attachment #290693 - Flags: review?(sspitzer)
(Assignee)

Updated

12 years ago
Whiteboard: [has patch][needs review sspitzer]
(Assignee)

Comment 30

12 years ago
(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.
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?
(Assignee)

Comment 32

11 years ago
Posted patch fix v5 (obsolete) — Splinter Review
address sspitzer review comments
Attachment #290693 - Attachment is obsolete: true
Attachment #291619 - Flags: review?(sspitzer)
Attachment #290693 - Flags: review?(sspitzer)
(Assignee)

Comment 33

11 years ago
Posted image screenshot
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?
Attachment #291619 - Flags: review?(sspitzer) → review+
(Assignee)

Comment 35

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review sspitzer]
dietrich, can you attach v6 (what you checked in?)
verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007120504 Minefield/3.0b2pre
Status: RESOLVED → VERIFIED
(Assignee)

Comment 38

11 years ago
Attachment #291619 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Depends on: 407018

Updated

11 years ago
Blocks: 421675
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.