Closed Bug 1428924 Opened 6 years ago Closed 6 years ago

Policy: Allow creation of bookmarks in the Bookmarks toolbar, Menu, or a folder inside them

Categories

(Firefox :: Enterprise Policies, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox59 --- wontfix
firefox60 --- verified

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(1 file)

There are a few policies that are related to the places feature:

- Adding default bookmarks to the toolbar
- Adding default bookmarks to the menu
- Removing some of the included default bookmarks
- Removing some of the included smart bookmarks

We can probably implement all of them on this same bug.
Depends on: 1438577
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Comment on attachment 8951761 [details]
Bug 1428924 - Policy: Allow creation of bookmarks in the Bookmarks toolbar, Menu, or a folder inside them.

Marco, can you take a look to see if this looks reasonable? It already works as I wanted, but I don't know if the additions I did to the search() function, and the way I'm using it, are reasonable or hacky.

I imagine that what you had in mind for bug 1438577 was to use the fetch() function instead of search(), right? Is fetch more efficient?
Attachment #8951761 - Flags: feedback?(mak77)
search is a special api that currently only exists for WebExtensions (because we don't yet have an async querying API). It's not a matter of efficiency but of API clarity (in practice search is deprecated by its birth, one day it will have to go for a proper querying api, as its javadoc says).
Also, I see you're searching by parentGuid, I thought we had figured out it was not necessary?
(In reply to Marco Bonardo [::mak] from comment #3)
> Also, I see you're searching by parentGuid, I thought we had figured out it
> was not necessary?

Yeah, it was simple to do it this way on the search() function, but it's not necessary. I'm doing it because in theory the admin could have specified two folders (one in the toolbar, one in the menu) with the same name. But I can easily do that in the .filter() function after I get the results.
Depends on: 1439418
Ok, I got rid of the .search() usage and used the new guidPrefix API from bug 1438577.

Since the code was working as I wanted with search(), I wrote the tests before switching APIs, to make sure everything still worked as expected.

Since this patch is big, I'll make this bug to be only about the Bookmarks policy, and will create a new bug to implement the policies about getting rid of the smart bookmarks and default bookmarks.
Attachment #8951761 - Flags: feedback?(mak77)
Summary: Implement Places policies → Policy: Allow creation of bookmarks in the Bookmarks toolbar, Menu, or a folder inside them
Comment on attachment 8951761 [details]
Bug 1428924 - Policy: Allow creation of bookmarks in the Bookmarks toolbar, Menu, or a folder inside them.

https://reviewboard.mozilla.org/r/221032/#review227432

mostly ok, I have a few questions

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:5
(Diff revision 2)
> +/* 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/. */
> +
> +"use strict";

I think you should have some documentation about the format of bookmarks in the policy config at the top of this module.

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:9
(Diff revision 2)
> +
> +"use strict";
> +
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +ChromeUtils.import("resource://gre/modules/PlacesUtils.jsm");

It's true that this module is imported lazily already, but I'd still probably defineModuleGetter PlacesUtils.

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:11
(Diff revision 2)
> +
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +ChromeUtils.import("resource://gre/modules/PlacesUtils.jsm");
> +
> +const PREF_LOGLEVEL           = "browser.policies.loglevel";

nit: the spacing looks odd

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:42
(Diff revision 2)
> +
> +async function addRemoveBookmarks(specifiedBookmarks) {
> +  let results = await calculateLists(specifiedBookmarks);
> +
> +  for (let bookmark of results.add.values()) {
> +    await insertBookmark(bookmark);

insertBookmark will stop at errors, how should it behave, continue with the next item or give up in the middle as it is now?

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:42
(Diff revision 2)
> +
> +async function addRemoveBookmarks(specifiedBookmarks) {
> +  let results = await calculateLists(specifiedBookmarks);
> +
> +  for (let bookmark of results.add.values()) {
> +    await insertBookmark(bookmark);

insertBookmark will stop at errors, and we won't even proceed with removals. How should it behave, continue with the next item or give up in the middle as it is now?

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:77
(Diff revision 2)
> +    (result) => existingBookmarks.push(result)
> +  );
> +
> +  for (let bookmark of existingBookmarks) {
> +    // log.debug(`ADDING2: ${bookmark.url.href}`);
> +    existingBookmarksMap.set(bookmark.url.href, bookmark);

why don't you directly add to the map from the fetch callback instead of building a temporary array?
Thw work you do doesn't seem particularly more expensive

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:135
(Diff revision 2)
> +      if (!foldersSeen.has(folder.title)) {
> +        log.debug(`Folder to remove: ${folder.title}`);
> +        foldersToRemove.add(folder);
> +      }
> +    }
> +  }

Again, you could directly use the callback and avoid the temp array

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:148
(Diff revision 2)
> +  }
> +
> +  return {
> +    add: specifiedBookmarksMap,
> +    remove: existingBookmarksMap,
> +    emptyFolders: foldersToRemove

Is there a reason to separate emptyFolders from remove? In the end they are expected to do the same removal action.

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:154
(Diff revision 2)
> +  };
> +}
> +
> +async function insertBookmark(bookmark) {
> +  let parentGuid = await getParentGuid(bookmark.placement,
> +                                            bookmark.folder);

indentation

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:157
(Diff revision 2)
> +async function insertBookmark(bookmark) {
> +  let parentGuid = await getParentGuid(bookmark.placement,
> +                                            bookmark.folder);
> +
> +  await PlacesUtils.bookmarks.insert({
> +    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,

type can be omitted since you specify a url

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:166
(Diff revision 2)
> +    parentGuid,
> +  });
> +
> +  if (bookmark.favicon) {
> +    await new Promise(resolve => {
> +      PlacesUtils.favicons.setAndFetchFaviconForPage(

How are icons defined in the policy file? This by the look of it will try to fetch the icon from the network. Is that ok?
If you want to set an icon for which you have data or a dataurl, you should call replaceFaviconData or replaceFaviconDataFromDataURL BEFORE calling setandFetchFaviconForPage.

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:182
(Diff revision 2)
> +
> +async function removeBookmark(bookmark) {
> +  try {
> +    await PlacesUtils.bookmarks.remove(bookmark);
> +  } catch (ex) {
> +    log.error(`Error removing bookmark ${bookmark.title}`);

You could just 
await PlacesUtils.bookmarks.remove(bookmark).catch(...);
above and completely avoid this removeBookmark wrapper, I think.

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:215
(Diff revision 2)
> +
> +  let validFolders = folders.filter(bookmark => {
> +    return bookmark.type == PlacesUtils.bookmarks.TYPE_FOLDER &&
> +           bookmark.title == folderTitle &&
> +           bookmark.parentGuid == parentGuid;
> +  });

You could probably keep around a cache for folders, it doesn't make much sense to requery them for every bookmark, you can fetch it once, and then add/remove from the cache when you modify them.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_bookmarks.js:11
(Diff revision 2)
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +ChromeUtils.import("resource://gre/modules/PlacesUtils.jsm");
> +
> +const BOOKMARK_GUID_PREFIX = "PolB-";
> +const FOLDER_GUID_PREFIX   = "PolF-";

BookmarksPolicies could expose these

::: browser/components/enterprisepolicies/tests/browser/browser_policy_bookmarks.js:62
(Diff revision 2)
> +}
> +
> +function findBookmarkInPolicy(bookmark) {
> +  // Find the entry in the given policy that corresponds
> +  // to this bookmark object from Places.
> +  for (let i = 0; i < CURRENT_POLICY.policies.Bookmarks.length; i++) {

for...of?

::: browser/components/enterprisepolicies/tests/browser/browser_policy_bookmarks.js:71
(Diff revision 2)
> +  }
> +  return null;
> +}
> +
> +async function promiseAllChangesMade({itemsToAdd, itemsToRemove}) {
> +  return new Promise(resolve => {

I suspect (but didn't verify) you could even do:
let a = 0, r = 0;
return Promise.all([
  PlacestestUtils.waitForNotification("onItemAdded",
    () => ++a == itemsToAdd),
  PlacestestUtils.waitForNotification("onItemRemoved",
    () => ++r == itemsToRemove);
]);

::: browser/components/enterprisepolicies/tests/browser/browser_policy_bookmarks.js:75
(Diff revision 2)
> +async function promiseAllChangesMade({itemsToAdd, itemsToRemove}) {
> +  return new Promise(resolve => {
> +    let bmObserver = {
> +      onItemAdded() {
> +        itemsToAdd--;
> +        if(itemsToAdd == 0 && itemsToRemove == 0) {

there are a few eslint errors around, like the missing space after if here and below.
Attachment #8951761 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #7)
> I suspect (but didn't verify) you could even do:
> let a = 0, r = 0;
> return Promise.all([
>   PlacestestUtils.waitForNotification("onItemAdded",
>     () => ++a == itemsToAdd),
>   PlacestestUtils.waitForNotification("onItemRemoved",
>     () => ++r == itemsToRemove);
> ]);

This was the only change that I didn't make, because PlacesTestUtils is not accessible from here. Since it's working fine this way, I just left it as is.

I added the correct support for favicons and a test for it.
(In reply to :Felipe Gomes (needinfo me!) from comment #9)
> This was the only change that I didn't make, because PlacesTestUtils is not
> accessible from here. Since it's working fine this way, I just left it as is.

it's not important, but what do you mean by not accessible? it's a normal module that you can import.
(In reply to Marco Bonardo [::mak] from comment #10)
> it's not important, but what do you mean by not accessible? it's a normal
> module that you can import.

Ah, for some reason I got confused and thought that function was from a head.js file and not a module
Comment on attachment 8951761 [details]
Bug 1428924 - Policy: Allow creation of bookmarks in the Bookmarks toolbar, Menu, or a folder inside them.

https://reviewboard.mozilla.org/r/221032/#review228216

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:11
(Diff revision 3)
> +
> +/*
> + * A Bookmark object received through the policy engine will be an
> + * object with the following properties:
> + *
> + * - URL (nsIURI)

why is this an nsIURI rather than a URL object? Are we using xpconnect anywhere?
The only Places API left using nsIURI is the favicons one, but you can easily newURI() when necessary.

PS: I'd also suggest so use a lowercase url property, but maybe you already have a naming convention for other parts of this module.

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:32
(Diff revision 3)
> + *
> + * - favicon (nsIURI)
> + *   (optional) A http, https or data URL with the favicon. It's
> + *   recommended that this parameter not be used, and let the favicon
> + *   be downloaded by normal usage when the bookmarked websited is
> + *   visited.

I'd probably rephrase this, since it reads a bit convoluted. also some typos like "websited"

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:64
(Diff revision 3)
> +
> +this.EXPORTED_SYMBOLS = [
> +  "BookmarksPolicies",
> +  "BOOKMARK_GUID_PREFIX",
> +  "FOLDER_GUID_PREFIX",
> +];

Just expose these const as properties of BookmarksPolicies, so that you export only one object and it's simpler to defineModuleGetter it.

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:64
(Diff revision 3)
> +
> +this.EXPORTED_SYMBOLS = [
> +  "BookmarksPolicies",
> +  "BOOKMARK_GUID_PREFIX",
> +  "FOLDER_GUID_PREFIX",
> +];



::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:260
(Diff revision 3)
> +  // the correct character length.
> +  return prefix + PlacesUtils.history.makeGuid().substring(prefix.length);
> +}
> +
> +// Cache of folder titles to guids to be used by the getParentGuid function.
> +const folderTitlesToGuidMap = new Map();

maybe add g prefix, just to clarify in code where it comes from.

::: browser/components/enterprisepolicies/helpers/BookmarksPolicies.jsm:282
(Diff revision 3)
> +
> +  let folders = [];
> +  await PlacesUtils.bookmarks.fetch(
> +    { guidPrefix: FOLDER_GUID_PREFIX },
> +    (result) => folders.push(result)
> +  );

You could add to the cache all the "title, guids" tuples we fetched here.
But actually, my idea was slightly different. You could fetch only once, you don't need to fetch again after that initial one because you're in control of new folder creations. Instead here you fetch the same list for every unknown folder you encounter.

You can make folderTitlesToGuidMap a lazy getter of a promise that runs once PlacesUtils.bookmarks.fetch, and then just returns the built map.
Then in getParentGuid you can await that promise. if the guid is not in the cache, you create the folder and add it to the cache.

Something like this:
XPCOMUtils.defineLazyGetter(this, "gFolderTitleToGuidMapPromise", () => new Promise(resolve => {
  let folders = new Map();
  PlacesUtils.bookmarks.fetch(
    { guidPrefix: FOLDER_GUID_PREFIX },
    f => folders.set(f.title, f.guid)
  ).then(() => resolve(folders));
}));
...
let foldersMap = await gFolderTitleToGuidMapPromise;
...
if (missing)
  create...
  foldersMap.set...
Attachment #8951761 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #12)
> why is this an nsIURI rather than a URL object? Are we using xpconnect
> anywhere?
> The only Places API left using nsIURI is the favicons one, but you can
> easily newURI() when necessary.

The first policies to use the URL type were policies related to the permissions manager, which also uses nsIURIs. I think it'll be nice to change it to use URL objects, but I'd like to avoid doing it now. I'll file a bug for it.

> 
> PS: I'd also suggest so use a lowercase url property, but maybe you already
> have a naming convention for other parts of this module.

The naming convention is in flux.. I prefer lowercase too, but it looks we'll settle to CamelCase (see bug 1440030). So instead of changing "URL" to "url", I think I'll end up changing "title" to "Title", "Folder", etc..  :'(  But that's the standard on the windows registry where we expect the most usage for the policy engine


> Something like this:
> XPCOMUtils.defineLazyGetter(this, "gFolderTitleToGuidMapPromise", () => new
> Promise(resolve => {
>   let folders = new Map();
>   PlacesUtils.bookmarks.fetch(
>     { guidPrefix: FOLDER_GUID_PREFIX },
>     f => folders.set(f.title, f.guid)
>   ).then(() => resolve(folders));
> }));
> ...
> let foldersMap = await gFolderTitleToGuidMapPromise;
> ...
> if (missing)
>   create...
>   foldersMap.set...

Ah, that's a good idea. I'll change to this approach
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/771e1f428eea
Policy: Allow creation of bookmarks in the Bookmarks toolbar, Menu, or a folder inside them. r=mak
Backed out for failing mochitest browser chrome at browser/components/enterprisepolicies/tests/browser/browser_policy_bookmarks.js

Push that contains the failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=afd291850b8021bf103678d808514530c84895df

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=163798748&repo=autoland&lineNumber=4787

Backout: https://hg.mozilla.org/integration/autoland/rev/3ab43ccebcdebfe946c451e82b4798db019d474e
Flags: needinfo?(felipc)
I believe the cause of the failure was because I forgot to `await` on the setupPolicyEngineWithJSON function. I'll push it again with this change
Flags: needinfo?(felipc)
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/353d68d27cd8
Policy: Allow creation of bookmarks in the Bookmarks toolbar, Menu, or a folder inside them. r=mak
https://hg.mozilla.org/mozilla-central/rev/353d68d27cd8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Marking wontfix for 59 since we are planning this feature for 60.
We tested “add default bookmarks to the toolbar and/or menu” policy using JSON file.  Bookmarks can be added to toolbar, menu or a folder using policies.  
So we verified this manually as fixed.

Test steps and runs are available here: https://testrail.stage.mozaws.net/index.php?/plans/view/7734

The bug will also be retested with ADMX files when it is ready for testing.
We retested this with adm file on beta 60 and it is verified as fixed.
We retested this on beta builds[FX60] 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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.