413437 - Locking homepage buttons to disable them is broken (again)

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Mike Kaply [:mkaply]]

Locking the preferences:

pref.browser.homepage.disable_button.current_page
pref.browser.homepage.disable_button.bookmark_page
pref.browser.homepage.disable_button.restore_default

Is supposed to disable the three buttons on the main page of preferences. It is not.

To test this, add these two preferences to your system:

pref("general.config.filename","mozilla.cfg");
pref("general.config.obscure_value", 0);

And create a file called mozilla.cfg in the same directory as the EXE:

lockPref("pref.browser.homepage.disable_button.current_page", true);
lockPref("pref.browser.homepage.disable_button.bookmark_page", true);
lockPref("pref.browser.homepage.disable_button.restore_default", true);

On trunk, two of the buttons disabled. On 1.8 branch, only one of the buttons disable.

This is a major bug for enterprise

Comment 1

[Michael Schonfeld]

Attachment: v1

I'm not sure if this would constitute as a workaround or not. Never the less, it does work properly.

Comment 2

[Michael Schonfeld]

Michael Kaply:  I'm pretty sure you're aware of this - but I figured I might as well say it... Your mozilla.cfg file must begin with "//".  So in essence, your demonstration file should look like this:

//
lockPref("pref.browser.homepage.disable_button.current_page", true);
lockPref("pref.browser.homepage.disable_button.bookmark_page", true);
lockPref("pref.browser.homepage.disable_button.restore_default", true);

Comment 3

[Michael Schonfeld]

Attachment: v1.2

New version. And its not even a workaround!

Comment 4

[Asaf Romano (gone)]

Comment on attachment 302764 [details] [diff] [review]
v1.2

This check should be done at the top of this function, before we even call getMostRecentWindow. Also, I think the correct check is document.getElementById("...").locked.

You don't need ui-r for bug fixes.

Comment 5

[Michael Schonfeld]

Why would you want to place that check at the top of the function, before the getMostRecentWindow call?

Comment 6

[Michael Schonfeld]

(In reply to comment #4)
> (From update of attachment 302764 [details] [diff] [review])
> This check should be done at the top of this function, before we even call
> getMostRecentWindow. Also, I think the correct check is
> document.getElementById("...").locked.
> 
> You don't need ui-r for bug fixes.
> 
Why would you want to place that check at the top of the function, before the
getMostRecentWindow call?

Comment 7

[Michael Schonfeld]

Attachment: v1.3

Thanks for the review ace, even though it was a -.

Updated patch.

Comment 8

[Michael Schonfeld]

Attachment: v1.4

even better!

Comment 9

[Asaf Romano (gone)]

Comment on attachment 302778 [details] [diff] [review]
v1.4

>Index: browser/components/preferences/main.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/preferences/main.js,v
>retrieving revision 1.16
>diff -u -p -8 -r1.16 main.js
>--- browser/components/preferences/main.js	6 Jan 2008 03:59:16 -0000	1.16
>+++ browser/components/preferences/main.js	12 Feb 2008 09:55:57 -0000
>@@ -126,41 +126,48 @@ var gMainPane = {
> 
>   /**
>    * Switches the "Use Current Page" button between its singular and plural
>    * forms.
>    */
>   _updateUseCurrentButton: function () {
>     var useCurrent = document.getElementById("useCurrent");
> 
>+    var windowIsPresent;
>     var win;
>     if (document.documentElement.instantApply) {
>       const Cc = Components.classes, Ci = Components.interfaces;
>       // If we're in instant-apply mode, use the most recent browser window
>       var wm = Cc["@mozilla.org/appshell/window-mediator;1"]
>                  .getService(Ci.nsIWindowMediator);
>       win = wm.getMostRecentWindow("navigator:browser");
>     }
>     else
>       win = window.opener;
> 
>     if (win && win.document.documentElement
>                   .getAttribute("windowtype") == "navigator:browser") {
>-      useCurrent.disabled = false;
>+      windowIsPresent = true;
> 
>       var tabbrowser = win.document.getElementById("content");
>       if (tabbrowser.browsers.length > 1)
>         useCurrent.label = useCurrent.getAttribute("label2");
>       else
>         useCurrent.label = useCurrent.getAttribute("label1");
>     }
>     else {
>+      windowIsPresent = false;
>       useCurrent.label = useCurrent.getAttribute("label1");
>-      useCurrent.disabled = true;
>     }
>+
>+    if (document.getElementById
>+        ("pref.browser.homepage.disable_button.current_page").locked)
>+      return;
>+
>+    useCurrent.disabled = !windowIsPresent;

Add a comment here explaining that the button disabled state is set by preferences.xml if the preference is locked.

r=mano otherwise.

Comment 10

[Michael Schonfeld]

Attachment: for checkin

Commented.

Comment 11

[Asaf Romano (gone)]

mozilla/browser/components/preferences/main.js 1.17