Closed Bug 447900 Opened 16 years ago Closed 11 years ago

places prefs need appropriate default values

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: dougt, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#725

in FF prefs, we list out all of the places prefs
we don't do this for mobile

in nsNavHistory.cpp, we read in all of the prefs, but do not take into account failures:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#1921


are the prefs required (and if so, why are they in toolkit or something like that), should the code be more protective, and what should mobile?
Summary: Figure out what to do about places prefs → non-firefox apps: Figure out what to do about places prefs
over to dietrich.  fennec will just copy the preferences in firefox.js into our preferences file.  it would be cool for this to not be required.
Assignee: doug.turner → dietrich
OS: Mac OS X → All
Hardware: PC → All
Summary: non-firefox apps: Figure out what to do about places prefs → places prefs need appropriate default values
See bug 424557 comment #29 - it seems to work for me without most of the prefs set there, but what I need is the following set:

--------------
// The special characters below can be typed into the urlbar to either restrict
// the search to visited history, bookmarked, tagged pages; or force a match on
// just the title text or url.
pref("browser.urlbar.restrict.history", "^");
pref("browser.urlbar.restrict.bookmark", "*");
pref("browser.urlbar.restrict.tag", "+");
pref("browser.urlbar.match.title", "#");
pref("browser.urlbar.match.url", "@");
--------------
Product: Firefox → Toolkit
QA Contact: places → places
Version: unspecified → Trunk
I think we actually should rework those prefs and split them up in two prefs for each of those restrictions: one true/false for defaulting to the restriction and one for the character, like we have it now. Merging those two different things into one single pref per item makes it hard to 1) expose one or two of those restrictions in UI (e.g. for SeaMonkey, we'd very much like to have UI for turning title matching on/off) and 2) add additional items in the future (e.g. when downloads are added and therefore a download restriction is added but some apps don't have a default pref for it and this is interpreted as empty)
No longer blocks: 447906
Depends on: 424557
In addition to the prefs mentioned above I found my frecencies were not being calculated correctly and resolved my problem by copying the following prefs:

