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

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Ed Lee :Mardak]

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.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

I haven't touched the tests yet, but a quick test run seems to work.

Comment 2

[Ed Lee :Mardak]

Attachment: v1.1

Updated tests.

Comment 3

[Dietrich Ayala (:dietrich)]

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!

Comment 4

[Ed Lee :Mardak]

Attachment: v1.2

Updated comments. Is this supposed to go in for b2? Is the privacy stuff supposed to be in for b2?

Comment 5

[Ed Lee :Mardak]

Attachment: v2

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.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 346796 [details] [diff] [review]
v2

r=me, thanks!

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 346796 [details] [diff] [review]
v2

required for the location bar suggestion changes to the privacy preferences pane.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 346796 [details] [diff] [review]
v2

gleefully approved.

Comment 9

[Dietrich Ayala (:dietrich)]

per irc, not making b2, needs more discussion.

Comment 10

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

Comment on attachment 346796 [details] [diff] [review]
v2

a191=beltzner

Comment 11

[Ed Lee :Mardak]

Attachment: v2.1

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.

Comment 12

[Ed Lee :Mardak]

Dietrich, are we taking this to address bug 447900 comment 3?

Comment 13

[Ed Lee :Mardak]

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?

Comment 14

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/5ce74cb333cc

Comment 15

[Marco Bonardo [:mak]]

(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

Comment 16

[Robert Kaiser]

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). :)

Comment 17

[Ed Lee :Mardak]

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.)

Comment 18

[Robert Kaiser]

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 ;-)

Comment 19

[Ed Lee :Mardak]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/74f40ddd9a9d

Comment 20

[Ed Lee :Mardak]

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.

Comment 23

[Eric Shepherd [:sheppy]]

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?

Comment 24

[Ed Lee :Mardak]

I suppose not if we didn't have anything saying to use an empty preference to trigger the filtering.