863747 - Add option to disable Location bar history

Status: RESOLVED FIXED

(SeaMonkey :: Location Bar, defect)

Description

[Ray Z.]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20100101 Firefox/20.0 SeaMonkey/2.17.1
Build ID: 20130410205209

Steps to reproduce:

We have the options to disable "Browsing history" and "Form and search history". It would be nice to have an option to disable "Location bar history".

Comment 1

[Stefan [:stefanh]]

if you mean the autocomplete feature (that shows suggestions from your browsing history), you can turn that off in "Preferences --> Browser --> Location Bar".

Comment 2

[Ray Z.]

I did mean the autocomplete. If you set your Seamonkey to disable "Browsing history" and "Form and search history" via "Preferences > History", it will still capture the entries that you typed in via the location bar. Also can be found on the list when click on the dropdown arrow on the far right hand of location bar. As I said on the original comment - It would be nice to have an option to disable that "Location bar history".

Comment 3

[Ray Z.]

Attachment: Location bar typed in history screenshot

Comment 4

[Ray Z.]

typo correction for "I did mean the autocomplete", it supposed to be "I did NOT mean the autocomplete", sorry about that. But it is related to autocomplete too. If you deselect the checkbox "Autocomplete from your browsing history as you type" in the Preferences panel, that typed in list (attached) is still shown if you click on the far right arrow on location bar. So right now, typed in entries are captured regardlessly.

Comment 5

[Philip Chee]

Looks like we need to fix addToUrlbarHistory() and createUBHistoryMenu()

Comment 6

[Edmund Wong (:ewong)]

Attachment: Proposed patch. (v1)

First attempt at a non-suggested preference bug in a long time.

Comment 7

[Ian Neal]

Comment on attachment 781571 [details] [diff] [review]
Proposed patch.  (v1)

