1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with something else

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P1)

Description

[Mark Banner (:standard8)]

We're working towards removing annotations from the bookmarks side of places (bug 1460577).

As part of that, the 'bookmarkPropertiesDialog/folderLastUsed' annotation needs replacing.

After having discussed with Marco, it seems that it might be best to store the data in the moz_meta table (a key/value store associated with the places database, but doesn't get synced).

Comment 1

[Mark Banner (:standard8)]

I'm investigating this, looks like it requires a little rework, but generally not too hard.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

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

Comment 3

[Mark Banner (:standard8)]

I realise we're delaying landing of migration affecting patches at the moment, hence the '99' and 'TBD' references in the patch.

Thought I'd get this in the review queue anyway so it can be close to landing once unblocked & also setting off a try server build to check for issues.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8975957 [details]
Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244182/diff/1-2/

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8975957 [details]
Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

Need to look a bit more into the failures in this. I suspect it may be needing waitForCondition or something, but taking out the review queue until I figure it out.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8975957 [details]
Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244182/diff/2-3/

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8975957 [details]
Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244182/diff/3-4/

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8975957 [details]
Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

https://reviewboard.mozilla.org/r/244182/#review252848

::: browser/base/content/browser-places.js:126
(Diff revision 4)
>            } else if (this._isNewBookmark) {
>              LibraryUI.triggerLibraryAnimation("bookmark");
>            }
> +
> +          if (!removeBookmarksOnPopupHidden) {
> +            this._storeRecentlyUsedFolder(selectedFolderGuid).catch(ex => console.error);

nit: just (console.error)?

::: browser/base/content/browser-places.js:348
(Diff revision 4)
> +
> +  async _storeRecentlyUsedFolder(selectedFolderGuid) {
> +    // These are displayed by default, so don't save the folder for them.
> +    if (selectedFolderGuid == PlacesUtils.bookmarks.unfiledGuid ||
> +        selectedFolderGuid == PlacesUtils.bookmarks.toolbarGuid ||
> +        selectedFolderGuid == PlacesUtils.bookmarks.menuGuid) {

I think we should not store any of the userContentRoots, that also simplifies this check. Storing into mobile from desktop doesn't sound like a good habit anyway.

::: browser/base/content/browser-places.js:353
(Diff revision 4)
> +        selectedFolderGuid == PlacesUtils.bookmarks.menuGuid) {
> +      return;
> +    }
> +
> +    // List of recently used folders:
> +    let lastUsedFolderGuids = await PlacesUIUtils.getLastUsedFoldersMetaData();

could we avoid getLastUsedFoldersMetaData if we'd add an optional defaultValue argument to PlacesUtils.metadata.get?

I'll go a bit further, if one passes a non-null object or array to metadata.set we store a string like 
 "data:application/json;base64," + btoa(JSON.stringify(val));
when get() is invoked we just check for the data prefix and unwrap it.
If a default argument is provided, the parsed value must be coherent with the default argument type.
If parsing fails the default argument is returned.
If parsing fails and there's no default argument, we throw.

Then you could just
let lastUsedFolderGuids = PlacesUtils.metadata.get(PlacesUIUtils.LAST_USED_META_DATA, []);

This change will require a dependency bug.

::: browser/components/places/PlacesUIUtils.jsm:214
(Diff revision 4)
>  };
>  
>  var PlacesUIUtils = {
>    LOAD_IN_SIDEBAR_ANNO: "bookmarkProperties/loadInSidebar",
>    DESCRIPTION_ANNO: "bookmarkProperties/description",
> +  LAST_USED_META_DATA: "places/bookmarks/edit/lastusedfolder",

I'd simplify the key to just
bookmarks/lastusedfolders (plural)
and name this LAST_USED_FOLDERS_META_KEY

::: browser/components/places/PlacesUIUtils.jsm:1075
(Diff revision 4)
> +  /**
> +   * Returns saved meta data for the last used folders.
> +   *
> +   * @return {Array} An array of GUIDs of the last used folders.
> +   */
> +  async getLastUsedFoldersMetaData() {

As said, we can probably do this transparently in metadata

::: browser/components/places/tests/browser/browser_bookmarkProperties_remember_folders.js:3
(Diff revision 4)
> +/* 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/. */

PD, if you don't mind

::: browser/components/places/tests/browser/browser_bookmarkProperties_remember_folders.js:17
(Diff revision 4)
> +let folders;
> +
> +async function clickBookmarkStar() {
> +  let shownPromise = promisePopupShown(bookmarkPanel);
> +  BookmarkingUI.star.click();
> +  await shownPromise;

you may want to disable animations when opening the panel, through bookmarkPanel.setAttribute("animate", false); and restore it in registerCleanupFunction

::: browser/components/places/tests/browser/browser_bookmarkProperties_remember_folders.js:38
(Diff revision 4)
> +  // Now select the item in the tree.
> +  document.getElementById("editBMPanel_folderTree").selectItems([guid]);
> +
> +  await hideBookmarksPanel();
> +
> +  await PlacesTestUtils.promiseAsyncUpdates();

nit: lots of newlines in this helper...

::: browser/components/places/tests/browser/browser_bookmarkProperties_remember_folders.js:60
(Diff revision 4)
> +      }
> +    }
> +  }
> +
> +  await TestUtils.waitForCondition(() => {
> +    getGuids(); return actualGuids.length == expectedGuids.length;

missing newline

::: toolkit/components/places/Database.cpp:2598
(Diff revision 4)
> +  }
> +  void Write(const char* aStr) override { mCString.Append(aStr); }
> +};
> +
> +nsresult
> +Database::MigrateV51Up()

I was thinking we could use MigrateUI for this... it would not support downgrade, but that's a minor problem in this case.
The only thing I'm unsure is whether it's ok to init the Places connection so early, and I think it's not, because migrateUI happens at final-ui-startup, that probably happens before migration? (the docs say it's fired before the first app window is shown, not specifying if it must be a browser window).
Anyway probably not worth the risk :(

::: toolkit/components/places/Database.cpp:2607
(Diff revision 4)
> +    "SELECT b.guid FROM moz_anno_attributes n "
> +    "JOIN moz_items_annos a ON n.id = a.anno_attribute_id "
> +    "JOIN moz_bookmarks b ON a.item_id = b.id "
> +    "WHERE n.name = :anno_name ORDER BY a.content DESC"
> +  ), getter_AddRefs(stmt));
> +  NS_ENSURE_SUCCESS(rv, rv);

