Closed Bug 530209 Opened 15 years ago Closed 10 years ago

Prefs UI for what to suggest in location bar isn't friendly to additional items/combination

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.3
Tracking Status
firefox36 --- verified
relnote-firefox --- -

People

(Reporter: Unfocused, Assigned: alexbardas)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 20 obsolete files)

115.22 KB, image/png
Details
32.30 KB, patch
alexbardas
: review+
Details | Diff | Splinter Review
96.98 KB, patch
alexbardas
: review+
Details | Diff | Splinter Review
Currently, the UI for changing what should be suggested in the location bar results is a dropdown menu with the following items: History and Bookmarks, History, Bookmarks, Nothing.

Unfortunately, this makes adding additional items (and therefore combinations of items) very awkward.

For example, bug 480350 adds tab matches to the location bar results. If this were to be added to the current prefs UI, the drop would have to include:

* History and Bookmarks and Tabs
* History and Bookmarks
* Bookmarks and Tabs
* History and Tabs
* History
* Bookmarks
* Tabs
* Nothing

This is not friendly, and is likely to result in someone's head exploding.

Rather than a dropdown menu listing every possible combination, a series of checkboxes could be used - a checkbox for each item, enabling or disabling that type of result.
I don't see how the following combinations:

* History and Bookmarks
* Bookmarks and Tabs
* History and Tabs

are even possible using the preference browser.urlbar.default.behavior.
@Elmar
Currently it's more along the lines of.

  0: History and Bookmarks
130: Bookmarks, include switching to opened bookmarks.
129: History, include switching to opened history items.

I guess this behavior won't be changed with Bug 546253?
> I guess this behavior won't be changed with Bug 546253?

I don't think so. The current pref design is good at restricting matches to one or more categories, but it cannot express "suggest items that are in either bookmarks or history" or "suggest items that are in bookmarks or an open tab".

Initially, I came here to find a way to disable the new behaviour that bug 480350 introduced (matching against the list of open tabs), but it looks like there is no way to do that at the moment.
Throw me into the pool of people who would like to see a way to disable the "switch to this tab" off.  I'm perfectly fine clicking out of the drop-down and clicking onto that tab if I want to switch.
Please feel free to vote.

As Kim said the awesomebar uses the perf browser.urlbar.default.behavior mentioned above.  The current default is set to 0: show all.  Here is how it works:

