Closed Bug 863747 Opened 11 years ago Closed 10 years ago

Add option to disable Location bar history

Categories

(SeaMonkey :: Location Bar, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.30

People

(Reporter: solidcolour, Assigned: ewong)

Details

Attachments

(3 files, 9 obsolete files)

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".
if you mean the autocomplete feature (that shows suggestions from your browsing history), you can turn that off in "Preferences --> Browser --> Location Bar".
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".
Component: General → Location Bar
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.
Looks like we need to fix addToUrlbarHistory() and createUBHistoryMenu()
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Proposed patch. (v1) (obsolete) — Splinter Review
First attempt at a non-suggested preference bug in a long time.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #781571 - Flags: review?(iann_bugzilla)
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
Attachment #781571 - Flags: review?(iann_bugzilla) → review-
Attached patch Proposed patch. (v2) (obsolete) — Splinter Review
Attachment #781571 - Attachment is obsolete: true
Attachment #786822 - Flags: review?(iann_bugzilla)
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.
Attachment #786822 - Flags: review?(iann_bugzilla) → review-
OS: Mac OS X → All
Hardware: x86 → All
Version: SeaMonkey 2.17 Branch → Trunk
Attached patch Proposed patch. (v3) (obsolete) — Splinter Review
Attachment #786822 - Attachment is obsolete: true
Attachment #797747 - Flags: review?(iann_bugzilla)
Attached patch Help patch (v1) (obsolete) — Splinter Review
Attachment #797748 - Flags: review?(jh)
Attached patch Help patch (v2) (obsolete) — Splinter Review
Attachment #797748 - Attachment is obsolete: true
Attachment #797748 - Flags: review?(jh)
Attachment #797753 - Flags: review?(jh)
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.
Attachment #797753 - Flags: review?(jh) → review+
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 on attachment 797747 [details] [diff] [review]
Proposed patch.  (v3)

r=me with line changed to:
urlbarHistButton.disabled = isBtnDisabled || !locBarPref.checked
Attachment #797747 - Flags: review?(iann_bugzilla) → review+
> >+                  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"
Attached patch Help patch (v3)Splinter Review
Attachment #797753 - Attachment is obsolete: true
Attachment #813435 - Flags: review+
(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?
Flags: needinfo?(iann_bugzilla)
(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
Flags: needinfo?(iann_bugzilla)
Attached patch Proposed patch. (v4) (obsolete) — Splinter Review
Attachment #797747 - Attachment is obsolete: true
Attachment #823219 - Flags: review?(iann_bugzilla)
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
Attachment #823219 - Flags: review?(iann_bugzilla) → review+
Attached patch proposed patch (v5) (obsolete) — Splinter Review
Attachment #823219 - Attachment is obsolete: true
Attachment #8349937 - Flags: review+
Attached patch proposed patch (v6) (obsolete) — Splinter Review
Attachment #8349937 - Attachment is obsolete: true
Attachment #8358780 - Flags: review+
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?
Attachment #8358780 - Flags: review+
Flags: needinfo?(iann_bugzilla)
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?
Attachment #8358780 - Flags: review+
Attachment #8358780 - Flags: review+ → review?(iann_bugzilla)
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.
Attachment #8358780 - Flags: review?(iann_bugzilla) → review+
Flags: needinfo?(iann_bugzilla)
Attached patch proposed patch (v7) (obsolete) — Splinter Review
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.
Attachment #8358780 - Attachment is obsolete: true
Attachment #8450044 - Flags: review?(iann_bugzilla)
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
Attachment #8450044 - Flags: review?(iann_bugzilla) → review+
removed the "this.disabled = true;" part.
Attachment #8450044 - Attachment is obsolete: true
Attachment #8459374 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.30
You need to log in before you can comment on or make changes to this bug.