460343 - Add privacy-section prefs to control awesomebar behaviour

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P2)

Description

[Johnathan Nightingale [:johnath]]

Alex mentioned this idea in discussion with mconnor and I but said that there wasn't a bug yet.

The mockup in the URL is self-explanatory, the lines to focus on are the location bar behaviour lines, the "Bookmarks and History" combo box and the "private" tag beneath it.

Nom'ng wanted - don't think it blocks, but it requires strings, so this would need to happen quickly.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: v1

two known issues with this patch:

- we have prefs for both bookmarks and tags, but the UI simplifies this to just bookmarks, but toggles both in the background. this means that if the user configures, for example, tags to be restricted but not bookmarks, reconfiguring via the UI will overwrite this choice.

- we allow user-configuration of the restrict character. reconfiguring via the UI with this patch will return the restrict characters back to the defaults. to do this such that it retains the user-configured characters, we need to keep mirror prefs of those values.

(the second issue is another problem with the tri-state approach of these prefs, per the discussion started here: http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/fef67b34f807bc68/0d5687e0d54643d6)

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 346624 [details] [diff] [review]
v1

gavin, do you have cycles for review here?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 346624 [details] [diff] [review]
v1

Talked to Dietrich on IRC about getting better backend prefs here to avoid having to mess with the restrict.* prefs, which should simplify this code a lot. We can take this for now to get the strings in and then rework it once the better prefs are added.

>diff --git a/browser/components/preferences/privacy.js b/browser/components/preferences/privacy.js

>+    this._initSuggestUI();

This means the UI won't properly update if some other code causes the pref to change while this dialog is up, but that's minor and should be fixed once we rework the backend prefs.