// The default behavior for the urlbar can be configured to use any combination
// of the restrict or match filters with each additional filter restricting
// more (intersection). Add the following values to set the behavior as the
// default: 1: history, 2: bookmark, 4: tag, 8: title, 16: url, 32: typed,
// 64: javascript, 128: tabs
// E.g., 0 = show all results (no filtering), 1 = only visited pages in history,
// 2 = only bookmarks, 3 = visited bookmarks, 1+16 = history matching in the url
(In reply to comment #5)
> Here is how it works:

I tried to describe in bug 564395 how it actually works at the moment.
Blair are we going to get this fixed?
Keywords: uiwanted
(In reply to comment #0)
> Rather than a dropdown menu listing every possible combination, a series of
> checkboxes could be used - a checkbox for each item, enabling or disabling that
> type of result.

Indeed. Having a list of checkboxes that say:

[x] History
[x] Bookmarks
[x] Open Tabs

…would be the proper solution here. I assume this has no impact on strings, but I could be wrong?
(In reply to comment #9)
> …would be the proper solution here. I assume this has no impact on strings, but
> I could be wrong?

Well, we don't have a "Open Tabs" string.
Please can we get something landed before FF 4.0 final to disable "Switch to Tabs" as it impedes my productivity with Firefox.
(In reply to comment #11)
> Please can we get something landed before FF 4.0 final to disable "Switch to
> Tabs" as it impedes my productivity with Firefox.

you know you can override switch to tab just by pressing shift when the autocomplete panel is open?
Marco, that's not what is being requested, what is being requested is a way to completely 100% disable a feature that some people simply do not want.  I notice now there is a way to display only typed in urls... funny thing is that 3 years ago we were told it was completely impossible to revert to Firefox 2.0 urlbar behavior, and yet with no fan-fare here it is again.  It's cool that Mozilla wants to encourage people to try new things, but forcing them to live with it is not cool. I personally do not want to have to press Shift or Alt or whatever every single time I type into the urlbar, that's insane.
 (In reply to comment #12)
> 
> you know you can override switch to tab just by pressing shift when the
> autocomplete panel is open?

Yes I do know about that hidden shortcut.  But why should I have to press Shift every time I load new URLs to override the switch to tab feature which provides me no value? Again, that impedes productivity.  Just make the feature configurable?  There is a descent sized portion of users who dislike this feature, and I know for a fact that the users where I work will be GREATLY frustrated with this when they upgrade to 4.0 as the style of their work involves opening multiple tabs to the same site/URL.
Count me in, I'm already starting to get bed dreams because of this switch to tab issue :/
Please, don't post useless me-too comments, we are already aware some users are unhappy about not being able to disable switch to tab.
Posting here won't make a positive difference on fixing the bug, most likely the opposite, nobody wants to read hundreds comments bugs.

(In reply to comment #10)
> Well, we don't have a "Open Tabs" string.

As a mental association, we could temporarily use the same string we use on the location bar itself (action.switchToTab.label) to indicate a switch to tab result. Even if it's not a dedicated string, the association of the checkbox to the kind of results is 1:1, actually the feature is called by users based on this label (if you ask an italian user he won't call it "switch to tab" or "open tabs", he will call it "passa alla scheda", that is exactly the label).
[x] History
[x] Bookmarks
[x] Switch to tab
Faaborg or Limi, do you think using the label to indicate the feature could work as association?
Do you think it's worth adding the option to help those users who find the feature interrupting their productivity?
Keywords: uiwanted
(In reply to comment #17)
> Faaborg or Limi, do you think using the label to indicate the feature could
> work as association?

It was in the original specification, but got dropped, so — yes. ;)

I don't think we can reuse labels like this, though. Just because it works in Italian, doesn't mean it'll work in every other language.

We should fix it, but I think it'll have to after Firefox 4 because of feature freeze and since we're so late in the cycle.
(In reply to comment #18)
> (In reply to comment #17)
> I don't think we can reuse labels like this, though. Just because it works in
> Italian, doesn't mean it'll work in every other language.

Sure I was mostly basing on how users "name" our features, that usually happens based on what identifies them on the ui.
Depends on: 631671
Whiteboard: [places-next-wanted]
I just expected it to be normal, to add an option for new features like "switch to tab". Because at the moment, there are options for what the urlBar can show, so if adding a new thing it can show => add a new option for it. This would be straight forward and intuitive.
I am no firefox developer [not yet ;)]  but I thought, if there is already the "browser.urlbar.default.behavior" option, which is already able to represent all combinations of what to show, it should be easy to add an option in the preferences dialog. And I am surprised that something like "adding a new String" seems to be such a complicated thing.
ok it's even more bad as I thought. 
// The default behavior for the urlbar can be configured to use any combination
// of the restrict or match filters with each additional filter restricting
// more (intersection).

Intersection?? So I cannot say, show bookmarks, show history, but NOT TABS!  What's this??? Even in the backend, there is no way to configure it?? Annoying.
Blocks: 648394
Blocks: 751709
So i've noticed that Bug 793723 has been duped to this bug, but from reading the comments i understand that this is meant for the visual changes in the options. So i guess that is more of a long term solution, the problem right now is that there is no about:config preference to turn switch-to-tab off. I found out pressing the "Alt" key disables it, so you must have a function that checks for that.

Would it be possible to just add a boolean variable and check for that as well?
At least for the time being?
Bug 546253 hasn't really helped for those who want switch to tab entries to be excluded.  If anything it has made things worse.  The problem is that the desired combinations can't be expressed in a "flat" bitmap.

So currently:
- if you have no bits set then everything is shown, including switch to tab entries, all good so far;
- if you have the tabs bit set then everything else is excluded and the switch to tab entries are still shown, probably still correct;
- if you have the tabs bit not set and another "restrict" bit set then entries are restricted in line with the bit set, but switch to tab entries are still shown also, undesirable.

The correct behaviour would be to have the bit work like all the others, which is to exclude everything except the set bit.  This was bug 546253 but the fix didn't actually do it.  The real fix, finger's crossed to be implemented by this bug, is to allow combinations more complex than a "flat" bit mask so that types of autocomplete entry can be separately excluded or included.  Then switch to tab can be switched off independently of any other autocomplete settings.
Is it not possible to put a check-box for each individual element?

* History
* Bookmarks
* Switch to tab

I don't know what's the hold up here, but i'd really like to see this fixed soon.
(In reply to Ian Nartowicz from comment #27)
> Bug 546253 hasn't really helped for those who want switch to tab entries to
> be excluded.

That's more closely related to bug 751709. We plan to mitigate it by fixing bug 810930.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #29)
> (In reply to Ian Nartowicz from comment #27)
> > Bug 546253 hasn't really helped for those who want switch to tab entries to
> > be excluded.
> 
> That's more closely related to bug 751709. We plan to mitigate it by fixing
> bug 810930.

Don't both those bugs apply only to the autofill?  Ie. to the urlbar textbox contents only, but not to the autocomplete dropdown?
(In reply to Ian Nartowicz from comment #30)
> Don't both those bugs apply only to the autofill?  Ie. to the urlbar textbox
> contents only, but not to the autocomplete dropdown?

Yes, the reference bug for dropdown behavior is this one, until we figure new preferences for the location bar it's not really useful to have further bugs about adding more choices.
Blocks: 937946
Whiteboard: [places-next-wanted] → [places-next-wanted] p=0
"Switch to tab" breaks important workflows. Perhaps it works for others, but it is just awful for me. Type part of the URL, press Down, press Enter - to open the site. Except that this won't always happen. Maybe you have another tab open somewhere, completely unrelated to what you're doing right now, but the "awesome" bar thinks you probably want to switch to it. Guessing my intentions wrong is worse than not guessing at all. It must be possible to disable this mis-prediction.

The Alt workaround unfortunately opens a new tab, so not really a good solution.
(In reply to Roman from comment #33)
> "Switch to tab" breaks important workflows. Perhaps it works for others, but
> it is just awful for me.

You should report that to a new bug.
All of the bugs requesting the ability to disable Switch to Tab thus far have been duped to this one.
Yes, I think makes sense to keep switch to tab into this bug until the work here starts, the new prefs should also allow to disable it.
Also bug 631671 may be a good target, since the blacklist may be defined so that any page is blacklisted.
I understand you don't know how the UI is going to look yet, but can someone at least make an about:config pref to disable the feature? It'll have to be done at some point anyway and it shouldn't be too hard to do.
Whiteboard: [places-next-wanted] p=0 → p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Depends on: 996916
Whiteboard: p=0 → p=5
Points: --- → 5
Flags: qe-verify?
Whiteboard: p=5
Blocks: 1041743
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Iteration: --- → 35.2
Blocks: 1071461
Flags: qe-verify? → qe-verify+
QA Contact: jbecerra
It's the approach for making the urlbar suggestions configurable from multiple boolean preferences. It's not finished yet (e.g.: lacks tests, accessibility keys), but feedback based on what it's implemented is very good.

It is not 100% backward compatible with the old version (and it can't be because now it's a union instead of an intersection of restrictions), but it should be compatible with the default behavior and with what the user could choose before.
Attachment #8494884 - Flags: feedback?(mak77)
Attachment #8494884 - Flags: feedback?(adw)
Comment on attachment 8494884 [details] [diff] [review]
[WIP Patch] Make Prefs UI for what to suggest in location bar friendly

Review of attachment 8494884 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but I'm interested in what Marco thinks.  Paolo would also be a good person to ask for feedback if Marco isn't available.

One thing is that I'm not sure how this affects restriction tokens.  For example, if you type "% foo" in the awesomebar, then you'll see only open tabs that match foo.  That's pretty useful behavior that ideally this wouldn't change.  Tokens are parsed here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#612  My guess is that your patch does change that, so that tokens are treated as a union instead of intersection, due to how Search.prototype.setBehavior and getBehavior work.  If we do want to preserve token behavior, it would probably be simple to keep another _restrictionBehavior alongside _behavior, and then hasBehavior would check both.

::: browser/app/profile/firefox.js
@@ +361,5 @@
> +pref("browser.urlbar.suggest.bookmark", true);
> +pref("browser.urlbar.suggest.openpage", true);
> +pref("browser.urlbar.suggest.search",   true);
> +pref("browser.urlbar.suggest.tag",      false);
> +pref("browser.urlbar.suggest.typed",    false);

We talked about this earlier, but I just want to call it out here.  tag and typed default to false so that if, for example, you have only Bookmarks checked in the UI, and there are no checkboxes for tag and typed, then you really only see bookmarks and not also tag and typed results.  If they defaulted to true, you would see those results which may be confusing.  (At least the typed results, since tag results are also bookmark results.)

::: browser/components/preferences/in-content/privacy.xul
@@ +243,5 @@
> +            preference="browser.urlbar.suggest.bookmark"/>
> +  <checkbox id="openpageSuggestion" label="&locbar.openpage.label;"
> +            accesskey="&newWindowsAsTabs.accesskey;"
> +            preference="browser.urlbar.suggest.openpage"/>
> +  <checkbox id="autofill" label="&locbar.autofill.label;"

It doesn't really make sense to put autofill here because it's not a type of suggestion that appears in the results list.  I'm not sure it should even be in the Privacy pref pane.  Maybe General or Advanced?  But in any case, adding a checkbox for autofill isn't part of this bug, is it?

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +24,5 @@
>  <!ENTITY  locbar.post.label             "">
>  <!ENTITY  locbar.both.label             "History and Bookmarks">
>  <!ENTITY  locbar.history.label          "History">
>  <!ENTITY  locbar.bookmarks.label        "Bookmarks">
> +<!ENTITY  locbar.openpage.label         "Opened tabs">

"Opened" may imply all pages you've ever opened, so I would call this "Open tabs".

::: toolkit/components/places/UnifiedComplete.js
@@ +40,5 @@
> +const PREF_SUGGEST_BOOKMARK =       [ "suggest.bookmark",       true];
> +const PREF_SUGGEST_SEARCH =         [ "suggest.search",         true];
> +const PREF_SUGGEST_OPENPAGE =       [ "suggest.openpage",       true];
> +const PREF_SUGGEST_TAG  =           [ "suggest.tag",            true];
> +const PREF_SUGGEST_TYPED =          [ "suggest.typed",          true];

The last two default to false, right?

@@ +1258,3 @@
>  
> +    // autoFill doesn't search titles.
> +    if (this.hasBehavior("title"))

Why this change?
Attachment #8494884 - Flags: feedback?(adw) → feedback+
(In reply to Drew Willcoxon :adw (Away 9/29–10/14) from comment #42)
> It doesn't really make sense to put autofill here because it's not a type of
> suggestion that appears in the results list.  I'm not sure it should even be
> in the Privacy pref pane.  Maybe General or Advanced?  But in any case,
> adding a checkbox for autofill isn't part of this bug, is it?

Do we really want this exposed in UI at all?
(In reply to Drew Willcoxon :adw (Away 9/29–10/14) from comment #42)
> One thing is that I'm not sure how this affects restriction tokens.  For
> example, if you type "% foo" in the awesomebar, then you'll see only open
> tabs that match foo.  That's pretty useful behavior that ideally this
> wouldn't change.

I agree that keeping the current restriction behavior, for now, should be the safest path forward here.

> ::: browser/app/profile/firefox.js
> @@ +361,5 @@
> > +pref("browser.urlbar.suggest.bookmark", true);
> > +pref("browser.urlbar.suggest.openpage", true);
> > +pref("browser.urlbar.suggest.search",   true);
> > +pref("browser.urlbar.suggest.tag",      false);
> > +pref("browser.urlbar.suggest.typed",    false);
> 
> We talked about this earlier, but I just want to call it out here.  tag and
> typed default to false so that if, for example, you have only Bookmarks
> checked in the UI, and there are no checkboxes for tag and typed, then you
> really only see bookmarks and not also tag and typed results.  If they
> defaulted to true, you would see those results which may be confusing.  (At
> least the typed results, since tag results are also bookmark results.)

FYI, tags in future may not be bookmarks, or better, they will be a different kind of bookmark (a tagged page that is kept until you remove the tag from it).

That said, I find this much confusing, typed is sort of a subfilter, should be suggest.history.onlyTyped imo (doesn't make sense to limit to typed bookmarks or to typed open pages, right?). It should not be presented as if it would work like any other suggest option. you might want history, or typed history, that's it.

Regarding tags, I think we should not expose it as a pref. it's useful to restrict results, and it's useful to control the match behavior (match on title, url, tag), but not really as an option about what to suggest (at least for now).
I'd vote to remove it for now.

> ::: browser/components/preferences/in-content/privacy.xul
> @@ +243,5 @@
> > +            preference="browser.urlbar.suggest.bookmark"/>
> > +  <checkbox id="openpageSuggestion" label="&locbar.openpage.label;"
> > +            accesskey="&newWindowsAsTabs.accesskey;"
> > +            preference="browser.urlbar.suggest.openpage"/>
> > +  <checkbox id="autofill" label="&locbar.autofill.label;"
> 
> It doesn't really make sense to put autofill here because it's not a type of
> suggestion that appears in the results list.  I'm not sure it should even be
> in the Privacy pref pane.  Maybe General or Advanced?  But in any case,
> adding a checkbox for autofill isn't part of this bug, is it?

Yes, let's keep autoFill for a separate bug. Here we are defining the underlying dataset, I think we will need (in future) a section to define the behavior (autofill, match on title/uri/tags, ...). With a separate bug we can better discuss costs and gains.
I agree that from a user perspective, typed urls are relevant only for history results. Right now, there is logic for typed bookmarks too (http://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#1279) but I'll try to get rid of it.

Removing the preference for tags will brake current behavior for users who have a custom default.behavior value and have the tag flag set (although I don't think there is someone who is using only tags).
Comment on attachment 8494884 [details] [diff] [review]
[WIP Patch] Make Prefs UI for what to suggest in location bar friendly

Review of attachment 8494884 [details] [diff] [review]:
-----------------------------------------------------------------

So, honestly I think we can take a similar change only at the beginning of a merge, the risks of regressions here are very high. I'm not optimist that we can have this in 35.

Though, if internally we'd rebuild the old behavior properties starting from the new prefs, and only later change that to simplify the code, then we could probably move faster.

::: browser/app/profile/firefox.js
@@ +343,5 @@
>  pref("browser.urlbar.restrict.typed", "~");
>  pref("browser.urlbar.match.title", "#");
>  pref("browser.urlbar.match.url", "@");
>  
> +// ** Start deprecated prefs (since v35) **

we don't usually keep "deprecated prefs" in this this file, just remove them.

::: browser/components/nsBrowserGlue.js
@@ +1343,5 @@
>      notification.persistence = -1; // Until user closes it
>    },
>  
>    _migrateUI: function BG__migrateUI() {
> +    const UI_VERSION = 35;

this is not the firefox version, it's just an incremental counter, just +1 it.

@@ +1602,5 @@
> +      // If the default behavior is:
> +      //    -1  - all new "...suggest.*" preferences will be false
> +      //     0  - all new "...suggest.*" preferences will use the default values
> +      //   > 0  - all new "...suggest.*" preferences will be inherited
> +      if (defaultBehavior === -1) {

nit: no reason for ===

@@ +1611,5 @@
> +        types.forEach(type => {
> +          Services.prefs.setBoolPref("browser.urlbar.suggest." + type,
> +            !!(defaultBehavior & Ci.mozIPlacesAutoComplete["BEHAVIOR_" + type.toUpperCase()]));
> +        });
> +      }

nit: I'd keep a single forEach and calculate the value inside it, should be shorter and still readable

::: browser/components/preferences/in-content/privacy.js
@@ +35,1 @@
>      setEventListener("privacy.sanitize.sanitizeOnShutdown", "change",

I don't know what's the status of in-content preferences, are we going to ship them in Firefox 33?
Mostly I'm not sure what's the policy to keep the old dialog prefences in sync with in-content ones, with your changes the dialog prefpane wouldn't work anymore.

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +21,5 @@
>  
>  <!ENTITY  locbar.pre.label              "When using the location bar, suggest:">
>  <!ENTITY  locbar.pre.accessKey          "u">
>  <!ENTITY  locbar.post.label             "">
>  <!ENTITY  locbar.both.label             "History and Bookmarks">

if we also update the dialog prefs, this can go away, along with "Nothing"

::: toolkit/components/places/SQLFunctions.cpp
@@ +362,5 @@
>      int32_t openPageCount = aArguments->AsInt32(kArgIndexOpenPageCount);
>  
> +    // Make sure that we match all the filter requirements and that the
> +    // corresponding condition is true if at least a given restriction is active.
> +    bool matches =

this change is likely breaking restrictions.

::: toolkit/components/places/UnifiedComplete.js
@@ +389,3 @@
>      // Further restrictions to apply for "empty searches" (i.e. searches for "").
>      store.emptySearchDefaultBehavior = store.defaultBehavior |
>                                         prefs.get(...PREF_EMPTY_BEHAVIOR);

Is this still correct? the empty behavior by default should be only typed history, unless the user excluded history, then likely should be nothing.

@@ +1107,5 @@
> +    let types = ["history", "bookmark", "openpage", "tag", "typed"];
> +    // An Array with the suggestions which will be applied
> +    let suggestionArray = types.filter(type => this.hasBehavior(type));
> +    // If all suggestion preferences are set to true, then don't
> +    // apply any filtering and return the default query.

I'd exclude tag and typed from these...

@@ +1115,5 @@
> +    // If all suggestion preferences are false, then don't apply any filtering
> +    // and set `_behavior` property to 0. There should be no results in the
> +    // awesomebar.
> +    if (!suggestionArray.length) {
> +      this._behavior = 0;

I think in this case Prefs.enable should be set to false (internally), so we'd not even reach this point.

@@ +1136,5 @@
> +    if (this.hasBehavior("typed")) {
> +      conditions.push("h.typed = 1");
> +    }
> +
> +    return conditions.length ? defaultQuery("AND (" + conditions.join(" OR ") + ")")

I think this should AND excluding conditions, like if bookmarks are not enabled, should add "AND visit_count > 0", if visits are not enabled (but bookmarks are) should add "AND bookmarked".

the problem using an OR like you did is that it will be extremely unefficient.
Attachment #8494884 - Flags: feedback?(mak77)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #46)
> I don't know what's the status of in-content preferences, are we going to
> ship them in Firefox 33?
> Mostly I'm not sure what's the policy to keep the old dialog prefences in
> sync with in-content ones, with your changes the dialog prefpane wouldn't
> work anymore.

In-content prefs are Nightly-only at the moment. We need to keep both versions working.
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #46)
> nit: no reason for ===

This is one of the obvious places where we differ from almost every other open source project that uses JavaScript, and IMO has become one of the many small papercuts for new contributors. In the interests of working towards lowing barriers, I don't think we should be discouraging use of === (if anything, we should be discouraging use of ==).
(In reply to Alex Bardas :alexbardas from comment #45)
> Removing the preference for tags will brake current behavior for users who
> have a custom default.behavior value and have the tag flag set (although I
> don't think there is someone who is using only tags).

I know that, I think it's ok to break it, as well ask typed bookmarks. it's use-cases created just for the sake of completeness, but their usefulness is nonexistent.
Made it compatible with old preferences pop-up. Token restrict searches are now correctly handled.

All xpcshell tests from unifiedcomplete folder are now passing, had to rewrite a few tests.
Attachment #8494884 - Attachment is obsolete: true
Attachment #8497352 - Flags: feedback?(mak77)
Iteration: 35.2 → 35.3
Same desc as the previous patch, but with some improved comments.
Attachment #8497352 - Attachment is obsolete: true
Attachment #8497352 - Flags: feedback?(mak77)
Attachment #8497676 - Flags: feedback?(mak77)
Comment on attachment 8497676 [details] [diff] [review]
[Patch 2] Make Prefs UI for what to suggest in location bar friendly

Review of attachment 8497676 [details] [diff] [review]:
-----------------------------------------------------------------

This is moving towards the right direction.

The only unfortunate thing is I take a huge amount of time to review patches touching these AC behaviors... oh well.

::: browser/app/profile/firefox.js
@@ +346,5 @@
>  
>  // The default behavior for the urlbar can be configured to use any combination
> +// of the match filters with each additional filter adding more results (union).
> +pref("browser.urlbar.suggest.history",              true);
> +pref("browser.urlbar.suggest.history.onlyTyped",    false);

since this is not part of the union, I'd move it apart from this paragraph, to a specific restrictions paragraph.

::: browser/components/nsBrowserGlue.js
@@ +1605,5 @@
> +      // If the default behavior is:
> +      //    -1  - all new "...suggest.*" preferences will be false
> +      //     0  - all new "...suggest.*" preferences will use the default values
> +      //   > 0  - all new "...suggest.*" preferences will be inherited
> +      types.forEach(type => {

for (let type of types), no reason to pay cost of a function

@@ +1608,5 @@
> +      //   > 0  - all new "...suggest.*" preferences will be inherited
> +      types.forEach(type => {
> +        let prefValue = true;
> +        if (defaultBehavior == -1) {
> +          prefValue = false;

let prefValue = defaultBehavior == 0;
if (defaultBehavior > 0) ...

@@ +1615,5 @@
> +        }
> +        Services.prefs.setBoolPref("browser.urlbar.suggest." + type, prefValue);
> +      });
> +
> +      // Old typed behivor will be used only for results from history.

typo: behivor

::: toolkit/components/places/SQLFunctions.cpp
@@ +369,5 @@
> +                  (HAS_BEHAVIOR(TYPED) && !typed) ||
> +                  (HAS_BEHAVIOR(BOOKMARK) && !bookmark) ||
> +                  (HAS_BEHAVIOR(TAG) && tags.IsVoid()) ||
> +                  (HAS_BEHAVIOR(OPENPAGE) && openPageCount == 0)
> +                );

If my boolean memory still works, this is the same as

(!HAS_BEHAVIOR(HISTORY) || visitCount > 0) &&
(!HAS_BEHAVIOR(TYPED) || typed) &&
(!HAS_BEHAVIOR(BOOKMARK) || bookmark) &&
(!HAS_BEHAVIOR(TAG) || !tags.IsVoid()) &&
(!HAS_BEHAVIOR(OPENPAGE) || openPageCount > 0)

that would remove the need for the wrapping !()

::: toolkit/components/places/UnifiedComplete.js
@@ +39,5 @@
>  
> +const PREF_SUGGEST_HISTORY =        [ "suggest.history",        true];
> +const PREF_SUGGEST_BOOKMARK =       [ "suggest.bookmark",       true];
> +const PREF_SUGGEST_SEARCH =         [ "suggest.search",         true];
> +const PREF_SUGGEST_OPENPAGE =       [ "suggest.openpage",       true];

nit: whitespace before ] for coherence with the other definitions

@@ +388,5 @@
>      // Further restrictions to apply for "empty searches" (i.e. searches for "").
> +    // The empty behavior by default should be only typed history. If the user excluded
> +    // history, then it should default to nothing.
> +    store.emptySearchDefaultBehavior = prefs.get(...PREF_EMPTY_BEHAVIOR) |
> +                                       Ci.mozIPlacesAutoComplete["BEHAVIOR_RESTRICT"];

This is probably changing the empty behavior and I'm not sure it's a bad thing, as it was previously, was basically unreadable/unmanageable.

The problem though is that this doesn't seem to do what the comment suggest, how is it returning nothing if history is not enabled? how is it returning typed history if the pref is set to zero?

I think we should remove support for the pref completely, and rather try to come up with a good behavior based on what defaultBehavior is.
That is: if history is enabled empty behavior is typed history, if bookmarks is enabled (but not history) only bookmarks, if both bookmarks and history are disabled, openpages (basically if, elseif, else)

@@ +621,5 @@
> +        // set the restrict bit to 1 (to intersect the search results).
> +        if (!foundToken) {
> +          foundToken = true;
> +          // Do not take into account previous behavior (e.g.: history, bookmark)
> +          this._behavior = 0;

This means we are basically throwing away the default behavior the user set in the prefs?
So, if the user wants only history but he types the restrict bookmarks special char, we override the pref and suggest bookmarks.

While it's a behavior change from before, I think I may be fine with it. since these tokens are undiscoverable this looks like an additional value given to the user.

@@ +704,4 @@
>                      this._searchQuery ];
>  
> +    if (this.hasBehavior("openpage")) {
> +      queries.push(this._switchToTabQuery);

this is wrong cause it's moving switch to tab results at the end of the popup (the results are added in the order of the queries). you should use queries.splice(1, 0, this._switchToTabQuery);

@@ +1282,3 @@
>      // Note: "openpages" behavior is supported by the default query.
>      //       _switchToTabQuery instead returns only pages not supported by
>      //       history and it is always executed.

this comment looks misplaced, should probably be moved where we add _switchToTabQuery

@@ +1386,4 @@
>  
> +    // autoFill doesn't search titles.
> +    if (this.hasBehavior("restrict") &&
> +        (this.hasBehavior("title") || this.hasBehavior("tags")))

First of all, this.hasBehavior("tags") is a typo (not your fault), it should be this.hasBehavior("tag").

I think originally we went too far with behaviors, mixing up datasets (bookmarks, history, openpage, search, tag), restrictions(typed), filters(javascript), and fields-to-search (url, title).

so, "restrict" here is a bit unclear, cause restrict + tags (based on what we do) makes sense, but "restrict"+"title" doesn't make any sense... maybe this should be
 if (this.hasBehavior("title") ||
     (this.hasBehavior("restrict") && this.hasBehavior("tags")))

I hope one day we'll revise mozIPlacesAutoComplete.idl clearly splitting the different pieces.

@@ +1409,5 @@
>     *         database with and an object containing the params to bound.
>     */
>    get _hostQuery() {
> +    let typed = Prefs.autofillTyped || (this.hasBehavior("history") &&
> +                                        this.hasBehavior("typed"));

I think that setting "typed" should automatically set "history", should be an easy change and *should* not have big consequences

@@ +1455,5 @@
>     *         database with and an object containing the params to bound.
>     */
>    get _urlQuery()  {
> +    let typed = Prefs.autofillTyped || (this.hasBehavior("history") &&
> +                                        this.hasBehavior("typed"));

ditto

@@ +1468,5 @@
>          query_type: QUERYTYPE_AUTOFILL_URL,
>          searchString: this._autofillUrlSearchString,
>          matchBehavior: MATCH_BEGINNING_CASE_SENSITIVE,
> +        searchBehavior: Ci.mozIPlacesAutoComplete.BEHAVIOR_URL |
> +                        Ci.mozIPlacesAutoComplete.BEHAVIOR_RESTRICT

what's the point of adding restrict here? shouldn't make a difference on BEHAVIOR_URL and the query is already filtering on typed.

::: toolkit/components/places/mozIPlacesAutoComplete.idl
@@ +97,5 @@
>    const long BEHAVIOR_OPENPAGE = 1 << 7;
>  
>    /**
> +   * Use intersection between all the previously defined searches, instead
> +   * of union, when the restrict bit is set.

this is incorrect, the intersection is only between history, typed, bookmark, tag and openpage.

::: toolkit/components/places/tests/unifiedcomplete/test_autoFill_default_behavior.js
@@ +124,5 @@
>    });
>  
>    // RESTRICT BOOKMARKS.
> +  Services.prefs.setBoolPref("browser.urlbar.suggest.history", false);
> +  Services.prefs.setBoolPref("browser.urlbar.suggest.history.onlyTyped", false);

I think history = false should make the code ignore onlyTyped value

::: toolkit/components/places/tests/unifiedcomplete/test_special_search.js
@@ +389,5 @@
>  
>    do_log_info("foo -> default history, is star");
> +  setSuggestPrefsToFalse();
> +  Services.prefs.setBoolPref("browser.urlbar.suggest.history", true);
> +  Services.prefs.setBoolPref("browser.urlbar.suggest.bookmark", true);

I think here you changed the outcome of some of the tests... the new tests you added are fine but we also need to keep some of the old tests.
What you are missing here is a test for only bookmarked page and a test for history=false, onlytyped=true, bookmark=true (that should basically ignore onlytyped)

What I just figured out is also that by providing the new prefs, we have no more a pref to specify WHERE to search, uri and title, only uri or only title. Though we have tokens in case someone wants to restrict a search. tag is also quite fancy cause it works both as a dataset and a searchfield...
I guess for now we can see how it's going without those and if we see lots of requests it will be possible to add about:config prefs for them.
Attachment #8497676 - Flags: feedback?(mak77) → feedback+
ah please also bump the UUID of the autocomplete idl.
I've split the current patch in 2 smaller patches:
    1. Deals with the new preferences, migration and changes in Unifiedcomplete
    2. Deals with changes in nsPlacesAutocomplete (applying the changes from unifiedcomplete there)

Other changes:
    - applied the suggestions from Marco's previous feedback
    - if the user typed some special restrict chars and if a result has tags, make sure to also show the tags (modified tests to allow this behavior)
    - added tests for the migration
    - changed the logic for the emptySearchBehavior
    - fix nsPlacesAutocomplete + tests


Adding a few more tests is worth considering (the test suite for unifiedcomplete is quite comprehensive). Also, the ui preferences do not have access keys yet (they can be decided in a follow-up bug maybe).

Is nsPlacesAutocomplete still used? ( == how many users have unifiedcomplete pref set to false?)
Attachment #8497676 - Attachment is obsolete: true
Attachment #8499375 - Flags: review?(mak77)
The fix for nsPlacesAutocomplete (fixed the tests, did not add more tests though, don't know if it is still used)
Attachment #8499377 - Flags: review?(mak77)
Blocks: 631671
No longer depends on: 631671
Points: 5 → 8
Comment on attachment 8499375 [details] [diff] [review]
Make Prefs UI for what to suggest in location bar friendly v3

Review of attachment 8499375 [details] [diff] [review]:
-----------------------------------------------------------------

First, we are not going to land this in 35, it will make 36. So, please do not land this until the merge has happened, even if that means carrying over the bug.
In this case the downside, that is risk of awesomebar regressions just before the merge point, is much bigger than carry-over points sadness.

A side problem with updating the old autocomplete to use these new prefs, is that it's going to break other Places consumers, but we can't avoid that if we want to be able to disable unified complete easily, so please file a bug to notify at least Seamonkey about this change and point them to this bug. They set default.behavior to 1 (only history) http://mxr.mozilla.org/comm-central/source/suite/browser/browser-prefs.js#157 so looks to me they should just set suggest.history to true, the others to false.

Also, since we are delaying landing regardless, I'd suggest to take some hours to do some manual testing, even if this is something more for QA, you know better which pieces of code you have modified and which are the most problematic cases, so you can easily find issues.  This is a very critical part of the ui and aftr the migration we won't be able to restore old user's settings.

btw, this is well executed afaict, so r=me with the following comments addressed.

::: browser/app/profile/firefox.js
@@ +352,5 @@
> +pref("browser.urlbar.suggest.search",               true);
> +
> +// Restrictions to current suggestions can also be applied (intersection).
> +// Typed suggestion works only if history is set to true.
> +pref("browser.urlbar.suggest.history.onlyTyped",    false);

Sync is syncing the old pref through
services.sync.prefs.sync.browser.urlbar.default.behavior
could you please remove that pref so we stop syncing that obsolete pref?

It should probably start syncing the new prefs instead... what about filing a follow-up bug to do that? I think it's safer to have that decision in a separate bug.

::: browser/components/nsBrowserGlue.js
@@ +304,5 @@
>              this._showUpdateNotification();
>          }
>          else if (data == "force-ui-migration") {
>            this._migrateUI();
> +          Services.obs.notifyObservers(null, "force-ui-migration-complete", "");

what's the point of this notification? _migrateUI is synchronous, so once you have notified force-io-migration, you know _migrateUI ran already...

::: browser/components/places/tests/unit/test_browserGlue_urlbar_defaultbehavior_migration.js
@@ +8,5 @@
> +const TOPIC_BROWSERGLUE_TEST_COMPLETE = "force-ui-migration-complete";
> +const DEFAULT_BEHAVIOR_PREF = "browser.urlbar.default.behavior";
> +
> +let gBrowserGlue = Cc["@mozilla.org/browser/browserglue;1"]
> +                    .getService(Ci.nsIObserver);

nit: align dot with [

@@ +25,5 @@
> +  }
> +  Services.prefs.clearUserPref("browser.migration.version");
> +};
> +
> +function setUp(aDefaultBehavior = 0) {

the default argument value only covers a single case... for which it would be better to explicitly pass the argument 0.

I'd also rename this to setupBehaviorAndMigrate(aBehavior)

@@ +32,5 @@
> +  Services.prefs.setIntPref("browser.migration.version", UI_VERSION - 1);
> +  Services.prefs.setIntPref(DEFAULT_BEHAVIOR_PREF, aDefaultBehavior);
> +};
> +
> +function promiseMigrationComplete() {

this helper sounds very pointless, as I said.
just make setUp call gBrowserGlue.observe(null, TOPIC_BROWSERGLUE_TEST, TOPICDATA_BROWSERGLUE_TEST);

::: browser/components/preferences/in-content/privacy.xul
@@ +231,3 @@
>    <caption><label>&locationBar.label;</label></caption>
> +  <label id="locationBarSuggestionLabel"
> +         control="locationBarSuggestion"

this is broken (locationBarSuggestion doesn't exist anymore), and when using the accesskey it will not point to anything.
&locbar.pre.accessKey; should be removed since it cannot point to a single control, and we need single access keys for each suggestion.
pay attention to not dupe accesskeys when adding them.

::: browser/components/preferences/privacy.xul
@@ +248,5 @@
>        <caption label="&locationBar.label;"/>
>  
> +      <label id="locationBarSuggestionLabel"
> +             control="locationBarSuggestion"
> +             accesskey="&locbar.pre.accessKey;">&locbar.pre.label;</label>

ditto

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +20,5 @@
>  <!ENTITY  locationBar.label             "Location Bar">
>  
>  <!ENTITY  locbar.pre.label              "When using the location bar, suggest:">
>  <!ENTITY  locbar.pre.accessKey          "u">
>  <!ENTITY  locbar.post.label             "">

both locbar.pre.accessKey and locbar.post.label should go away.

::: mobile/android/app/mobile.js
@@ +309,5 @@
> +pref("browser.urlbar.suggest.search",               true);
> +
> +// Restrictions to current suggestions can also be applied (intersection).
> +// Typed suggestion works only if history is set to true.
> +pref("browser.urlbar.suggest.history.onlyTyped",    false);

just remove the existing stuff from mobile.js, mobile doesn't use Places, these prefs have been copied ages ago and never removed.
I actually think that all of the following prefs are currently unused by mobile:

line 301 -- pref("browser.urlbar.default.behavior", 0);
line 302 -- pref("browser.urlbar.default.behavior.emptyRestriction", 0);
line 310 -- pref("browser.urlbar.clickSelectsAll", true);
line 311 -- pref("browser.urlbar.doubleClickSelectsAll", true);
line 312 -- pref("browser.urlbar.autoFill", false);
line 313 -- pref("browser.urlbar.matchOnlyTyped", false);
line 314 -- pref("browser.urlbar.matchBehavior", 1);
line 315 -- pref("browser.urlbar.filter.javascript", true);
line 316 -- pref("browser.urlbar.maxRichResults", 24); // increased so we see more results when portrait
line 317 -- pref("browser.urlbar.search.chunkSize", 1000);
line 318 -- pref("browser.urlbar.search.timeout", 100);
line 319 -- pref("browser.urlbar.restrict.history", "^");
line 320 -- pref("browser.urlbar.restrict.bookmark", "*");
line 321 -- pref("browser.urlbar.restrict.tag", "+");
line 322 -- pref("browser.urlbar.match.title", "#");
line 323 -- pref("browser.urlbar.match.url", "@");
line 324 -- pref("browser.urlbar.autocomplete.search_threshold", 5);

You might ask margaret or mfinkle for a confirmation.

::: toolkit/components/places/UnifiedComplete.js
@@ +1273,5 @@
> +    let conditions = [];
> +    // Enforce ignoring the visit_count index, since the frecency one is much
> +    // faster in this case.  ANALYZE helps the query planner to figure out the
> +    // faster path, but it may not have up-to-date information yet.
> +    if (this.hasBehavior("history")) {

please move the comment inside the if body

@@ +1403,4 @@
>  
> +    // autoFill doesn't search titles.
> +    if (this.hasBehavior("title") ||
> +        (this.hasBehavior("restrict") || this.hasBehavior("tag")))

there is something wrong in this boolean condition, looks like the last || should be an && (that likely means we are missing a test for this? May you add it please?)

@@ +1474,5 @@
>      let typed = Prefs.autofillTyped || this.hasBehavior("typed");
> +    let bookmarked = this.hasBehavior("bookmark") && !this.hasBehavior("history");
> +    let searchBehavior = Ci.mozIPlacesAutoComplete.BEHAVIOR_URL;
> +
> +    // Enable searches in typed history if autofillTyped pref is true.

not just the pref, we also check the typed behavior here.

::: toolkit/components/places/tests/unifiedcomplete/test_autoFill_default_behavior.js
@@ +176,1 @@
>    yield cleanup();

doesn't cleanup already clear the prefs so that cleanSuggestPrefs is useless?

::: toolkit/components/places/tests/unifiedcomplete/test_empty_search.js
@@ +91,1 @@
>    yield cleanup();

cleanup is already resetting the prefs
Attachment #8499375 - Flags: review?(mak77) → review+
Comment on attachment 8499377 [details] [diff] [review]
Fix nsPlacesAutoComplete after adding urlbar suggest prefs v1

Review of attachment 8499377 [details] [diff] [review]:
-----------------------------------------------------------------

fwiw, it's ok to not add more tests for the "legacy" autocomplete component, we must work on improving test coverage for unified.

Some of the previous comments may apply to the code you added here, so please double check.

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +301,5 @@
> +                                         :matchBehavior, :searchBehavior)
> +                ${conditions}
> +                ORDER BY h.frecency DESC, h.id DESC
> +                LIMIT :maxResults`;
> +   return query;

something strange happened to indentation here, looks like 1 space instead of 2 now?

@@ +347,5 @@
> +  XPCOMUtils.defineLazyGetter(this, "_customQuery", function() {
> +    return function(conditions = "") {
> +      return this._db.createAsyncStatement(baseQuery(conditions));
> +    }
> +  });

I'm not sure why this is a lazyGetter, and not just a method... There's nothing here that needs to be lazy

@@ +963,5 @@
> +    let conditions = [];
> +    // Enforce ignoring the visit_count index, since the frecency one is much
> +    // faster in this case.  ANALYZE helps the query planner to figure out the
> +    // faster path, but it may not have up-to-date information yet.
> +    if (this._hasBehavior("history")) {

move the comment inside the if please
Attachment #8499377 - Flags: review?(mak77) → feedback+
Iteration: 35.3 → ---
Added the new browser.urlbar.suggest.* preferences to mobile prefs and removed some unused preferences.

For more info about that check Marco's suggestions from comment 56.
Attachment #8502892 - Flags: review?(margaret.leibovic)
Minor changes after the last review
Attachment #8499377 - Attachment is obsolete: true
Attachment #8502896 - Flags: review?(mak77)
*  extracted the changes to mobile prefs into a new bug.
*  removed the browser tests for both privacy in-content/current location bar preferences. I don't know if new tests would bring any value, because it's a 1-1 map between a checkbox and a preference.
*  added some tests for title / tag autofill behavior

TODO (follow-up bugs):
    *  Seamonkey team to set suggest.history to true and the other suggest prefs to false.
    * Sync the new suggest prefs


I'll do a try push to make sure that nothing else is broken.
Attachment #8499375 - Attachment is obsolete: true
Attachment #8502924 - Flags: review?(mak77)
Comment on attachment 8502892 [details] [diff] [review]
Added new suggest.* prefs and removed unused mobile prefs for browser.urlbar ns

Review of attachment 8502892 [details] [diff] [review]:
-----------------------------------------------------------------

I think you should not even add the new prefs... since they will be unused.
To be clear, mobile doesn't use the awesombar at all.
Comment on attachment 8502892 [details] [diff] [review]
Added new suggest.* prefs and removed unused mobile prefs for browser.urlbar ns

Review of attachment 8502892 [details] [diff] [review]:
-----------------------------------------------------------------

Mak is right, we don't use places, so all these prefs in mobile.js are leftover from XUL fennec, and you can remove them.

It looks like the only browser.urlbar prefs we use are "browser.urlbar.autocomplete.enabled" and "browser.urlbar.trimURLs".

I filed bug 1081951 to clean up mobile.js, but in the meantime you don't need to worry about this :)
Attachment #8502892 - Flags: review?(margaret.leibovic)
Iteration: --- → 36.1
Comment on attachment 8502924 [details] [diff] [review]
Make Prefs UI for what to suggest in location bar friendly v4

Review of attachment 8502924 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/tests/unit/test_browserGlue_urlbar_defaultbehavior_migration.js
@@ +29,5 @@
> +  cleanup();
> +  // Migrate browser.urlbar.default.behavior preference
> +  Services.prefs.setIntPref("browser.migration.version", UI_VERSION - 1);
> +  Services.prefs.setIntPref(DEFAULT_BEHAVIOR_PREF, aDefaultBehavior);
> +};

should finally call gBrowserGlue.observe(null, TOPIC_BROWSERGLUE_TEST, TOPICDATA_BROWSERGLUE_TEST);

(it's called AndMigrate cause it's supposed to also migrate)

that same call can be removed from all of the following tests using it.

::: browser/components/preferences/privacy.xul
@@ +254,5 @@
> +                  preference="browser.urlbar.suggest.history"/>
> +        <checkbox id="bookmarkSuggestion" label="&locbar.bookmarks.label;"
> +                  preference="browser.urlbar.suggest.bookmark"/>
> +        <checkbox id="openpageSuggestion" label="&locbar.openpage.label;"
> +                  preference="browser.urlbar.suggest.openpage"/>

still missing accesskeys

There is another problem too, these prefs should somehow also control browser.urlbar.autocomplete.enabled. Ideally when that pref is set tp false, all of these should be unchecked, and viceversa. When any is checked it should be set to true.

So I fear we still need some support code for these. Or we need to remove autocomplete.enabled too, but then this patch goes too far (I think it might be worth to evaluate for a follow-up).

we also need a test covering that... having the test just for in-content prefs may be enough since those are the future target.

All of this is clearly valid for both in-content and dialog prefs.
Attachment #8502924 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #63)
> Comment on attachment 8502924 [details] [diff] [review]
> So I fear we still need some support code for these. Or we need to remove
> autocomplete.enabled too, but then this patch goes too far (I think it might
> be worth to evaluate for a follow-up).
> 
> we also need a test covering that... having the test just for in-content
> prefs may be enough since those are the future target.

Will do. So autocomplete.enabled pref should be true only when suggest.history == true || suggest.bookmark == true || suggest.openpage == true ?

It looks a bit redundant for me to have a pref only for that and it's not widely used (I can see it only in tests + nsPlacesAutoComplete + UnifiedComplete), but I'll keep it for now.
Comment on attachment 8502896 [details] [diff] [review]
Fix nsPlacesAutoComplete after adding urlbar suggest prefs v2

Review of attachment 8502896 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +889,5 @@
>    _getSearch: function PAC_getSearch(aTokens)
>    {
> +    let foundToken = false;
> +    let restrict = (behavior) =>
> +    {

brace on same line as => please
Attachment #8502896 - Flags: review?(mak77) → review+
(In reply to Alex Bardas :alexbardas from comment #64)
> Will do. So autocomplete.enabled pref should be true only when
> suggest.history == true || suggest.bookmark == true || suggest.openpage ==
> true ?

nope, if sugget.* is false, then autocomplete.enabled must be set to false, and if autocomplete.enabled is false, suggest.* must be set to false

> It looks a bit redundant for me to have a pref only for that and it's not
> widely used (I can see it only in tests + nsPlacesAutoComplete +
> UnifiedComplete), but I'll keep it for now.

I agree it's a little bit pointless now that we have the suggest.* prefs, but I prefer to not add too much changes to this single bug. Mobile uses that autocomplete.enabled pref, and I suspect other consumers might. So, I'd be fine removing it in a follow-up bug, also to have separate regression ranges.
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #66)
> (In reply to Alex Bardas :alexbardas from comment #64)
> > Will do. So autocomplete.enabled pref should be true only when
> > suggest.history == true || suggest.bookmark == true || suggest.openpage ==
> > true ?
> 
> nope, if sugget.* is false, then autocomplete.enabled must be set to false,
> and if autocomplete.enabled is false, suggest.* must be set to false

...and clearly, if any of the suggest.* prefs is true, autocomplete.enabled must be true.
Attachment #8502892 - Attachment is obsolete: true
Attachment #8502896 - Attachment is obsolete: true
Attachment #8505764 - Flags: review+
* added accesskeys
* added toggle support for autocomplete.enabled when suggest prefs are modified and vice versa + tests

I've tested locally and I don't see obvious / unexpected regressions. I'm happy to deal with them and add tests when they will be reported.
Attachment #8502924 - Attachment is obsolete: true
Attachment #8506323 - Flags: review?(mak77)
Comment on attachment 8506323 [details] [diff] [review]
Make Prefs UI for what to suggest in location bar friendly v5

Review of attachment 8506323 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry if this is taking ages, it wasn't easy and we are so close...

::: browser/components/nsBrowserGlue.js
@@ +225,4 @@
>    observe: function BG_observe(subject, topic, data) {
>      switch (topic) {
> +      case "nsPref:changed":
> +        if (data == PREF_AUTOCOMPLETE_ENABLED) {

heh, the problem here is that the autocomplete code is in toolkit, but you are handling the prefs in browserglue. It won't work for other consumers.
The code handling the pref should be added to UnifiedComplete and nsPlacesAutocomplete (that on the positive side, are already reading the values on startup and listening for changes)

I see why you put this here, cause prefs coule be out-of-sync if autocomplete has not started yet. Though the right fix would be to add some initialization code to the prefs privacy pane to ensure prefs are consistent on loading it.

@@ +237,5 @@
> +          if (pref) {
> +            // If the preference was activated, activate all suggest
> +            // preference only if all of them are false.
> +            let getPref = Services.prefs.getBoolPref;
> +            let suggestPrefsArray = [getPref(basePref + type) for (type of types)];

types.map(t => getPref(basePref + t))?

::: browser/components/places/tests/unit/test_browserGlue_prefs.js
@@ +63,5 @@
>    },
>  
> +  // Test that switching on / off autocomplete.enabled pref also activates / deactivates
> +  // the suggest prefs.
> +  function test_autocompleteEnabledPreference() {

that means also the test needs a new home

::: browser/components/preferences/in-content/privacy.js
@@ +350,2 @@
>     */
> +  writeSuggestionPref: function PPP_writeSuggestionPref() {

there's no more need to label functions like this

@@ +359,5 @@
> +
> +    if (suggestHistory || suggestBookmark || suggestOpenpage) {
> +      Services.prefs.setBoolPref("browser.urlbar.autocomplete.enabled", true);
> +    } else {
> +      Services.prefs.setBoolPref("browser.urlbar.autocomplete.enabled", false);

you might even do
let enabled = ["history", "bookmark", "openpage"].map(getVal).some(v => v);
Services.prefs.setBoolPref("browser.urlbar.autocomplete.enabled", enabled);

::: browser/components/preferences/privacy.js
@@ +325,5 @@
> +
> +    if (suggestHistory || suggestBookmark || suggestOpenpage) {
> +      Services.prefs.setBoolPref("browser.urlbar.autocomplete.enabled", true);
> +    } else {
> +      Services.prefs.setBoolPref("browser.urlbar.autocomplete.enabled", false);

ditto

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +25,4 @@
>  <!ENTITY  locbar.bookmarks.label        "Bookmarks">
> +<!ENTITY  locbar.bookmarks.accesskey    "d">
> +<!ENTITY  locbar.openpage.label         "Open tabs">
> +<!ENTITY  locbar.openpage.accesskey     "g">

I'm not sure "d" is good for "Bookmarks" and "g" for Open tabs... "m" and "t" look free, so far?

::: toolkit/components/places/UnifiedComplete.js
@@ +723,5 @@
> +    // "openpage" behavior is supported by the default query.
> +    // _switchToTabQuery instead returns only pages not supported by history.
> +    if (this.hasBehavior("openpage")) {
> +      queries.splice(1, 0, this._switchToTabQuery);
> +    }

nit: Tim in bug 1075450 suggested something like
let queries = [ this._adaptiveQuery ];
if (this.hasBehavior("openpage"))
  queries.push(this._switchToTabQuery);
queries.push(this._searchQuery);

that is probably less error prone than splice.
If you change this, please also do accordingly in the other patch
Attachment #8506323 - Flags: review?(mak77)
Blocks: 1075450
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #70)
> Comment on attachment 8506323 [details] [diff] [review]
> ::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
> @@ +25,4 @@
> >  <!ENTITY  locbar.bookmarks.label        "Bookmarks">
> > +<!ENTITY  locbar.bookmarks.accesskey    "d">
> > +<!ENTITY  locbar.openpage.label         "Open tabs">
> > +<!ENTITY  locbar.openpage.accesskey     "g">
> 
> I'm not sure "d" is good for "Bookmarks" and "g" for Open tabs... "m" and
> "t" look free, so far?
> 


I'll come today with a patch which addresses all suggestions. About the accesskeys, I had a brief chat with Blair and Gavin on irc. "h", "b", "o", "t", "p", "s" are already taken by other preferences and they suggested to assign some random letters which aren't taken yet.

Unfortunately, changing the other prefs' accesskeys seems to not be a solution.
this entity is unused
ENTITY trackingProtection.accesskey "m"
so we could remove it and use m for book_m_arks.

"t" is taken by "clearDataSettings" that is a button controlled by the "alwaysClear" checkbox... this button is usually disabled and the checkbox has an accesskey so its value is quite low. It may still be a bad idea to change it though.
Maybe we could change "open tabs" with "open pages" to make "g" more meaningful.
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #72)
> Maybe we could change "open tabs" with "open pages" to make "g" more
> meaningful.

Note that we don't use the term "pages" anywhere in the UI.
Yeah. Let's just change bookmarks to m (removing the currently unused "m" accesskey) for now.

The only free accesskeys here are "d", "g", "i", "l", "q", "v", "z", I can't think of alternatives to "Open Tabs" including one of these atm.
Tracking Protection was just added in August in https://hg.mozilla.org/mozilla-central/rev/f5bf94faa19c for bug 1031033 - so it shouldn't be unused for long?
thanks for pointing that out, someone should have added a localization note there.

So, let's proceed with what you already had in the patch and nvm.
Added observer for autocomplete.enabled pref.
Attachment #8505764 - Attachment is obsolete: true
Attachment #8508351 - Flags: review?(mak77)
Removed the autocomplete.enabled pref observer handler from nsBrowserGlue and added it to UnifiedComplete.

I need the Prefs to be loaded as soon as possible (the solution may be a bit hackish so I'm expecting suggestions regarding a better way of doing this, if it is).
Attachment #8506323 - Attachment is obsolete: true
Attachment #8508354 - Flags: review?(mak77)
(In reply to Alex Bardas :alexbardas from comment #79)
> https://tbpl.mozilla.org/?tree=Try&rev=623872de4ca9

I see that there is a regression. I'll have to investigate more, can't tell its cause right now. Marco, do you have any thoughts regarding it? (with default preferences, everything should work the same as before)
(In reply to Alex Bardas :alexbardas from comment #78)
> Removed the autocomplete.enabled pref observer handler from nsBrowserGlue
> and added it to UnifiedComplete.
> 
> I need the Prefs to be loaded as soon as possible (the solution may be a bit
> hackish so I'm expecting suggestions regarding a better way of doing this,
> if it is).

I don't understand. autocomplete components don't need the prefs until they are actually searching, so no change there should be needed at all.
What should be needed is init code in the privacy pane so that on load it ensures the prefs are consistent... What am I missing?
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #81)
> (In reply to Alex Bardas :alexbardas from comment #78)
> > Removed the autocomplete.enabled pref observer handler from nsBrowserGlue
> > and added it to UnifiedComplete.
> > 
> > I need the Prefs to be loaded as soon as possible (the solution may be a bit
> > hackish so I'm expecting suggestions regarding a better way of doing this,
> > if it is).
> 
> I don't understand. autocomplete components don't need the prefs until they
> are actually searching, so no change there should be needed at all.
> What should be needed is init code in the privacy pane so that on load it
> ensures the prefs are consistent... What am I missing?

I thought about the scenario in which autocomplete.enabled is toggled directly from about:config (nothing would happen if unifiedcomplete is not initialized yet).
(In reply to Alex Bardas :alexbardas from comment #82)
> I thought about the scenario in which autocomplete.enabled is toggled
> directly from about:config (nothing would happen if unifiedcomplete is not
> initialized yet).

well, if it is initialized, it is listening to changes already (through Preferences.jsm).  If it's not initialized, it will read the pref value when it will initialize... Isn't it?
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #83)
> (In reply to Alex Bardas :alexbardas from comment #82)
> well, if it is initialized, it is listening to changes already (through
> Preferences.jsm).  If it's not initialized, it will read the pref value when
> it will initialize... Isn't it?

I remember I tested these scenarios and it ended up being unsynced.

When autocomplete.enabled is set to false, and other options are true (in about:config), then it will not happen anything when initialized since the code in UI is listening for observer events (which will not be triggered anymore).

If the code would not listen for these events and just change the suggest prefs if autocomplete is enabled / disabled, it would be a bit unpredictable, because we probably want to trigger an update only on an autocomplete.enabled change, not based on autocomplete.enabled state.

Now it works as before, when the observer was in nsBrowserGlue.

Regarding the failing tests, I'm under the impression that there are only some small changes which need to be made in order to have them work again. I'll invest more time in debugging today, but if you having anything in mind regarding them, please let me know.
(In reply to Alex Bardas :alexbardas from comment #84)
> When autocomplete.enabled is set to false, and other options are true (in
> about:config), then it will not happen anything when initialized since the
> code in UI is listening for observer events (which will not be triggered
> anymore).

I think that's ok.

autocomplete.enabled set to false means autocomplete is disabled, it's fine that autocomplete doesn't work. I don't care if the suggest prefs are not coherent with that.

If the user then opens the prefs pane, the init code I was suggesting will set the suggest prefs to false and make them coherent. Then the user can just check one of the prefs and autocomplete.enabled will be set to true.

> If the code would not listen for these events and just change the suggest
> prefs if autocomplete is enabled / disabled, it would be a bit
> unpredictable, because we probably want to trigger an update only on an
> autocomplete.enabled change, not based on autocomplete.enabled state.

We don't care having the prefs coherent in about:config, that's an advanced UI that is only for expert users, they must know what they are doing. There's a warning for that.
We only care the preferences pane works as we expect.

So, please revert any change to the status where tests were passing, just make the preferences pane fixup prefs on init, and make it set autocomplete.enabled to 0 when all suggest checkboxes are unset.

Basically, I'd like to restart from v4 (provided tests were fine at that time?), make interdiffs on top of it with only the good changes from v6.

If we keep trying to expand the purpose of this to make it "perfect", it will never land. It's also very hard for me to help you with the failures if we don't start on a green version and build interdiffs on top of it. Let's try to proceed step by step please.
needinfo just to make sure we are on the same boat :)
Flags: needinfo?(abardas)
Comment on attachment 8508351 [details] [diff] [review]
Fix nsPlacesAutoComplete after adding urlbar suggest prefs v4

let's clarify bug status. 

I don't think initing the service on startup is something we ever want to do for various reasons, mostly startup performance.
Attachment #8508351 - Flags: review?(mak77)
Attachment #8508354 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #87)
> Comment on attachment 8508351 [details] [diff] [review]
> Fix nsPlacesAutoComplete after adding urlbar suggest prefs v4
> 
> let's clarify bug status. 
> 
> I don't think initing the service on startup is something we ever want to do
> for various reasons, mostly startup performance.

No worries with that, initializing the service (if uninitialized) in preferences#privacy would work just fine. My main problem was that a few tests are still failing. When I worked on it, I tested only /autocomplete and /unifiedcomplete folders and not everything from places .. unfortunately.

I've already fixed some of them with only one change in nsPlacesAutocomplete. Debugging those xpcshell tests was not that straightforward (for me at least).

So the next patch will also have all the fixes for xpcshell tests.
Flags: needinfo?(abardas)
If we do nothing for about:config changes then registering the observer earlier is useless. I think we don't need the privacy pane to initialize something from the UnifiedComplete component anymore.

It will be registered at the first search and the behavior will be consistent with what is shown in about:preferences#privacy.

The tests are now fixed (with the extra changes from this and the nsAutocompletePlaces patch: https://tbpl.mozilla.org/?tree=Try&rev=d1eea965371e) 

Other changes in the patches:
 - make sure tag ui is shown for history results which have tags
 - fix nsAutocompletePlaces autofill
 - fix nsNavHistory.cpp condition for searching in everything (history, bookmarks, openpages) (this fixed most of the failing tests)
Attachment #8508354 - Attachment is obsolete: true
Attachment #8511756 - Flags: review?(mak77)
Attachment #8508351 - Attachment is obsolete: true
Attachment #8511758 - Flags: review?(mak77)
Comment on attachment 8511756 [details] [diff] [review]
Make Prefs UI for what to suggest in location bar friendly v6.1

Review of attachment 8511756 [details] [diff] [review]:
-----------------------------------------------------------------

It would be an r+, if I could find where the magic .enabled => .suggest.* conversion happens in UnifiedComplete.js... I really can't see why the additional tests in test_autocomplete_functional.js would be passing?
I was expecting to see that magic code in the prefs observer but there's nothing there... maybe a qref problem?

::: browser/app/profile/firefox.js
@@ +1247,5 @@
>  pref("services.sync.prefs.sync.browser.startup.page", true);
>  pref("services.sync.prefs.sync.browser.tabs.loadInBackground", true);
>  pref("services.sync.prefs.sync.browser.tabs.warnOnClose", true);
>  pref("services.sync.prefs.sync.browser.tabs.warnOnOpen", true);
>  pref("services.sync.prefs.sync.browser.urlbar.autocomplete.enabled", true);

did you already file a bug to make Sync handle the new prefs? If so please make it depend on this.

::: browser/base/content/searchSuggestionUI.css
@@ +40,5 @@
>    overflow: hidden;
>    padding: 6px 8px;
>    text-overflow: ellipsis;
>    white-space: nowrap;
> +}
\ No newline at end of file

this change is unrelated to this bug and should be moved elsewhere

::: browser/components/nsBrowserGlue.js
@@ +1620,5 @@
> +    if (currentUIVersion < 25) {
> +      // Refactor urlbar suggestion preferences to make it extendable and
> +      // allow new suggestion types (e.g: search suggestions).
> +      let types = ["history", "bookmark", "openpage"];
> +      let defaultBehavior = 0;

I think if autocomplete.enabled is false we should already set all suggest prefs to false here, so we are better synced up.
adding a test for that should be easy too.

::: toolkit/components/places/UnifiedComplete.js
@@ +15,2 @@
>  
> +const PREF_UNIFIEDCOMPLETE = "unifiedcomplete";

what is this for? I can't find any usage of it (And if it would be false this component wouldn't be loaded at all)

@@ +423,4 @@
>      QueryInterface: XPCOMUtils.generateQI([ Ci.nsIObserver ])
>    };
>    loadPrefs();
>    prefs.observe("", store);

nit: I suspect here we might do

let store = {};
function loadPrefs() ...

loadPrefs();
prefs.observe("", loadPrefs);
...

by using the fact nsIObserver has the function annotation.

::: toolkit/components/places/nsNavHistory.cpp
@@ +3275,5 @@
>    bool hasSearchTerms;
> +  int32_t searchBehavior = mozIPlacesAutoComplete::BEHAVIOR_HISTORY |
> +                           mozIPlacesAutoComplete::BEHAVIOR_BOOKMARK |
> +                           mozIPlacesAutoComplete::BEHAVIOR_TYPED |
> +                           mozIPlacesAutoComplete::BEHAVIOR_OPENPAGE;

OPENPAGE should not be needed here
I'm not sure about TYPED, I think it should not be needed as well?

Here we want to match against the whole history and bookmarks dataset.

good catch btw.

::: toolkit/components/places/tests/unifiedcomplete/test_autoFill_default_behavior.js
@@ +171,5 @@
> +  // Don't autofill because it's a title.
> +  do_log_info("Restrict bookmarks, title, autofill.typed = false, should not autoFill");
> +  yield check_autocomplete({
> +    search: "ti",
> +    matches: [ { uri: uri5, title: "title", tags: ["foo"] } ],

I think these tests should be slightly different.
we autoFill only on host/uri, but if we have title or tag _behavior_ we should not even autofill on uri.

So you should still try to match on the uri (that here is "tagged") but that should fail if there is title behavior. so search: "ta" and restrict title.

the tests you did here instead are basically no-op cause autofill doesn't use title nor tags for comparison.

@@ +178,5 @@
> +  });
> +
> +  // Don't autofill because it's a tag.
> +  do_log_info("Restrict bookmarks, tag, autofill.typed = false, should not autoFill");
> +  yield check_autocomplete({

ditto

::: toolkit/components/places/tests/unifiedcomplete/test_autocomplete_functional.js
@@ +152,5 @@
> +  Services.prefs.setBoolPref(PREF_AUTOCOMPLETE_ENABLED, false);
> +  let types = ["history", "bookmark", "openpage"];
> +  for (type of types) {
> +    do_check_eq(Services.prefs.getBoolPref("browser.urlbar.suggest." + type), false,
> +                "suggest." + type + "pref should be false");

I'm sorry, but I cannot find where you are doing this conversion from .enabled to .suggest.*? In UnifiedComplete.js I can't find it, and privacy pane is not accessed here.
Attachment #8511756 - Flags: review?(mak77) → feedback+
Comment on attachment 8511758 [details] [diff] [review]
Fix nsPlacesAutoComplete after adding urlbar suggest prefs v4.1

Review of attachment 8511758 [details] [diff] [review]:
-----------------------------------------------------------------

basically the same question
Attachment #8511758 - Flags: review?(mak77)
Iteration: 36.1 → 36.2
Thank you very much for the reviews Marco. I feel like we're getting very close. I was a bit busy in the last few days, but I'm focusing now on this in order to have it ready asap. I've already fixed most things from the last review, but I would like an opinion about some final details.
 
Indeed, I ran the tests with a version that had those autocomplete.enabled observers in both modules. I don't remember what I had in mind when I removed them. I think I manually tested that using the preferences#privacy ui would work (because the suggest preferences are changed by the user and autocomplete.enabled would reflect that). I stopped considering the case when autocomplete.enabled would be manually set.

1. I think it's ok to re-add them to support that use case (or I can just remove that part from the test if you think it's not necessary anymore). 

In case I should proceed to initialize UnifiedComplete prefs from privacy.js, what would be the "recommended" way? Should I expose a new method in one of the interfaces UnifiedComplete implements? (like migrateSuggestPrefs()) Or what would you suggest to do here?


2. I would like a suggestion related to the recommended fixes from test_autoFill_default_behavior.js. I can restrict to title or tag by adding a restrict token. So I can call "ta #" or "ta +". But autofill is not performed at all because of those tokens. 

So for "# ta" I would get no matches, autofilled == completed == "# ta". Would that be enough?

3. Another thing would be related to restyling the tag results. I'll immediately upload an image showing how they look. I didn't do any work there, but both the tags and the bookmark star are displayed right now. Should I fill a follow-up bug to remove the star if the suggest.bookmark pref is false? (the tags will obviously appear all the time).

Finally, I would like to ask for a feedback not related to code, but to how I structured the patches. It's hard for me to follow everything (like that stupid .css change which came from another bug) and I guess it can also be a nightmare to you. Would have been much better to have multiple patches, one for preferences#privacy panels and tests, another one for nsbrowserglue, another one for UnifiedComplete and the last one for nsPlacesAutocomplete?
Flags: needinfo?(mak77)
(In reply to Alex Bardas :alexbardas from comment #93)
> Thank you very much for the reviews Marco. I feel like we're getting very
> close.

yeah, we are close!

> 1. I think it's ok to re-add them to support that use case (or I can just
> remove that part from the test if you think it's not necessary anymore). 
> 
> In case I should proceed to initialize UnifiedComplete prefs from
> privacy.js, what would be the "recommended" way? Should I expose a new
> method in one of the interfaces UnifiedComplete implements? (like
> migrateSuggestPrefs()) Or what would you suggest to do here?

I think, whatever we do, should stay as simple as possible.
I'd add an onload/init handler to the privacy pane, and in that handler initialize the autocomplete component (the right one depending on the unifiedComplete pref). You could just put a Prefs; in the autocomplete component constructor to make it initialize prefs on startup (with a comment explaining why we do that).
Then handle the magic in the Prefs handler in autocomplete (e.g. if enabled is false set suggest.prefs to false). You should do this regardless so that when autocomplete starts it will sync up the prefs to a coherent status.

what do you think of this?

> 2. I would like a suggestion related to the recommended fixes from
> test_autoFill_default_behavior.js. I can restrict to title or tag by adding
> a restrict token. So I can call "ta #" or "ta +". But autofill is not
> performed at all because of those tokens. 
> 
> So for "# ta" I would get no matches, autofilled == completed == "# ta".
> Would that be enough?

so, iirc we remove special tokens, so _searchTokens should become 1 there and we should only see "ta".
that means this passes if (!this._searchTokens.length == 1) check in shouldAutofill
I think it should be enough. the scope of this test is to arrive at this code:

// autoFill doesn't search titles or tags.
if (this.hasBehavior("title") || this.hasBehavior("tags"))	
return false;	

and hit the return false path.

> 3. Another thing would be related to restyling the tag results. I'll
> immediately upload an image showing how they look. I didn't do any work
> there, but both the tags and the bookmark star are displayed right now.
> Should I fill a follow-up bug to remove the star if the suggest.bookmark
> pref is false? (the tags will obviously appear all the time).

Yes, we should show tags, but not the star, we are decoupling the 2 concepts.

> Would have been much better to have multiple patches, one
> for preferences#privacy panels and tests, another one for nsbrowserglue,
> another one for UnifiedComplete and the last one for nsPlacesAutocomplete?

It's the same for me, just do what helps you moving on faster. I'm used to review large patches touching different components.
This may not be the same for any reviewer, usually the best thing is not to split by file, but to split by dependency (so "I need to do this to be able to do that", means I want a patch for this and one for that). Smaller dependent changes.
Flags: needinfo?(mak77)
I added the initializer in UnifiedComplete. I couldn't register the prefs observer using the function annotation because of how that observer is implemented (http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Preferences.jsm#415). It doesn't pass all the arguments.
Attachment #8511756 - Attachment is obsolete: true
Attachment #8515310 - Flags: review?(mak77)
Make sure that switching autocomplete.enabled updates the preferences.
Attachment #8511758 - Attachment is obsolete: true
Attachment #8515329 - Flags: review?(mak77)
Comment on attachment 8515310 [details] [diff] [review]
Make Prefs UI for what to suggest in location bar friendly v6.5

Review of attachment 8515310 [details] [diff] [review]:
-----------------------------------------------------------------

please, remember to file the bug about syncing prefs, and the bug about removing the star for non-bookmark behavior.

I think this is only missing one piece, that is initializing the (right) autocomplete component, in gPrivacyPane::init(), so that the Prefs; call you added in the autocomplete constructor is useful for something. Its only use is to ensure prefs are in sync, and the only case they may be out of order is:
1. play with the prefs in about:config before autocomplete has started
2. open the prefs pane before autocomplete has started
3. now the prefs will be out of sync.

If we init autocomplete when initing the privacy pane, it will ensure things are correct.
Something simple like:

// Initialize autocomplete to ensure prefs are in sync.
if (unifiedcomplete_pref_is_true) {
  Components.classes["@mozilla.org/autocomplete/search;1?name=unifiedcomplete"]
                    .getService(Components.interfaces.mozIPlacesAutoComplete);
} else {
 Components.classes["@mozilla.org/autocomplete/search;1?name=history"]
                   .getService(Components.interfaces.mozIPlacesAutoComplete);
}

Btw, I think once these comments are addressed this can land and we can move on with much smaller targeted patches.
Well done.

::: browser/components/nsBrowserGlue.js
@@ +1670,5 @@
> +      let types = ["history", "bookmark", "openpage"];
> +      let defaultBehavior = 0;
> +      let autocompleteEnabled = true;
> +      try {
> +        autocompleteEnabled = Services.prefs.getBoolPref("browser.urlbar.autocomplete.enabled");

let's define autocompleteEnabled only in the try scope, later for TYPED you can use defaultBehavior != -1

@@ +1676,5 @@
> +          defaultBehavior = -1;
> +        } else {
> +          defaultBehavior = Services.prefs.getIntPref("browser.urlbar.default.behavior");
> +        }
> +      } catch (ex) {}

there's something fishy here, if autocomplete.enabled would not exist, we'd not even try to read defaultBehavior.

This is likely not a real-life issue, but for code sanity I'd prefer smaller try scopes:

let defaultBehavior = 0;
try {
  defaultBehavior = getIntPref...
} catch () {}
try {
  let autocompleteEnabled = Services.prefs.get...
  if (!autocompleteEnabled)
    defaultBehavior = -1;
} catch () {}

@@ +1690,5 @@
> +        Services.prefs.setBoolPref("browser.urlbar.suggest." + type, prefValue);
> +      }
> +
> +      // Typed behavior will be used only for results from history.
> +      if (autocompleteEnabled &&

as I said, I think you can use defaultBehavior != -1 here.

::: browser/components/places/tests/unit/test_browserGlue_urlbar_defaultbehavior_migration.js
@@ +32,5 @@
> +  // Migrate browser.urlbar.default.behavior preference
> +  Services.prefs.setIntPref("browser.migration.version", UI_VERSION - 1);
> +  Services.prefs.setIntPref(DEFAULT_BEHAVIOR_PREF, aDefaultBehavior);
> +  Services.prefs.setBoolPref(AUTOCOMPLETE_PREF, aAutocompleteEnabled);
> +  gBrowserGlue.observe(null, TOPIC_BROWSERGLUE_TEST, TOPICDATA_BROWSERGLUE_TEST);

Add comment above this:
// Simulate a migration.

::: toolkit/components/places/UnifiedComplete.js
@@ +419,5 @@
> +
> +    // Synchronize suggest.* prefs with autocomplete.enabled at initialization
> +    // or every time autocomplete.enabled is changed.
> +    if (arguments.length == 0 ||
> +        (topic == TOPIC_PREFCHANGED && data == PREF_BRANCH + PREF_ENABLED[0])) {

on construction, we should do this BEFORE adding the pref observer, otherwise we'd be called again for every pref change.

I think you could move this special code into an helper function in the Prefs lazy loader. For example syncEnabledPref()

in the Prefs lazy loader function, do

syncEnabledPref();
just before the loadPrefs(); call

in loadPrefs do
if (data == PREF....)
  syncEnabledPref();

I also don't think there's any need to check the topic (it can only be TOPIC_PREFCHANGED)... indeed, please also remove const TOPIC_PREFCHANGED from the top of the file, it's useless.

@@ +423,5 @@
> +        (topic == TOPIC_PREFCHANGED && data == PREF_BRANCH + PREF_ENABLED[0])) {
> +      let suggestPrefs = [PREF_SUGGEST_HISTORY, PREF_SUGGEST_BOOKMARK, PREF_SUGGEST_OPENPAGE];
> +      if (store.enabled) {
> +        // If the autocomplete preference is active, activate all suggest
> +        // preferences only if all of them are false.

s/activate/set to default value/

@@ +1550,5 @@
>  
>  function UnifiedComplete() {
> +  // Make sure the preferences are initialized as soon as possible.
> +  // If the value of browser.urlbar.autocomplete.enabled is set to false,
> +  // then all the other suggest preferences for history, bookarmarks and

typo: bookarmarks
Attachment #8515310 - Flags: review?(mak77) → review+
Comment on attachment 8515329 [details] [diff] [review]
Fix nsPlacesAutoComplete after adding urlbar suggest prefs v4.5

Review of attachment 8515329 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +877,5 @@
>      if (aRegisterObserver) {
>        this._prefs.addObserver("", this, false);
>      }
> +    if (arguments.length == 0 ||
> +        (aTopic == kPrefChanged && aData == kBrowserUrlbarAutocompleteEnabledPref)) {

my comment here is the same as for unified complete
Attachment #8515329 - Flags: review?(mak77) → review+
Assignee: alex.bardas → nobody
Status: ASSIGNED → NEW
Iteration: 36.2 → ---
Assignee: nobody → alex.bardas
Status: NEW → ASSIGNED
Target Milestone: Firefox 3.7a1 → ---
I've also removed browser.urlbar.suggest.search (since it's related to another bug, it can be added there).

I'll file the other 2 bugs shortly.
Attachment #8515310 - Attachment is obsolete: true
Attachment #8517873 - Flags: review+
Attachment #8517873 - Flags: feedback?(mak77)
Attachment #8515329 - Attachment is obsolete: true
Attachment #8517876 - Flags: review+
Attachment #8517876 - Flags: feedback?(mak77)
Blocks: 1094567
Blocks: 1094580
Comment on attachment 8517873 [details] [diff] [review]
Make Prefs UI for what to suggest in location bar friendly v7

Review of attachment 8517873 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/UnifiedComplete.js
@@ +355,5 @@
> +    let suggestPrefs = [PREF_SUGGEST_HISTORY, PREF_SUGGEST_BOOKMARK, PREF_SUGGEST_OPENPAGE];
> +
> +    // If the property is not initialized yet, initialize it with the
> +    // preference value.
> +    let setValue = (property, pref) => {

to simplify this code, just add an init=false optional bool argument, pass true at the single call before loadPrefs, and do the prefs.get only if that boolean is true. Then you should not need this helper anymore.

@@ +356,5 @@
> +
> +    // If the property is not initialized yet, initialize it with the
> +    // preference value.
> +    let setValue = (property, pref) => {
> +      if (!store[property]) {

fwiw, this would be wrong (should be if (!(property in store))), but you don't need any of this complication.
Attachment #8517873 - Flags: feedback?(mak77) → feedback+
Comment on attachment 8517876 [details] [diff] [review]
Fix nsPlacesAutoComplete after adding urlbar suggest prefs v5

Review of attachment 8517876 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +816,5 @@
> +
> +    // If the property is not initialized yet, initialize it with
> +    // the preference value.
> +    let setValue = (property, pref, defaultPrefValue = true) => {
> +      if (!this[property]) {

I think you can do the same simplification here as well (and also here this[property] check would have been wrong)
Attachment #8517876 - Flags: feedback?(mak77) → feedback+
Attachment #8517873 - Attachment is obsolete: true
Attachment #8518456 - Flags: review+
Comment on attachment 8518456 [details] [diff] [review]
Make Prefs UI for what to suggest in location bar friendly v7.1

Review of attachment 8518456 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/privacy.js
@@ +51,5 @@
> +    } else {
> +      Components.classes["@mozilla.org/autocomplete/search;1?name=history"]
> +                .getService(Components.interfaces.mozIPlacesAutoComplete);
> +    }
> +  }

looks like it's missing a comma here
Fixed typo (added comma)
Attachment #8518456 - Attachment is obsolete: true
Attachment #8521321 - Flags: review+
yeah, I think we can proceed with the checkin.
Keywords: uiwantedcheckin-needed
The second patch doesn't apply to fx-team unfortunately:

atching file toolkit/components/places/tests/unifiedcomplete/test_autoFill_default_behavior.js
Hunk #2 succeeded at 81 with fuzz 2 (offset 0 lines).
Hunk #3 FAILED at 114
Hunk #4 FAILED at 161
Hunk #6 FAILED at 220
3 out of 6 hunks FAILED -- saving rejects to file toolkit/components/places/tests/unifiedcomplete/test_autoFill_default_behavior.js.rej
patching file toolkit/components/places/tests/unifiedcomplete/test_empty_search.js
Hunk #1 FAILED at 8
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/places/tests/unifiedcomplete/test_empty_search.js.rej
patching file toolkit/components/places/tests/unifiedcomplete/test_special_search.js
Hunk #2 FAILED at 47
Hunk #3 succeeded at 83 with fuzz 2 (offset 0 lines).
Hunk #4 FAILED at 254
Hunk #5 FAILED at 362
3 out of 5 hunks FAILED -- saving rejects to file toolkit/components/places/tests/unifiedcomplete/test_special_search.js.rej
Keywords: checkin-needed
Alex, can you please rebase the second patch?
Flags: needinfo?(alex.bardas)
(In reply to Tim Taubert [:ttaubert] from comment #112)
> Alex, can you please rebase the second patch?

Sure, I'm fixing this now.
Flags: needinfo?(alex.bardas)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/86e5c0b7f1cd
https://hg.mozilla.org/mozilla-central/rev/af22db534027
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Iteration: --- → 36.3
Blocks: 1098350
Depends on: 1098556
Depends on: 1099332
Depends on: 1100385
Performed Exploratory testing on this using Nightly 36.0a1 (2014-11-25) with & w/o e10s, covering the following platforms: Windows 7 64-bit, Ubuntu 12.04 LTS 32-bit and Mac OS X 10.9.5.

Potential issues/inconsistencies:
1. [Windows, Linux, Mac] Scrolling while the suggestions pane is opened, dismisses it.
  * steps: type in the first letter from a previously visited website → scroll up or down while hovering over the location bar
  * note: might be intended behavior or very old regression, as I managed to replicate it on a nightly from 2013-01-10
2. [Mac-only] SimpleServiceDiscovery.jsm:165:0 throws an error.
  * error: https://pastebin.mozilla.org/7557005 
  * note: could not isolate a testcase for this exception, but it seems to reproduce often

Alex, any thoughts on these?
Status: RESOLVED → VERIFIED
Flags: needinfo?(alex.bardas)
(In reply to Andrei Vaida, QA [:avaida] from comment #117)
> 1. [Windows, Linux, Mac] Scrolling while the suggestions pane is opened,
> dismisses it.
>   * steps: type in the first letter from a previously visited website →
> scroll up or down while hovering over the location bar
>   * note: might be intended behavior or very old regression, as I managed to
> replicate it on a nightly from 2013-01-10

This reproduces in any popup, formfill, searchbar and so on. It might be expected, I don't know, it's a platform decision. I couldn't find a bug filed for it.
Btw, it's totaly unrelated to the fix here.

> 2. [Mac-only] SimpleServiceDiscovery.jsm:165:0 throws an error.
>   * error: https://pastebin.mozilla.org/7557005 
>   * note: could not isolate a testcase for this exception, but it seems to
> reproduce often

bug 1102482.
It's also unrelated to this fix.
Flags: needinfo?(alex.bardas)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #118)
> Btw, it's totaly unrelated to the fix here.
I suspected they weren't specifically related to this fix, but posting them here was the quickest way to get a confirmation and wrap things up.  Thank you for your feedback.
Depends on: 1114022
In firefox 36, for some reason the opentabs pref is set to false without my having changed the setting
In 35 the tab results were shown just fine.
(In reply to Danial Horton from comment #120)
> In firefox 36, for some reason the opentabs pref is set to false without my
> having changed the setting
> In 35 the tab results were shown just fine.

if you set it back to true, does it stick? does everything work properly?

Unfortunately the combinations we had before were quite complex so there is the possibility we might not have a perfect conversion.
yeah it does, I found this bug by searching the new prefs.

Urlbar behavior was set to 1 to exclude bookmarks, so it's probably why opentabs were preffed false once I installed beta 36.

It had been bugging me for days that I had lost switch to tab and I'd been individually testing extensions to see what was up before I started to dig through the full list in about:config and spotted these new urlbar prefs.
sorry, it was restricting history,  looking at the pref combinations though, now im surprised I still had switch to tab working without it being preffed to 129.
there was no need to search about:config, the prefs are exposed in Options / Privacy
This change isn't mentioned in the beta change notes, I had no idea these options existed until after i'd found the prefs and found this bug.
ugh, let's see if we can do something for release notes.

I suspect we could relnote something about a clearer preferences UI for the awesomebar
Flags: needinfo?(sledru)
Keywords: relnote
Marco, could you fill the relnote template? I am not sure what should be the wording here:

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:


PS: we are no longer using the keyword relnote but the tracking flag instead.
relnote-firefox: --- → ?
Flags: needinfo?(sledru) → needinfo?(mak77)
Keywords: relnote
whoops, things change!
I'm not very good at wording for relnotes, so please take care of making it more proper.

Release Note Request (optional, but appreciated)
[Why is this notable]: We slightly changed the urlbar preferences behavior, users having a very customized behavior have been migrated to the new one, but we could not do a perfect match cause the old prefs were really complicate and confusing. So if some user notices missing results in the awesomebar, they could have to check the new prefs ui under Options / Privacy / Location Bar.
[Suggested wording]: Location bar privacy preferences have been migrated to a simpler system, controllable through a new user-friendly UI in Options / Privacy / Location Bar.
[Links (documentation, blog post, etc)]: Sorry, we are lazy devs.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] -- currently sick :( (delayed answers) from comment #128)

> [Suggested wording]: Location bar privacy preferences have been migrated to
> a simpler system, controllable through a new user-friendly UI in Options /
> Privacy / Location Bar.
I am sorry but I cannot find the option myself. Do you have some documentation on this? Thanks
Here's the difference in screenshots (specifically in the "Location bar" section):
Old (35): https://cloudup.com/cl2yYILercE
New (36): https://cloudup.com/cliaKvGWTO9
Blocks: 1146323
Updating the relnote flag as we didn't relnote this change in 36.
Why are the "suggest" prefs linked with the "enabled" pref both in the UI and in the back end? You're just making extra work for yourself, and making the code very confusing, as you're setting the "enabled" pref from a handler triggered by the code that reads preferences.
(In reply to neil@parkwaycc.co.uk from comment #132)
> Why are the "suggest" prefs linked with the "enabled" pref both in the UI
> and in the back end? You're just making extra work for yourself, and making
> the code very confusing, as you're setting the "enabled" pref from a handler
> triggered by the code that reads preferences.

Since we have overlapping functionality between these prefs and the enabled pref and the user can set both in about:config bypassing our code.
I agree the situation is not the best regarding code clarity, but ideally one day the .enabled pref will disappear with all of this code.
AFAIR, the code in the UI synchronizes prefs => enabled, while the code in the backend synchronizes enabled => prefs.
We could probably unify the paths in the backend, but this patch was taking ages and the current solution was not so terrible to deserve blocking it.
(In reply to Marco Bonardo from comment #133)
> I agree the situation is not the best regarding code clarity, but ideally
> one day the .enabled pref will disappear with all of this code.
I do hope so.

> AFAIR, the code in the UI synchronizes prefs => enabled, while the code in
> the backend synchronizes enabled => prefs.
> We could probably unify the paths in the backend, but this patch was taking
> ages and the current solution was not so terrible to deserve blocking it.
Well, it's not clear from the code that you turn on the enabled pref after turning on the synchronise pref, which means that you avoid the backend code turning the other synchronise prefs on too.
You need to log in before you can comment on or make changes to this bug.