This may become problematic once we remove item annotations and someone downgrades/upgrades.
Though I suppose we'll have to leave moz_items_annos as an empty table for a while for downgrades, so it may not be that critical.
Anyway, we can probably:

if (NS_FAILED(rv)) {
  MOZ_ASSERT(false, "Should succeed unless item annotations table has been removed");
  return NS_OK;
));

::: toolkit/components/places/Database.cpp:2613
(Diff revision 4)
> +
> +  rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("anno_name"),
> +                                  NS_LITERAL_CSTRING(LAST_USED_ANNO));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCString json;

Auto

::: toolkit/components/places/Database.cpp:2625
(Diff revision 4)
> +    jw.StringElement(stmt->AsSharedUTF8String(0, &length));
> +  }
> +  jw.EndArray();
> +
> +  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "INSERT INTO moz_meta "

in case of downgrade/upgrade there could be a value there already, and this would fail. Should be an INSERT OR REPLACE

::: toolkit/components/places/Database.cpp:2631
(Diff revision 4)
> +    "VALUES (:key, :value) "
> +  ), getter_AddRefs(stmt));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("key"),
> +                                  NS_LITERAL_CSTRING(LAST_USED_META_DATA));

nit: store the literal string in the define

::: toolkit/components/places/Database.cpp:2634
(Diff revision 4)
> +
> +  rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("key"),
> +                                  NS_LITERAL_CSTRING(LAST_USED_META_DATA));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("value"),

nit: remove newline (and oneline?)

::: toolkit/components/places/Database.cpp:2646
(Diff revision 4)
> +  // Clean up the now redundant annotations.
> +  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "DELETE FROM moz_items_annos WHERE anno_attribute_id = "
> +      "(SELECT id FROM moz_anno_attributes WHERE name = :anno_name) "
> +  ), getter_AddRefs(stmt));
> +  NS_ENSURE_SUCCESS(rv, rv);

if this fails we should ignore it with an assert, like for the initial statement.

::: toolkit/components/places/tests/head_common.js:7
(Diff revision 4)
>   * 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/. */
>  
> -const CURRENT_SCHEMA_VERSION = 50;
> +const CURRENT_SCHEMA_VERSION = 51;
>  const FIRST_UPGRADABLE_SCHEMA_VERSION = 30;

ah, we can move both of these to migration/head.js!

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8975957 [details]
Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244182/diff/4-5/

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8975957 [details]
Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

https://reviewboard.mozilla.org/r/244182/#review255252

