1439443 - Policies: Remove Default Bookmarks and Smart Bookmarks

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

Policies:

- Remove smart bookmarks
  -> browser.places.smartBookmarksVersion = -1

- Remove the included default bookmarks:
  -> look into browser.bookmarks.restore_default_bookmarks and nsBrowserGlue.
  Probably doable with the constraint that it needs to be set before first run.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1439443 - Policies: Remove Default Bookmarks and Smart Bookmarks.

Review commit: https://reviewboard.mozilla.org/r/228196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228196/

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8959360 [details]
Bug 1439443 - Policies: Remove Default Bookmarks and Smart Bookmarks.

https://reviewboard.mozilla.org/r/228196/#review234108

FWIW, I'm evaluating whether the smart bookmarks idea still makes sense. It was intended to be a way to advertize the places querying capabilities, but we could pretty much expose a better version of most visited from the Address Bar empty dropdown, and Recent Tags falls into tags management that should likely get a new UI in general (Plus is it really useful to know which tags you added recently VS a normal list of tags?). Btw, I don't think it's a problem if in the future the smartBookmarks policy becomes useless, right?

::: browser/components/enterprisepolicies/tests/browser/smart_and_default_bookmarks/browser_policies_smart_and_default_bookmarks.js:12
(Diff revision 1)
> +// No Default Bookmarks policy needs to be present on
> +// the first run of the profile, and not dinamically loaded
> +// like most of the policies tested in the main test folder.
> +
> +add_task(async function test_smart_and_default_bookmarks() {
> +  is(Services.prefs.getIntPref("browser.places.smartBookmarksVersion"), 0, "Smart Bookmarks are disabled");

0 doesn't mean disabled, -1 is disabled, so checking this is not useful.
You're just skipping creation like if the pref would be set to -1

::: browser/components/nsBrowserGlue.js:1652
(Diff revision 1)
>  
>          if (bookmarksUrl) {
>            // Import from bookmarks.html file.
>            try {
> +            if (Services.policies.isAllowed("defaultBookmarks")) {
> -            await BookmarkHTMLUtils.importFromURL(bookmarksUrl, true);
> +              await BookmarkHTMLUtils.importFromURL(bookmarksUrl, true);

I think you should also exclude the above call to
await this._distributionCustomizer.applyBookmarks();
Since if the admin doesn't want default bookmarks, he likely also doesn't work bookmarks from distribution.ini

I wonder if we should factor out the whole import into an helper function and skip all of it. Maybe we could keep this idea for a future cleanup.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(In reply to Marco Bonardo [::mak] from comment #2)
> [...] Btw, I don't think
> it's a problem if in the future the smartBookmarks policy becomes useless,
> right?

Yeah it's fine to make the policy useless in the future. However, if we do plan to drop smart bookmarks, what about we just make this one policy, "NoDefaultBookmarks", which also implies no smart bookmarks? 

It loses a little bit of fine-grained control, but maybe it's gonna be the most common use-case anyway to use these two together.

> 
> > +add_task(async function test_smart_and_default_bookmarks() {
> > +  is(Services.prefs.getIntPref("browser.places.smartBookmarksVersion"), 0, "Smart Bookmarks are disabled");
> 
> 0 doesn't mean disabled, -1 is disabled, so checking this is not useful.
> You're just skipping creation like if the pref would be set to -1

Ah ok. The test passes, so indeed it gets set to 0. But if it's not useful to check that, should I just remove this part of the test?

> 
> 
> I think you should also exclude the above call to
> await this._distributionCustomizer.applyBookmarks();
> Since if the admin doesn't want default bookmarks, he likely also doesn't
> work bookmarks from distribution.ini

I thought about this too, but I believe there will be people who might gradually migrate to policies, while still keep using their distribution.ini to fill the bookmarks.

Comment 4

[Marco Bonardo [:mak]]

(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> (In reply to Marco Bonardo [::mak] from comment #2)
> > [...] Btw, I don't think
> > it's a problem if in the future the smartBookmarks policy becomes useless,
> > right?
> 
> Yeah it's fine to make the policy useless in the future. However, if we do
> plan to drop smart bookmarks, what about we just make this one policy,
> "NoDefaultBookmarks", which also implies no smart bookmarks? 

I'm fine with that and I think it makes sense.

> Ah ok. The test passes, so indeed it gets set to 0. But if it's not useful
> to check that, should I just remove this part of the test?

Yes I think checking the pref is pointless.

> > I think you should also exclude the above call to
> > await this._distributionCustomizer.applyBookmarks();
> > Since if the admin doesn't want default bookmarks, he likely also doesn't
> > work bookmarks from distribution.ini
> 
> I thought about this too, but I believe there will be people who might
> gradually migrate to policies, while still keep using their distribution.ini
> to fill the bookmarks.

Are policies intended to replace distribution.ini. My understanding was that distribution was more for partners distributing Firefox than enterprise deployment. Worth clarifying what's our intent.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e35648ab76
Policies: Don't create the default and smart bookmarks on the profile. r=mak

Comment 6

[:Felipe Gomes (needinfo for replies!)]

[Tracking Requested - why for this release]:
Enterprise Policies for ESR

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/92e35648ab76

Comment 8

[Abe - QA (:Abe_LV)]

We tested this on latest nightly with JSON policy format and it is verified as fixed.

It will also be tested with the admx format when implemented.

With this policy, a fresh profile starts with no bookmarks. 

The bug will be marked as verified when it is tested with the admx format as well. 
But, the JSON format is verified here.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8959360 [details]
Bug 1439443 - Policies: Remove Default Bookmarks and Smart Bookmarks.

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise policy to remove the default bookmarks (bookmarks shipped by default with Firefox)
[User impact if declined]: this policy will be missing
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: QA has already verified
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: limited to this policy being used
[String changes made/needed]: none

Comment 10

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8959360 [details]
Bug 1439443 - Policies: Remove Default Bookmarks and Smart Bookmarks.

policy for default bookmarks, beta60+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Needs a rebased patch for Beta uplift.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/75e3a82eac12

Comment 13

[Abe - QA (:Abe_LV)]

We re-tested this on Nightly 61 with adm file and it is verified as fixed.
Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760

Comment 14

[Abe - QA (:Abe_LV)]

We retested this on beta builds[FX61]  with ADM and JSON policy formats and it is verified as fixed.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760