Closed Bug 463459 Opened 11 years ago Closed 11 years ago

Use a separate pref instead of empty restrict/match values to specify defaults

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 4 obsolete files)

Missing/empty pref on restrict/match behavior will make that filter the default behavior. We can separate the multiple use of the pref into a default.behavior pref that lets you toggle which things to use by default.
Attached patch v1 (obsolete) — Splinter Review
I haven't touched the tests yet, but a quick test run seems to work.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #346699 - Flags: review?(dietrich)
Component: Location Bar and Autocomplete → Places
Product: Firefox → Toolkit
QA Contact: location.bar → places
Attached patch v1.1 (obsolete) — Splinter Review
Updated tests.
Attachment #346699 - Attachment is obsolete: true
Attachment #346704 - Flags: review?(dietrich)
Attachment #346699 - Flags: review?(dietrich)
Attachment #346704 - Flags: review?(dietrich) → review+
Comment on attachment 346704 [details] [diff] [review]
v1.1

>diff --git a/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp b/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp
>--- a/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp
>+++ b/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp
>@@ -789,11 +789,11 @@
> nsNavHistory::ProcessTokensForSpecialSearch()
> {
>   // If any of the special searches are empty, automatically use it
>-  mRestrictHistory = mAutoCompleteRestrictHistory.IsEmpty();
>-  mRestrictBookmark = mAutoCompleteRestrictBookmark.IsEmpty();
>-  mRestrictTag = mAutoCompleteRestrictTag.IsEmpty();
>-  mMatchTitle = mAutoCompleteMatchTitle.IsEmpty();
>-  mMatchUrl = mAutoCompleteMatchUrl.IsEmpty();
>+  mRestrictHistory = mAutoCompleteDefaultBehavior & kAutoCompleteDefaultHistory;
>+  mRestrictBookmark = mAutoCompleteDefaultBehavior & kAutoCompleteDefaultBookmark;
>+  mRestrictTag = mAutoCompleteDefaultBehavior & kAutoCompleteDefaultTag;
>+  mMatchTitle = mAutoCompleteDefaultBehavior & kAutoCompleteDefaultTitle;
>+  mMatchUrl = mAutoCompleteDefaultBehavior & kAutoCompleteDefaultUrl;

update the comment

r=me, thanks!
Attached patch v1.2 (obsolete) — Splinter Review
Updated comments. Is this supposed to go in for b2? Is the privacy stuff supposed to be in for b2?
Attachment #346704 - Attachment is obsolete: true
Blocks: 463535
Attached patch v2 (obsolete) — Splinter Review
Fix bug 463535 about clearing the cache on pref change so that the testcase can be made even better.

Also switch to bit getters/setters instead of 5 separate bool variables.
Attachment #346714 - Attachment is obsolete: true
Attachment #346796 - Flags: review?(dietrich)
Comment on attachment 346796 [details] [diff] [review]
v2

r=me, thanks!
Attachment #346796 - Flags: review?(dietrich) → review+
Blocks: 463558
Blocks: 463483
Attachment #346796 - Flags: approval1.9.1b2?
Comment on attachment 346796 [details] [diff] [review]
v2

required for the location bar suggestion changes to the privacy preferences pane.
Target Milestone: --- → mozilla1.9.1b2
Attachment #346796 - Flags: approval1.9.1b2? → approval1.9.1b2+
Comment on attachment 346796 [details] [diff] [review]
v2

gleefully approved.
Blocks: 463661
No longer blocks: 463661
per irc, not making b2, needs more discussion.
Blocks: 463661
Target Milestone: mozilla1.9.1b2 → mozilla1.9.1
No longer blocks: 463661
No longer blocks: 463535
Attachment #346796 - Flags: approval1.9.1b2-
Attachment #346796 - Flags: approval1.9.1b2+
Attachment #346796 - Flags: approval1.9.1?
Attachment #346796 - Flags: approval1.9.1? → approval1.9.1+
Attached patch v2.1Splinter Review
Updated patch from changes from bug 463661, but dietrich had concerns about if this should make it for 3.1 now that bug 463661 provides a search.sources pref.

This patch would get rid of the "apply if pref is empty" behavior which would fix problems from other apps using places and don't have defaults set.
Attachment #346796 - Attachment is obsolete: true
Dietrich, are we taking this to address bug 447900 comment 3?
Pushed to mozilla-central.. should I leave the bug open for pushing to 1.9.1 branch? Do we want it on 1.9.1 branch right away or until blocked bugs are ready?
(In reply to comment #13)
> Pushed to mozilla-central.. should I leave the bug open for pushing to 1.9.1
> branch? Do we want it on 1.9.1 branch right away or until blocked bugs are
> ready?

after pushing to central bugs should be closed, there is a query in https://wiki.mozilla.org/Places:Plan that shows bugs that still need to make 1.9.1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
How does this new pref relate to browser.urlbar.search.sources introduced by bug 463661?
Still, thanks for doing that as bug 468326 will now become easier to solve (SeaMonkey probably wants a UI pref for a t least url/url+title matching). :)
search.sources values of 1-3 are aliases of default.behavior other than the "no search" ability.

search sources value :: equivalent default behavior value:
0 (search none) :: special to search.sources
1 (history only) :: 1 (restrict to history)
2 (bookmark only) :: 2 (restrict to bookmark)
3 (search hist&book) :: 0 (restrict nothing)

These combine to be more restrictive, so if the user somehow ends up with search.sources=2 (bookmarks) and default.behavior=8 (restrict title) and searches for "^ foo" (dynamic restrict to history), it will match "history bookmarks with foo in the title" or alternatively.. bookmarks that you've visited with foo in the title.

In regards to seamonkey, I would suggest just using default.behavior and only search.sources if you want to turn off searching completely. (And now that empty "special searches" doesn't mean "default use it", those prefs can be set to empty to turn them off.)
OK, maybe we should set both of those prefs in the unit tests I changed in bug 468341 then.

We're currently setting search.sources=1 in SeaMonkey default prefs, should we use default.behavior=1 instead there?

In the UI prefs, I think SeaMonkey will mostly want switching default.behavior between 1 and 17 for now, will be interesting to figure out a UI variant that doesn't break when someone sets other values manually ;-)
No longer blocks: 460343
Flags: in-testsuite+
Keywords: fixed1.9.1
Yeah, default.behavior=1 would be good as you'll probably have logic to set/clear bits for restricting url. Or if you only want to support certain values and wipe out any user set values, just set to 1 or 17 (but you'll still want to use default.behavior).

An alternative approach is to only use default.behavior to toggle "restrict url", so set search.sources=1 and default.behavior=16, but that requires twiddling multiple prefs.
Blocks: 470031
Blocks: 471886
Keywords: dev-doc-needed
Duplicate of this bug: 450844
Duplicate of this bug: 474078
I'm not convinced this is really a developer documentation issue, as it's more of a preference guide issue.  Is there a specific need to have this documented aside from the fact that in general we need docs for all prefs?
I suppose not if we didn't have anything saying to use an empty preference to trigger the filtering.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.