+// the (maximum) number of the recent visits to sample
+// when calculating frecency
+pref("places.frecency.numVisits", 10);
+
+// Number of records to update frecency for when idle.
+pref("places.frecency.numCalcOnIdle", 50);
+
+// Number of records to update frecency for when migrating from
+// a pre-frecency build.
+pref("places.frecency.numCalcOnMigrate", 50);
+
+// Perform frecency recalculation after this amount of idle, repeating.
+// A value of zero disables updating of frecency on idle.
+// Default is 1 minute (60000ms).
+pref("places.frecency.updateIdleTime", 60000);
+
+// buckets (in days) for frecency calculation
+pref("places.frecency.firstBucketCutoff", 4);
+pref("places.frecency.secondBucketCutoff", 14);
+pref("places.frecency.thirdBucketCutoff", 31);
+pref("places.frecency.fourthBucketCutoff", 90);
+
+// weights for buckets for frecency calculations
+pref("places.frecency.firstBucketWeight", 100);
+pref("places.frecency.secondBucketWeight", 70);
+pref("places.frecency.thirdBucketWeight", 50);
+pref("places.frecency.fourthBucketWeight", 30);
+pref("places.frecency.defaultBucketWeight", 10);
+
+// bonus (in percent) for visit transition types for frecency calculations
+pref("places.frecency.embedVisitBonus", 0);
+pref("places.frecency.linkVisitBonus", 100);
+pref("places.frecency.typedVisitBonus", 2000);
+pref("places.frecency.bookmarkVisitBonus", 150);
+pref("places.frecency.downloadVisitBonus", 0);
+pref("places.frecency.permRedirectVisitBonus", 0);
+pref("places.frecency.tempRedirectVisitBonus", 0);
+pref("places.frecency.defaultVisitBonus", 0);
+
+// bonus (in percent) for place types for frecency calculations
+pref("places.frecency.unvisitedBookmarkBonus", 140);
+pref("places.frecency.unvisitedTypedBonus", 200);
(In reply to comment #4)
> In addition to the prefs mentioned above I found my frecencies were not being
> calculated correctly and resolved my problem by copying the following prefs:

true, without defining prefs frecency does not work and locationbar does not report any visited website
bug 463459 should have eased that a bit, I hope we at least diaply websites again when no prefs are defined. frecencies are a different topic though.
Attached patch wip (obsolete) — Splinter Review
put pref defaults in a single location. access them via helpers that allow passing default values.

one example call included. asking for preliminary review of the approach.
Attachment #381361 - Flags: review?
Attachment #381361 - Flags: review? → review?(sdwilsh)
Comment on attachment 381361 [details] [diff] [review]
wip

>+// preference defaults
can we pull all of these out into some kind of #include?

>+// static
>+nsCAutoString 
>+nsNavHistory::GetCharPref(const char *aPref, const nsCString aDefault)
these can't be static if we are accessing mPrefBranch ;)

>+{
>+  nsCAutoString result;
>+  if (mPrefBranch &&
>+    NS_FAILED(mPrefBranch->GetCharPref(aPref, getter_Copies(result)))) {
>+    result = aDefault;
>+  }
it'd be simpler to set result to aDefault initially, and then return if we have success.  Also, no braces for one line if's please

>+PRPackedBool
>+nsNavHistory::GetBoolPref(const char *aPref, PRBool aDefault)
>+{
>+  PRBool result;
>+  if (!mPrefBranch ||
>+      NS_FAILED(mPrefBranch->GetBoolPref(aPref, &result))) {
>+    result = aDefault;
>+  }
>+  return result;
I'm not sure it's safe to use PRPackedBool here (nor does it win anything for us being on the stack)

>+  // Pref utils
>+  nsCAutoString GetCharPref(const char *aPref,
>+                            nsCString aDefault = EmptyCString());
>+  PRPackedBool GetBoolPref(const char *aPref,
>+                           PRBool aDefault = PR_FALSE);
>+  PRInt32 GetIntPref(const char *aPref, PRInt32 aDefault = 0);
Real javadoc comments too please (even if it seems obvious)
Attachment #381361 - Flags: review?(sdwilsh) → review-
otherwise, approach is fine by me
Just as a personal preference, i would prefer if those helpers would be called differently from original pref service method names, to avoid confusing approaching devs.
So something like GetPlacesBoolPref.
I don't have a preference on that, so do whatever.
Attached patch wipSplinter Review
fixed sdwilsh's comments. marco, if this looks ok to you, i'll keep going and replace instances of pref calls w/ the new calls.
Attachment #381361 - Attachment is obsolete: true
Attachment #381578 - Flags: review?(mak77)
Attachment #381578 - Flags: review?(mak77)
Comment on attachment 381578 [details] [diff] [review]
wip

i think the approach is fine for me, some comment:

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>+                               mAutoCompleteDefaultBehavior(PREF_DEFAULT_AUTOCOMPLETE_DEFAULT_BEHAVIOR),

aren't there too many DEFAULT in here? PREF_DEFAULT_AUTOCOMPLETE_BEHAVIOR should be enough


>+/**
>+ * PlacesGetCharPref

those are still somehow getters, so i would still prefer GetPlacesCharPref (And so on) this will underline it is a getter helper for a places char pref. Sounds more correct to me, but is clearly a personal opinion.

>+ *
>+ * @param aPref
>+ *        preference name
>+ * @param aDefault

aDefaultValue

>diff --git a/toolkit/components/places/src/nsPlacesPreferences.h b/toolkit/components/places/src/nsPlacesPreferences.h
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2008

2009
Assignee: dietrich → nobody
we are now using Preferences helpers who allow to provide default values, so this is no more needed.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: