Closed Bug 1429120 Opened 2 years ago Closed 2 years ago

Policies: display menu bar by default, display bookmarks toolbar by default

Categories

(Firefox :: Enterprise Policies, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox60 --- verified

People

(Reporter: Felipe, Assigned: bytesized)

References

Details

Attachments

(2 files)

Two separate policies:

  - display the menu bar by default (is this only relevant on Windows? and Linux)
  - display the bookmarks toolbar by default

I think it doesn't matter much for sysadmins if this is enforced, or just an option made different from the default (i.e., which still allows the user to change it).
Assignee: nobody → ksteuber
FWIW, if you need to set prefs, use the function implemented by bug 1428923
Attachment #8943816 - Flags: review?(felipc)
Attachment #8943817 - Flags: review?(felipc)
Comment on attachment 8943816 [details]
Bug 1429120 - Create an enterprise policy to display the menu bar by default

https://reviewboard.mozilla.org/r/214194/#review219902
Attachment #8943816 - Flags: review?(felipc) → review+
Comment on attachment 8943817 [details]
Bug 1429120 - Add enterprise policy to display the bookmarks toolbar by default

https://reviewboard.mozilla.org/r/214196/#review219904
Attachment #8943817 - Flags: review?(felipc) → review+
r+ for all the policies-related code, but please request review from Gijs for the actual implementation code of these two policies
Attachment #8943816 - Flags: review?(gijskruitbosch+bugs)
Attachment #8943817 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8943817 [details]
Bug 1429120 - Add enterprise policy to display the bookmarks toolbar by default

https://reviewboard.mozilla.org/r/214196/#review220180

::: browser/components/enterprisepolicies/Policies.jsm:75
(Diff revision 1)
> +        // This policy is meant to change the default behavior, not to force it.
> +        // If this policy was alreay applied and the user chose to re-hide the
> +        // bookmarks toolbar, do not show it again.
> +        if (!Services.prefs.getBoolPref(PREF_BOOKMARKS_ALREADY_DISPLAYED, false)) {
> +          log.debug("Showing the bookmarks toolbar");
> +          let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);

Use `XPCOMUtils.defineLazyServiceGetter` at the top of this file to define a lazy getter for `gXULStore` or something.

::: browser/components/enterprisepolicies/Policies.jsm:76
(Diff revision 1)
> +        // If this policy was alreay applied and the user chose to re-hide the
> +        // bookmarks toolbar, do not show it again.
> +        if (!Services.prefs.getBoolPref(PREF_BOOKMARKS_ALREADY_DISPLAYED, false)) {
> +          log.debug("Showing the bookmarks toolbar");
> +          let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
> +          let browserDocumentUrl = "chrome://browser/content/browser.xul";

Nit: const, and use ALL_CAPS. You're probably going to want this for other things so might as well stick it at the toplevel.

::: browser/components/enterprisepolicies/schemas/policies.json:23
(Diff revision 1)
>        "enum": [true]
>      },
>  
> +    "display_bookmarks_toolbar": {
> +      "description": "Causes the bookmarks toolbar to be displayed by default.",
> +      "first_available": "59.0",

I expect this will need to land with 60.0 unless we're uplifting.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_display_bookmarks.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Normally we license tests as public domain via CC0, unless there's a compelling reason not to:

```
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
```
Attachment #8943817 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8943816 [details]
Bug 1429120 - Create an enterprise policy to display the menu bar by default

https://reviewboard.mozilla.org/r/214194/#review220184

r=me with similar changes to the other review (global the doc URL, gXULStore; separately: test licensing and what version this ships in)

::: browser/components/enterprisepolicies/Policies.jsm:55
(Diff revision 1)
> +          let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
> +          let browserDocumentUrl = "chrome://browser/content/browser.xul";

Same nits as in the other file, then you can omit this duplicate code.

::: browser/components/enterprisepolicies/schemas/policies.json:15
(Diff revision 1)
>        "enum": [true]
>      },
>  
> +    "display_menu_bar": {
> +      "description": "Causes the menu bar to be displayed by default.",
> +      "first_available": "59.0",

Maybe 60 too?
Attachment #8943816 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc33122dc2fa
Create an enterprise policy to display the menu bar by default r=Felipe,Gijs
https://hg.mozilla.org/integration/autoland/rev/95506a51e4ab
Add enterprise policy to display the bookmarks toolbar by default r=Felipe,Gijs
https://hg.mozilla.org/mozilla-central/rev/dc33122dc2fa
https://hg.mozilla.org/mozilla-central/rev/95506a51e4ab
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
We tested "display menu bar and bookmarks toolbar by default" policy using JSON file. We verified this policy implementation as fixed, but still, it will be retested with ADMX files when it is ready for testing.

Test documentation is available here: https://testrail.stage.mozaws.net/index.php?/plans/view/7734
We retested this on beta builds with ADM and JSON policy formats and it is verified as fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.