612059 - URL autocomplete cannot be disabled

Status: VERIFIED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[philippe (part-time)]

Setting the hidden pref 'browser.urlbar.autocomplete.enabled' to 'false' has no effect (even after a browser restart).


[12:17pm] sauron|FUS: yeah, it looks like between 1.9.1 and 1.9.2, checking of that pref moved from places to browser.js
...
[12:29pm] sauron|FUS: it may have broken when we removed xpautocomplete; not clear from how the code works
[12:29pm] sauron|FUS: but at any rate, nothing's checking for the pref now

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This actually broke when we stopped using xpautocomplete and started using Dan's Cocoa autocomplete.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Potential fix

This fixes it (code is mirrored on cl's fix for bug 384689).  There's also some "these are really sentences" comment cleanup near where I was adding mShouldAutocomplete.

Stuart, is this the best place to short-circuit autocompletion (on the fly)?  That looked to me like the entry point to the whole autocomplete system, but I admit to lack of familiarity with that code (or I'd've done more to fix the 387 outstanding bugs that drive me nuts about it :P ).

Also, are there other places where it would be beneficial to check mShouldAutocomplete to save some work?

Comment 3

[Stuart Morgan]

Comment on attachment 490442 [details] [diff] [review]
Potential fix

>+  mShouldAutocomplete = [[PreferenceManager sharedInstance] getBooleanPref:kGeckoPrefLocationBarAutocompleteEnabled withSuccess:NULL];

This should use the more complicated construction that actually passes in a success variable and sets the variable to YES if looking up the pref fails, since we want the default to be YES. You'll probably want a helper method since you'll need that code twice.

>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(LocationBarAutocompleteEnabledPrefChanged:)
>+                                               name:kPrefChangedNotificationName
>+                                             object:self]; // since we added ourself as the Gecko pref observer

This doesn't do what you want, because there's nothing pref-specific about this listener. If you have registered for more than one Gecko pref you have to figure out which one changed in the callback method.

>+const char* const kGeckoPrefLocationBarAutocompleteEnabled = "browser.urlbar.autocomplete.enabled";

Do we already list this in the prefs.js file?


As for where else to short-circuit, it looks like for now this is all; I'll need to add more logic in the big restructuring patch though.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Potential fix, v1.1

I think I've addressed everything _correctly_.

(In reply to comment #3)
> If you have registered for more than one Gecko pref you have to
> figure out which one changed in the callback method.

Consensus on irc was that this means just the one callback, and it checks both prefs for their values.  If that's not what you meant, several of us would like to know how we figure out which pref changed. 

> >+const char* const kGeckoPrefLocationBarAutocompleteEnabled = "browser.urlbar.autocomplete.enabled";
> 
> Do we already list this in the prefs.js file?

Yes (and it's still in all.js even though only browser.js checks it, just in case.  Because all.js is the best place to put prefs that Firefox alone checks :P )

Comment 5

[Stuart Morgan]

(In reply to comment #4)
> Consensus on irc was that this means just the one callback, and it checks both
> prefs for their values.  If that's not what you meant, several of us would like
> to know how we figure out which pref changed. 

It's in the userInfo dict under kPrefChangedPrefNameUserInfoKey. See SafeBrowsingListManager's safeBrowsingPrefChanged: for an example.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Potential fix, v1.2

Now with more what-pref-is-this-checking.

Comment 7

[Chris Lawson (gone)]

Two quick comments:

1) I don't see any compelling reason to use strcmp() here (or in the similar code that murph used in Safe Browsing, though I didn't give that more than a cursory glance); native Cocoa methods are certainly less obtuse. I might be missing something obvious, though.

2) The first comment that's changed would be clearer (since you're changing it anyway) if it read "Remembers..." instead of "Used to remember", so people don't wonder "if it *used to* do something, what does it do *now*, and why is it still around?"

cl

Comment 8

[Stuart Morgan]

Comment on attachment 490663 [details] [diff] [review]
Potential fix, v1.2

>   [[PreferenceManager sharedInstanceDontCreate] removeObserver:self forPref:kGeckoPrefInlineLocationBarAutocomplete];
>+  [[PreferenceManager sharedInstanceDontCreate] removeObserver:self forPref:kGeckoPrefLocationBarAutocompleteEnabled];

Replace these two lines with
   [[PreferenceManager sharedInstanceDontCreate] removeObserver:self];

>+  // If we can't get the pref, ensure we leave autocomplete enabled.
>+  if (!gotPref)
>+    geckoAutocompletePrefState = YES;
>+    
>+  return geckoAutocompletePrefState;

This could all just be:
    return (gotPref ? geckoAutocompletePrefState : YES);
but if this is fine too if you prefer it.

sr=smorgan

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/3d8ae29ec375

(In reply to comment #8)
> >+  // If we can't get the pref, ensure we leave autocomplete enabled.
> >+  if (!gotPref)
> >+    geckoAutocompletePrefState = YES;
> >+    
> >+  return geckoAutocompletePrefState;
> 
> This could all just be:
>     return (gotPref ? geckoAutocompletePrefState : YES);
> but if this is fine too if you prefer it.

The way I had it is clearer to me, so I left it ;-)

(In reply to comment #7)
> 2) The first comment that's changed would be clearer (since you're changing it
> anyway) if it read "Remembers..." instead of "Used to remember", so people
> don't wonder "if it *used to* do something, what does it do *now*, and why is
> it still around?"

I fixed this, too (they also have a better parallel structure that way).

Comment 10

[philippe (part-time)]

verified fixed with:
Version 2.1a1pre (1.9.2.14pre 20101201001436)