>+  onSuggestUIChange: function PPP_onSuggestUIChange() {

>+    this._urlBarPrefs.history.value = history ? defaults.getCharPref("history") : "";
>+    this._urlBarPrefs.bookmark.value = bookmark ? defaults.getCharPref("bookmark") : "";
>+    this._urlBarPrefs.tag.value = bookmark ? defaults.getCharPref("tag") : "";

You can use pref.reset() here rather than getting the default value yourself.

>diff --git a/browser/components/preferences/privacy.xul b/browser/components/preferences/privacy.xul

>+        <label id="locationBarSuggestionLabel"

>+               control="locationBarSuggestionLabel"

This should be locationBarSuggestion, not locationBarSuggestionLabel.

>+        <menulist id="locationBarSuggestion"

>+            <menuitem label="&history.label;"/>

Localizers might not like this string reuse... I suppose it's pretty unlikely that the strings will vary based on the context, but it wouldn't really hurt to use separate strings here.

r=me with those.

Comment 4

[Dietrich Ayala (:dietrich)]

screenshot: http://www.grabup.com/uploads/f3e8a316b815447a9bf4fe13f7b7fdc6.png

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 346624 [details] [diff] [review]
v1

Looks good, although hopefully we will get the full reorganization of bug 462041

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: v2

updated to use the pref in bug 463459.

Comment 7

[Ed Lee :Mardak]

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

>+function URLBarPrefs() {
>+}
>+URLBarPrefs.prototype = {
>+  get _default() { return document.getElementById("browser.urlbar.default.behavior").value || 0; },
>+  set _default(val) { document.getElementById("browser.urlbar.default.behavior").value = val; },
...
We can reduce all of that code to the above and the following: :)

function URLBarPrefs() {
  const kBits = ["history", "bookmark", "tag", "title", "url"];
  for (let i = kBits.length; --i >= 0; ) {
    let bit = kBits[i];
    let mask = 1 << i;

    this.__defineGetter__(bit, function() this._default & mask);
    this.__defineSetter__(bit, function(aVal) this._default = aVal ?
      this._default | mask : this._default & ~mask);
  }
}

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: v3

- w/ mardak's comment
- using sync attrs

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: v3.1

fix bookmark filter

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 346853 [details] [diff] [review]
v3.1

>diff --git a/browser/components/preferences/privacy.js b/browser/components/preferences/privacy.js

>+  writeSuggestionPref: function PPP_writeSuggestionPref()

>+    let index = document.getElementById("locationBarSuggestion").selectedIndex;

I'd use value instead of selectedIndex (faster to get, too).

>+function URLBarPrefs(aDefault) {

Having a helper like this is probably a little overkill given that it isn't used that much (it would be simpler to just adjust the bitmask manually I think), but doesn't really matter.

Comment 11

[Simon Bünzli]

Comment on attachment 346853 [details] [diff] [review]
v3.1

>+      // reset maxRichResults if needed
>+      try {
>+        resultsEl.reset();

Won't this reset the pref whenever I switch from bookmarks to history or vice versa? Why not reset it only if it's actually 0? Else you risk annoying people who prefer the Firefox 2.0 behavior of actually displaying (almost) all available hits by setting maxRichResults to a fairly large number.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: v4

using the simpler pref from bug 463661.

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 347024 [details] [diff] [review]
v4

requesting approval for late strings and UI for beta 2.

Comment 14

[Dietrich Ayala (:dietrich)]

Axel says no chance to get these strings in for beta 2, but to nominate for late-l10n for RC1.

This fix addresses some of the top complaints about the smart location bar in the Firefox 3 release, as well as complementing the privacy-oriented feature set of the 3.1 release, making it worth taking these strings late.

Comment 15

[Axel Hecht [:Pike]]

l10n-impact comment:

Glued together phrases as in this patch are tricky for l10n. Having some bake time to get feedback on how this works in various locales would have been good.

This patch does, depending on how long the strings in question are in particular locales, come with a risk of having to resize the preferences dialog, which requires involved testing/patching cycles on all three platforms.

Comment 16

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

Comment on attachment 347024 [details] [diff] [review]
v4

As per l10n feedback, not gonna make b2; perhaps next milestone, I'll do a swing through the minused b2 approvals at that time.

Comment 18

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

Comment on attachment 347024 [details] [diff] [review]
v4

a191=beltzner, let's get this in. Axel, please let us know if it causes a lot of consternation, l10n-wise, due to the stapled strings.

If the reaction is that it would be a lot easier to do this without stapled strings, we can revisit the UI design.

Comment 19

[Axel Hecht [:Pike]]

I think it'd be safer to allow for trailing text, i.e., have 

      <hbox align="center">
        <label id="locationBarSuggestionLabel"
               control="locationBarSuggestion"
               accesskey="&locbar.pre.accessKey;">&locbar.pre.label;</label>
        <menulist id="locationBarSuggestion"
                  preference="browser.urlbar.search.sources">
          <menupopup>
            <menuitem label="&locbar.both.label;" value="3"/>
            <menuitem label="&locbar.history.label;" value="1"/>
            <menuitem label="&locbar.bookmarks.label;" value="2"/>
            <menuitem label="&locbar.nothing.label;" value="0"/>
          </menupopup>
        </menulist>
        <label>&locbar.post.label;</label>
      </hbox>

with locbar.post.label being empty in en-US, and probably most languages.

That way, languages can, if it makes sense, have text after the dropdown. Reminds me of larry, where we don't have that, and Persian needs it.

Comment 20

[(no longer active)]

(In reply to comment #19)
> That way, languages can, if it makes sense, have text after the dropdown.
> Reminds me of larry, where we don't have that, and Persian needs it.

Yes, in fact I can confirm that Persian *will* need that trailing string here as well.  We need a new patch here.  Dietrich, if you don't have the time, I can make the required changes to your patch.

Comment 21

[Ed Lee :Mardak]

Attachment: v4.1

Dietrich, I've updated the patch and it's ready to land by anyone. Should I push it to trunk?

Comment 22

[[:Cww]]

Adding user-doc-needed so we can update helpdoc screenshots and text.

Comment 23

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/eb18fff240df

We have a query somewhere to check for pushing to 1.9.1 yeah?

Comment 24

[David Tenser [:djst]]

Does "Nothing" here actually mean previously typed URLs? "Nothing" seems misleading here, but I'm not sure what the best wording is given the current options. Another alternative would be three checkboxes instead of radio buttons:

When using the location bar, suggest:

[x] Bookmarks
[x] History 
[x] Previously typed addresses

Comment 25

[u88484]

Nothing means nothing.  At least for me on Vista.  With it set as nothing, there is no location bar drop-down at all...nothing is suggested.

Comment 26

[David Tenser [:djst]]

So there is no way to revert to the Firefox 2 behavior?

Comment 27

[Ed Lee :Mardak]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6c623d0b6997

(In reply to comment #26)
> So there is no way to revert to the Firefox 2 behavior?
You can make the location bar match only in the url (default.behavior) and match only at the beginning of the url (matchBehavior)

Comment 28

[Chris Ilias [:cilias]]

user-doc-complete
http://support.mozilla.com/kb/Options+window?bl=n#privacy_options

Comment 29

[Tyler Downer [He/Him]]

v FIXED in Trunk and 1.9.1.

Comment 30

[u88484]

Why did target get changed to 3.6a1 when this has been checked into the trunk and the 1.9.1 branch for 3.5?

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Target milestones reflect the first trunk milestone that will contain the fix.

Comment 32

[Alex Faaborg [:faaborg] (Firefox UX)]

filed follow up bug 485122 about seeing bookmarks after selecting history only.