>+++ b/suite/browser/browser-prefs.js
>@@ -30,16 +30,17 @@ pref("general.startup.browser",         
> pref("general.startup.mail",                false);
> pref("general.startup.news",                false);
> pref("general.startup.editor",              false);
> pref("general.startup.compose",             false);
> pref("general.startup.addressbook",         false);
> 
> pref("general.open_location.last_url",      "");
> pref("general.open_location.last_window_choice", 0);
>+pref("general.open_location.enabled",       true);
Is there a better pref name than this?

>+++ b/suite/common/contentAreaClick.js

>+    var locBarPrefEnabled = Services.prefs.getBoolPref("general.open_location.enabled");

>+    if (locBarPrefEnabled && !locBarPrefEnabled.locked) {
Why should it matter if the pref is locked or not?
Why all the changes below, can you not just do an early return when the location bar history is disabled?


>+++ b/suite/common/pref/pref-history.js

> function Startup()
> { 
>   var urlbarHistButton = document.getElementById("ClearUrlBarHistoryButton");
>   var lastUrlPref = document.getElementById("general.open_location.last_url");
>+  var locBarPref = document.getElementById("locationBarHistoryEnabled");
As it is only used once, why not inline it?

>+function prefLocationBarToggle()
>+{
>+  var urlbarHistButton = document.getElementById("ClearUrlBarHistoryButton");
>+  var lastUrlPref = document.getElementById("general.open_location.last_url");
>+  var locBarPref = document.getElementById("locationBarHistoryEnabled");
>+
>+  urlbarHistButton.disabled = !locBarPref.checked && lastUrlPref.locked;
You could have passed this.checked through as an argument, again not sure we need the one use variables.

r- for the moment

Comment 8

[Edmund Wong (:ewong)]

Attachment: Proposed patch. (v2)

Comment 9

[Ian Neal]

Comment on attachment 786822 [details] [diff] [review]
Proposed patch.  (v2)

>+++ b/suite/common/contentAreaClick.js
>@@ -195,16 +195,21 @@
>     event.stopPropagation();
>   }
> 
>   function addToUrlbarHistory(aUrlToAdd)
>   {
>     if (gPrivate)
>       return;
> 
>+    var locBarPrefEnabled = Services.prefs.getBoolPref("general.urlbarhistory.enabled");
>+
>+    if (!locBarPrefEnabled)
>+      return;
As you only use this the once, you can inline rather than declaring as a var.

>+++ b/suite/common/pref/pref-history.js

> function Startup()
> { 
>   var urlbarHistButton = document.getElementById("ClearUrlBarHistoryButton");
>   var lastUrlPref = document.getElementById("general.open_location.last_url");
>+  var locBarPref = document.getElementById("urlbarHistoryEnabled");
Potentially this could be inlined too, at minimum move the declaration to just before it is used.
>+
>   try {
>     var isBtnDisabled = lastUrlPref.locked;
>     if (!isBtnDisabled && !lastUrlPref.hasUserValue) {
>       var file = GetUrlbarHistoryFile();
>       if (!file.exists())
>         isBtnDisabled = true;
>       else {
>         var connection = Services.storage.openDatabase(file);
>         isBtnDisabled = !connection.tableExists("urlbarhistory");
>         connection.close();
>       }
>     }
>-    urlbarHistButton.disabled = isBtnDisabled;
>+    urlbarHistButton.disabled = isBtnDisabled && !locBarPref.checked;


>+++ b/suite/common/pref/pref-history.xul

>+      <preference id="general.open_location.enabled"
>+                  name="general.open_location.enabled"
>+                  type="bool"/>
You didn't change the preference id/name here to match the new name!

>+        <vbox pack="end">
>+        <checkbox id="urlbarHistoryEnabled"
>+                  label="&urlBarHistoryEnabled.caption;"
>+                  accesskey="&urlBarHistoryEnabled.accesskey;"
>+                  preference="general.urlbarhistory.enabled"
Shouldn't this be general.urlbarHistory.enabled? i.e. uppercase H

Also please make sure you log a bug for making the help changes if you are not going to do that as part of this bug.

Comment 10

[Edmund Wong (:ewong)]

Attachment: Proposed patch. (v3)

Comment 11

[Edmund Wong (:ewong)]

Attachment: Help patch (v1)

Comment 12

[Edmund Wong (:ewong)]

Attachment: Help patch (v2)

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 797753 [details] [diff] [review]
Help patch (v2)

[Splinter seems to hate me; appears blank for me. Well then, let's do it the old school way.]

>+      <li><strong>Enable Location Bar History</strong>: Check this to enable

Please make sure you have the label within the strong tag exactly like the string in the DTD file. Currently there's a difference in the case of "History". r=me with that.

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 797747 [details] [diff] [review]
Proposed patch.  (v3)

Some drive-bys:

1. The following need to match exactly (i.e. also in case):

>+pref("general.urlbarHistory.enabled",       true);
(...)
>+    if (!Services.prefs.getBoolPref("general.urlbarhistory.enabled"))
(...)
>+      <preference id="general.urlbarHistory.enabled"
>+                  name="general.urlbarHistory.enabled"
>+                  type="bool"/>
(...)
>+        <checkbox id="urlbarHistoryEnabled"
>+                  label="&urlBarHistoryEnabled.caption;"
>+                  accesskey="&urlBarHistoryEnabled.accesskey;"
>+                  preference="general.urlbarHistory.enabled"
>+                  oncommand="prefUrlBarHistoryToggle(this.checked);"/>

2.

>-    urlbarHistButton.disabled = isBtnDisabled;
>+    let locBarPref = document.getElementById("urlbarHistoryEnabled");
>+
>+    urlbarHistButton.disabled = isBtnDisabled && !locBarPref.checked;

AFAICS this is wrong. Shouldn't it be something like:

urlbarHistButton.disabled = isBtnDisabled || !locBarPref.checked;

so that either condition disables the button?

[And for consistency, maybe update isBtnDisabled with the locBarPref.checked check first, then leave the assignment as-is.]

Comment 15

[Ian Neal]

Comment on attachment 797747 [details] [diff] [review]
Proposed patch.  (v3)

r=me with line changed to:
urlbarHistButton.disabled = isBtnDisabled || !locBarPref.checked

Comment 16

[Philip Chee]

> >+                  preference="general.urlbarHistory.enabled"

Sorry for the late comment but shouldn't the pref be browser.urlbar.<something> to fit with all the other urlbar prefs which start with "browser.urlbar."

Suggestion "browser.urlbar.historyenabled"

Comment 17

[Edmund Wong (:ewong)]

Attachment: Help patch (v3)

Comment 18

[Edmund Wong (:ewong)]

(In reply to Philip Chee from comment #16)
> > >+                  preference="general.urlbarHistory.enabled"
> 
> Sorry for the late comment but shouldn't the pref be
> browser.urlbar.<something> to fit with all the other urlbar prefs which
> start with "browser.urlbar."
> 
> Suggestion "browser.urlbar.historyenabled"

IanN, should I change the pref?

Comment 19

[Ian Neal]

(In reply to Edmund Wong (:ewong) from comment #18)
> (In reply to Philip Chee from comment #16)
> > > >+                  preference="general.urlbarHistory.enabled"
> > 
> > Sorry for the late comment but shouldn't the pref be
> > browser.urlbar.<something> to fit with all the other urlbar prefs which
> > start with "browser.urlbar."
> > 
> > Suggestion "browser.urlbar.historyenabled"
> 
> IanN, should I change the pref?

Yes, perhaps browser.urlbar.historyEnabled

Comment 20

[Edmund Wong (:ewong)]

Attachment: Proposed patch. (v4)

Comment 21

[Ian Neal]

Comment on attachment 823219 [details] [diff] [review]
Proposed patch.  (v4)

>+++ b/suite/common/pref/pref-history.js
> function Startup()
> { 
>   var urlbarHistButton = document.getElementById("ClearUrlBarHistoryButton");
>   var lastUrlPref = document.getElementById("general.open_location.last_url");
>+
>   try {
>     var isBtnDisabled = lastUrlPref.locked;
>     if (!isBtnDisabled && !lastUrlPref.hasUserValue) {
>       var file = GetUrlbarHistoryFile();
>       if (!file.exists())
>         isBtnDisabled = true;
>       else {
>         var connection = Services.storage.openDatabase(file);
>         isBtnDisabled = !connection.tableExists("urlbarhistory");
>         connection.close();
>       }
>     }
>-    urlbarHistButton.disabled = isBtnDisabled;
>+    let locBarPref = document.getElementById("urlbarHistoryEnabled");
>+
>+    urlbarHistButton.disabled = isBtnDisabled || !locBarPref.checked;
I agree with Jens, it is probably worth moving the !locBarPref.checked part to the start, so maybe something like:
var isBtnDisabled = !locBarPref.checked || lastUrlPref.locked;
Might even be able to inline locBarPref
Does that part even have to be within the try?
r=me with that addressed/fixed/tested

Comment 22

[Edmund Wong (:ewong)]

Attachment: proposed patch (v5)

Comment 23

[Edmund Wong (:ewong)]

Attachment: proposed patch (v6)

Comment 24

[Edmund Wong (:ewong)]

Comment on attachment 8358780 [details] [diff] [review]
proposed patch (v6)

There's actually something strange about my patch which I need some clarification.

Let's assume we start anew.  

1) In Edit->Preferences->History, the "Enable Location Bar history" is checked, and the "Clear Location Bar" is greyed out.

2) In the location bar in the browser, go to www.google.com, and then
   www.yahoo.com  (or whichever site you want).

3) Edit->Preferences->History

   - The clear Location Bar button is greyed out.  (Shouldn't it be enabled?)

To enable it I need to:

   1) Uncheck "Enable Location Bar history"
   2) Check "Enable Location Bar history"
     
    then the "Clear Location Bar" Button is enabled.

Debugging it, I get to:

> var isBtnDisabled = lastUrlPref.locked || !locBarPref.checked;

isBtnDisabled is 'true'.  Since locBarPref.checked is true (therefore making
!locBarPref.checked == false), then lastUrlPref.locked must be true.

Why is lastUrlPref.locked is true?  I don't understand this. Uncheck and then
check the "Enable Location Bar history" checkbox makes the lastUrlPref.locked
false.

Is this supposed to happen?

Comment 25

[Edmund Wong (:ewong)]

Comment on attachment 8358780 [details] [diff] [review]
proposed patch (v6)

I just noticed this part:
            <button id="ClearUrlBarHistoryButton"
                    label="&clearLocationBarButton.label;"
                    accesskey="&clearLocationBarButton.accesskey;"
                    oncommand="prefClearUrlbarHistory(); this.disabled = true;"

"this.disabled = true;"

Is this what's disabling it?

Comment 26

[Ian Neal]

Comment on attachment 8358780 [details] [diff] [review]
proposed patch (v6)

>+++ b/suite/common/pref/pref-history.js
> function Startup()
> { 
>   var urlbarHistButton = document.getElementById("ClearUrlBarHistoryButton");
>   var lastUrlPref = document.getElementById("general.open_location.last_url");
>+  var locBarPref = document.getElementById("urlbarHistoryEnabled");
This should be "browser.urlbar.historyEnabled" as the checkbox won't be initialised yet.
>+
>+  var isBtnDisabled = lastUrlPref.locked || !locBarPref.checked;
So this needs to be !locBarPref.value rather than !locBarPref.checked

> function prefClearUrlbarHistory()
If you pass this as an argument from the xul then you can move the disabling into this function.
> {
>   document.getElementById("general.open_location.last_url").valueFromPreferences = "";
>   var file = GetUrlbarHistoryFile();
>   if (file.exists())
>     file.remove(false);
Something like aButton.disabled = true; here.
> }

>+++ b/suite/common/pref/pref-history.xul

>+            <button id="ClearUrlBarHistoryButton"
>+                    label="&clearLocationBarButton.label;"
>+                    accesskey="&clearLocationBarButton.accesskey;"
>+                    oncommand="prefClearUrlbarHistory(); this.disabled = true;"
This would become:
oncommand="prefClearUrlbarHistory(this);"
>+                    preference="pref.browser.history.disable_button.clear_urlbar"/>

r=me with the locBarPref stuff fixed, the change to prefClearUrlbarHistory function is optional.

Comment 27

[Edmund Wong (:ewong)]

Attachment: proposed patch (v7)

in the prefUrlBarHistoryToggle(), I added a check to see if the history file exists.  If it does, then it sets the toggle, otherwise it doesn't bother
with the toggle since if the history file doesn't exist, then toggling it
makes no sense.

So asking for a review.

Comment 28

[Ian Neal]

Comment on attachment 8450044 [details] [diff] [review]
proposed patch (v7)

>+++ b/suite/common/pref/pref-history.xul

>+            <button id="ClearUrlBarHistoryButton"
>+                    label="&clearLocationBarButton.label;"
>+                    accesskey="&clearLocationBarButton.accesskey;"
>+                    oncommand="prefClearUrlbarHistory(this); this.disabled = true;"
You didn't remove the "this.disabled = true;" part

r=me with that fixed

Comment 29

[Edmund Wong (:ewong)]

Attachment: proposed patch (v8)

removed the "this.disabled = true;" part.

Comment 30

[Edmund Wong (:ewong)]

Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/6c1179e027f0

(help)
https://hg.mozilla.org/comm-central/rev/00ea6ee30a53