::: browser/base/content/browser-places.js:126
(Diff revision 5)
>            } else if (this._isNewBookmark) {
>              LibraryUI.triggerLibraryAnimation("bookmark");
>            }
> +
> +          if (!removeBookmarksOnPopupHidden) {
> +            this._storeRecentlyUsedFolder(selectedFolderGuid).catch(console.error);

based just on code inspection, theorically we may hit a case where selectedFolderGuid is undefined or false. We should probably check it here or in _storeRecentlyUsedFolder before using it.

::: browser/base/content/browser-places.js:378
(Diff revision 5)
> +    if (lastUsedFolderGuids.length > 5) {
> +      lastUsedFolderGuids.pop();
> +    }
> +
> +    await PlacesUtils.metadata.set(PlacesUIUtils.LAST_USED_FOLDERS_META_KEY,
> +      lastUsedFolderGuids);

nit: align arguments

::: browser/components/places/tests/browser/browser_bookmarkProperties_remember_folders.js:16
(Diff revision 5)
> +const bookmarkPanel = document.getElementById("editBookmarkPanel");
> +let folders;
> +
> +async function clickBookmarkStar() {
> +  let shownPromise = promisePopupShown(bookmarkPanel);
> +  BookmarkingUI.star.click();

May want to set bookmarkPanel.setAttribute("animate", false); and removeAttribute in registerCleanupFunction, to speed up opening and reduce likelihood of failures...

Or maybe we should move these to helpers in head...

::: browser/components/places/tests/browser/browser_bookmarkProperties_remember_folders.js:35
(Diff revision 5)
> +  // Expand the folder tree.
> +  document.getElementById("editBMPanel_foldersExpander").click();
> +  document.getElementById("editBMPanel_folderTree").selectItems([guid]);
> +
> +  await hideBookmarksPanel();
> +  await PlacesTestUtils.promiseAsyncUpdates();

is this necessary?
One day we should probably see which of the many calls to promiseAsyncUpdates are actually necessary.

::: toolkit/components/places/Database.cpp:2612
(Diff revision 5)
> +  while (NS_SUCCEEDED(stmt->ExecuteStep(&hasMore)) && hasMore) {
> +    jw.StringElement(stmt->AsSharedUTF8String(0, &length));
> +  }
> +  jw.EndArray();
> +
> +  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(

Shouldn't we bail out here if we didn't find any anno?

::: toolkit/components/places/Database.cpp:2622
(Diff revision 5)
> +
> +  rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("key"),
> +                                  LAST_USED_FOLDERS_META_KEY);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("value"),

nit: remove newline

::: toolkit/components/places/Database.cpp:2626
(Diff revision 5)
> +
> +  rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("value"),
> +                                  json);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = stmt->Execute();

nit: remove newline

::: toolkit/components/places/Database.cpp:2638
(Diff revision 5)
> +  ), getter_AddRefs(stmt));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("anno_name"), LAST_USED_ANNO);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = stmt->Execute();

nit: remove newline

::: toolkit/components/places/Database.cpp:2645
(Diff revision 5)
> +
> +  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "DELETE FROM moz_anno_attributes WHERE name = :anno_name "
> +  ), getter_AddRefs(stmt));
> +  if (NS_FAILED(rv)) {
> +    MOZ_ASSERT(false, "Should succeed unless item annotations table has been removed");

this is unnecessary, we'd already bail out at the first CreateStatement, just NS_ENSURE_SUCCESS

::: toolkit/components/places/Database.cpp:2651
(Diff revision 5)
> +    return NS_OK;
> +  };
> +  rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("anno_name"),  LAST_USED_ANNO);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = stmt->Execute();

nit: remove newline

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8975957 [details]
Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

https://reviewboard.mozilla.org/r/244182/#review255252

> May want to set bookmarkPanel.setAttribute("animate", false); and removeAttribute in registerCleanupFunction, to speed up opening and reduce likelihood of failures...
> 
> Or maybe we should move these to helpers in head...

The attribute is set in the setup() function (and reset in there as well).

> is this necessary?
> One day we should probably see which of the many calls to promiseAsyncUpdates are actually necessary.

It does seem necessary as the test is frequent intermittent without it. From what I can tell, we need it to ensure that we've had time to write the metadata to disk before doing other parts of the test.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8975957 [details]
Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244182/diff/5-6/

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8975957 [details]
Bug 1460579 - Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244182/diff/6-7/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d31e89c47054
Replace the 'bookmarkPropertiesDialog/folderLastUsed' annotation with a key/value pair in moz_meta. r=mak

Comment 15

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d31e89c47054