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".

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()

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

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 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 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 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

> >+ 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"

(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?

(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 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 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 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 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.

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 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

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

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