Bug 1678607: Make BookmarkTagsChanged event have tags. r=mak
☠☠ backed out by 1b6f0e11b8bb ☠ ☠
authorDaisuke Akatsuka <daisuke@birchill.co.jp>
Sun, 17 Oct 2021 22:47:37 +0000
changeset 596150 bf20be578758bb517b79e7bd1d494a581e5c0f53
parent 596149 fd3fbf15e95df232a1ae494349cfd9ce57e428e2
child 596151 002603264291acc776613e0d2480a99d0043d926
push id151649
push userdakatsuka.birchill@mozilla.com
push dateSun, 17 Oct 2021 23:08:16 +0000
treeherderautoland@bf20be578758 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1678607
milestone95.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1678607: Make BookmarkTagsChanged event have tags. r=mak Differential Revision: https://phabricator.services.mozilla.com/D128328
browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js
dom/base/PlacesBookmarkTags.h
dom/chrome-webidl/PlacesEvent.webidl
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/tests/bookmarks/head_bookmarks.js
toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
--- a/browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js
+++ b/browser/components/places/tests/browser/browser_bookmarkProperties_editTagContainer.js
@@ -31,22 +31,17 @@ add_task(async function() {
   Assert.ok(
     tree.controller.isCommandEnabled("placesCmd_show:info"),
     "'placesCmd_show:info' on current selected node is enabled"
   );
 
   let promiseTagResetNotification = PlacesTestUtils.waitForNotification(
     "bookmark-tags-changed",
     events =>
-      events.some(event => {
-        const tags = PlacesUtils.tagging.getTagsForURI(
-          Services.io.newURI(event.url)
-        );
-        return tags.length === 1 && tags[0] === "tag1";
-      }),
+      events.some(({ tags }) => tags.length === 1 && tags[0] === "tag1"),
     "places"
   );
 
   await withBookmarksDialog(
     true,
     function openDialog() {
       tree.controller.doCommand("placesCmd_show:info");
     },
@@ -61,22 +56,17 @@ add_task(async function() {
       let namepicker = dialogWin.document.getElementById(
         "editBMPanel_namePicker"
       );
       Assert.ok(!namepicker.readOnly, "Name field should not be read-only");
       Assert.equal(namepicker.value, "tag1", "Node title is correct");
       let promiseTagChangeNotification = PlacesTestUtils.waitForNotification(
         "bookmark-tags-changed",
         events =>
-          events.some(event => {
-            const tags = PlacesUtils.tagging.getTagsForURI(
-              Services.io.newURI(event.url)
-            );
-            return tags.length === 1 && tags[0] === "tag2";
-          }),
+          events.some(({ tags }) => tags.length === 1 && tags[0] === "tag2"),
         "places"
       );
 
       let promiseTagRemoveNotification = PlacesTestUtils.waitForNotification(
         "bookmark-removed",
         events =>
           events.some(
             event => event.parentGuid == PlacesUtils.bookmarks.tagsGuid
--- a/dom/base/PlacesBookmarkTags.h
+++ b/dom/base/PlacesBookmarkTags.h
@@ -19,31 +19,36 @@ class PlacesBookmarkTags final : public 
   static already_AddRefed<PlacesBookmarkTags> Constructor(
       const GlobalObject& aGlobal, const PlacesBookmarkTagsInit& aInitDict) {
     RefPtr<PlacesBookmarkTags> event = new PlacesBookmarkTags();
     event->mId = aInitDict.mId;
     event->mItemType = aInitDict.mItemType;
     event->mUrl = aInitDict.mUrl;
     event->mGuid = aInitDict.mGuid;
     event->mParentGuid = aInitDict.mParentGuid;
+    event->mTags = aInitDict.mTags;
     event->mLastModified = aInitDict.mLastModified;
     event->mSource = aInitDict.mSource;
     event->mIsTagging = aInitDict.mIsTagging;
     return event.forget();
   }
 
   JSObject* WrapObject(JSContext* aCx,
                        JS::Handle<JSObject*> aGivenProto) override {
     return PlacesBookmarkTags_Binding::Wrap(aCx, this, aGivenProto);
   }
 
   const PlacesBookmarkTags* AsPlacesBookmarkTags() const override {
     return this;
   }
 
+  void GetTags(nsTArray<nsString>& aTags) const { aTags.Assign(mTags); }
+
+  nsTArray<nsString> mTags;
+
  private:
   ~PlacesBookmarkTags() = default;
 };
 
 }  // namespace dom
 }  // namespace mozilla
 
 #endif
--- a/dom/chrome-webidl/PlacesEvent.webidl
+++ b/dom/chrome-webidl/PlacesEvent.webidl
@@ -307,24 +307,31 @@ interface PlacesBookmarkGuid : PlacesBoo
 };
 
 dictionary PlacesBookmarkTagsInit {
   required long long id;
   required unsigned short itemType;
   DOMString? url = null;
   required ByteString guid;
   required ByteString parentGuid;
+  required sequence<DOMString> tags;
   required long long lastModified;
   required unsigned short source;
   required boolean isTagging;
 };
 
 [ChromeOnly, Exposed=Window]
 interface PlacesBookmarkTags : PlacesBookmarkChanged {
   constructor(PlacesBookmarkTagsInit initDict);
+
+  /**
+   * Tags the bookmark has currently.
+   */
+  [Constant,Cached]
+  readonly attribute sequence<DOMString> tags;
 };
 
 dictionary PlacesBookmarkTimeInit {
   required long long id;
   required unsigned short itemType;
   DOMString? url = null;
   required ByteString guid;
   required ByteString parentGuid;
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -333,26 +333,29 @@ var Bookmarks = Object.freeze({
           parentGuid: item.parentGuid,
           source: item.source,
           isTagging: isTagging || isTagsFolder,
         }),
       ];
 
       // If it's a tag, notify bookmark-tags-changed event to all bookmarks for this URL.
       if (isTagging) {
+        const tags = PlacesUtils.tagging.getTagsForURI(NetUtil.newURI(url));
+
         for (let entry of await fetchBookmarksByURL(item, {
           concurrent: true,
         })) {
           notifications.push(
             new PlacesBookmarkTags({
               id: entry._id,
               itemType: entry.type,
               url,
               guid: entry.guid,
               parentGuid: entry.parentGuid,
+              tags,
               lastModified: entry.lastModified,
               source: item.source,
               isTagging: false,
             })
           );
         }
       }
 
@@ -911,23 +914,27 @@ var Bookmarks = Object.freeze({
 
             // If we're updating a tag, we must notify all the tagged bookmarks
             // about the change.
             if (isTagging) {
               for (let entry of await fetchBookmarksByTags(
                 { tags: [updatedItem.title] },
                 { concurrent: true }
               )) {
+                const tags = PlacesUtils.tagging.getTagsForURI(
+                  NetUtil.newURI(entry.url)
+                );
                 notifications.push(
                   new PlacesBookmarkTags({
                     id: entry._id,
                     itemType: entry.type,
                     url: entry.url,
                     guid: entry.guid,
                     parentGuid: entry.parentGuid,
+                    tags,
                     lastModified: entry.lastModified,
                     source: updatedItem.source,
                     isTagging: false,
                   })
                 );
               }
             }
           }
@@ -1327,26 +1334,28 @@ var Bookmarks = Object.freeze({
             parentGuid: item.parentGuid,
             source: options.source,
             isTagging: isUntagging,
             isDescendantRemoval: false,
           })
         );
 
         if (isUntagging) {
+          const tags = PlacesUtils.tagging.getTagsForURI(NetUtil.newURI(url));
           for (let entry of await fetchBookmarksByURL(item, {
             concurrent: true,
           })) {
             notifications.push(
               new PlacesBookmarkTags({
                 id: entry._id,
                 itemType: entry.type,
                 url,
                 guid: entry.guid,
                 parentGuid: entry.parentGuid,
+                tags,
                 lastModified: entry.lastModified,
                 source: options.source,
                 isTagging: false,
               })
             );
           }
         }
       }
@@ -3271,24 +3280,26 @@ var removeFoldersContents = async functi
         isTagging: isUntagging,
         isDescendantRemoval: !PlacesUtils.bookmarks.userContentRoots.includes(
           item.parentGuid
         ),
       })
     );
 
     if (isUntagging) {
+      const tags = PlacesUtils.tagging.getTagsForURI(NetUtil.newURI(url));
       for (let entry of await fetchBookmarksByURL(item, true)) {
         notifications.push(
           new PlacesBookmarkTags({
             id: entry._id,
             itemType: entry.type,
             url,
             guid: entry.guid,
             parentGuid: entry.parentGuid,
+            tags,
             lastModified: entry.lastModified,
             source,
             isTagging: false,
           })
         );
       }
     }
   }
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -5,16 +5,17 @@
 
 #include "nsNavBookmarks.h"
 
 #include "nsNavHistory.h"
 #include "nsPlacesMacros.h"
 #include "Helpers.h"
 
 #include "nsAppDirectoryServiceDefs.h"
+#include "nsITaggingService.h"
 #include "nsNetUtil.h"
 #include "nsUnicharUtils.h"
 #include "nsPrintfCString.h"
 #include "nsQueryObject.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/ProfilerLabels.h"
 #include "mozilla/storage.h"
 #include "mozilla/dom/PlacesBookmarkAddition.h"
@@ -70,16 +71,28 @@ inline int32_t DetermineInitialSyncStatu
 }
 
 // Indicates whether an item has been uploaded to the server and
 // needs a tombstone on deletion.
 inline bool NeedsTombstone(const BookmarkData& aBookmark) {
   return aBookmark.syncStatus == nsINavBookmarksService::SYNC_STATUS_NORMAL;
 }
 
+inline nsresult GetTags(nsIURI* aURI, nsTArray<nsString>& aResult) {
+  nsresult rv;
+  nsCOMPtr<nsITaggingService> taggingService =
+      do_GetService("@mozilla.org/browser/tagging-service;1", &rv);
+
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  return taggingService->GetTagsForURI(aURI, aResult);
+}
+
 }  // namespace
 
 nsNavBookmarks::nsNavBookmarks() : mCanNotify(false) {
   NS_ASSERTION(!gBookmarksService,
                "Attempting to create two instances of the service!");
   gBookmarksService = this;
 }
 
@@ -449,25 +462,30 @@ nsNavBookmarks::InsertBookmark(int64_t a
   // bookmark-folder result nodes which contain a bookmark for the new
   // bookmark's url.
   if (grandParentId == tagsRootId) {
     // Notify a tags change to all bookmarks for this URI.
     nsTArray<BookmarkData> bookmarks;
     rv = GetBookmarksForURI(aURI, bookmarks);
     NS_ENSURE_SUCCESS(rv, rv);
 
+    nsTArray<nsString> tags;
+    rv = GetTags(aURI, tags);
+    NS_ENSURE_SUCCESS(rv, rv);
+
     for (uint32_t i = 0; i < bookmarks.Length(); ++i) {
       // Check that bookmarks doesn't include the current tag itemId.
       MOZ_ASSERT(bookmarks[i].id != *aNewBookmarkId);
       RefPtr<PlacesBookmarkTags> tagsChanged = new PlacesBookmarkTags();
       tagsChanged->mId = bookmarks[i].id;
       tagsChanged->mItemType = TYPE_BOOKMARK;
       tagsChanged->mUrl.Assign(NS_ConvertUTF8toUTF16(utf8spec));
       tagsChanged->mGuid = bookmarks[i].guid;
       tagsChanged->mParentGuid = bookmarks[i].parentGuid;
+      tagsChanged->mTags.Assign(tags);
       tagsChanged->mLastModified = bookmarks[i].lastModified / 1000;
       tagsChanged->mSource = aSource;
       tagsChanged->mIsTagging = false;
       success = !!notifications.AppendElement(tagsChanged.forget(), fallible);
       MOZ_RELEASE_ASSERT(success);
     }
   }
 
@@ -585,23 +603,28 @@ nsNavBookmarks::RemoveItem(int64_t aItem
     // change to all bookmarks for this URI.
     nsTArray<BookmarkData> bookmarks;
     rv = GetBookmarksForURI(uri, bookmarks);
     NS_ENSURE_SUCCESS(rv, rv);
 
     nsAutoCString utf8spec;
     uri->GetSpec(utf8spec);
 
+    nsTArray<nsString> tags;
+    rv = GetTags(uri, tags);
+    NS_ENSURE_SUCCESS(rv, rv);
+
     for (uint32_t i = 0; i < bookmarks.Length(); ++i) {
       RefPtr<PlacesBookmarkTags> tagsChanged = new PlacesBookmarkTags();
       tagsChanged->mId = bookmarks[i].id;
       tagsChanged->mItemType = TYPE_BOOKMARK;
       tagsChanged->mUrl.Assign(NS_ConvertUTF8toUTF16(utf8spec));
       tagsChanged->mGuid = bookmarks[i].guid;
       tagsChanged->mParentGuid = bookmarks[i].parentGuid;
+      tagsChanged->mTags.Assign(tags);
       tagsChanged->mLastModified = bookmarks[i].lastModified / 1000;
       tagsChanged->mSource = aSource;
       tagsChanged->mIsTagging = false;
       success = !!notifications.AppendElement(tagsChanged.forget(), fallible);
       MOZ_RELEASE_ASSERT(success);
     }
   }
 
@@ -899,23 +922,28 @@ nsresult nsNavBookmarks::RemoveFolderChi
       // bookmark's url.
       nsTArray<BookmarkData> bookmarks;
       rv = GetBookmarksForURI(uri, bookmarks);
       NS_ENSURE_SUCCESS(rv, rv);
 
       nsAutoCString utf8spec;
       uri->GetSpec(utf8spec);
 
+      nsTArray<nsString> tags;
+      rv = GetTags(uri, tags);
+      NS_ENSURE_SUCCESS(rv, rv);
+
       for (uint32_t i = 0; i < bookmarks.Length(); ++i) {
         RefPtr<PlacesBookmarkTags> tagsChanged = new PlacesBookmarkTags();
         tagsChanged->mId = bookmarks[i].id;
         tagsChanged->mItemType = TYPE_BOOKMARK;
         tagsChanged->mUrl.Assign(NS_ConvertUTF8toUTF16(utf8spec));
         tagsChanged->mGuid = bookmarks[i].guid;
         tagsChanged->mParentGuid = bookmarks[i].parentGuid;
+        tagsChanged->mTags.Assign(tags);
         tagsChanged->mLastModified = bookmarks[i].lastModified / 1000;
         tagsChanged->mSource = aSource;
         tagsChanged->mIsTagging = false;
         success = !!notifications.AppendElement(tagsChanged.forget(), fallible);
         MOZ_RELEASE_ASSERT(success);
       }
     }
   }
--- a/toolkit/components/places/tests/bookmarks/head_bookmarks.js
+++ b/toolkit/components/places/tests/bookmarks/head_bookmarks.js
@@ -126,16 +126,17 @@ function expectPlacesObserverNotificatio
         case "bookmark-tags-changed":
           notifications.push({
             type: event.type,
             id: event.id,
             itemType: event.itemType,
             url: event.url,
             guid: event.guid,
             parentGuid: event.parentGuid,
+            tags: event.tags,
             lastModified: new Date(event.lastModified),
             source: event.source,
             isTagging: event.isTagging,
           });
           break;
         case "bookmark-time-changed":
           notifications.push({
             type: event.type,
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
@@ -185,16 +185,17 @@ add_task(async function insert_bookmark_
     },
     {
       type: "bookmark-tags-changed",
       id: itemId,
       itemType: bm.type,
       url: bm.url,
       guid: bm.guid,
       parentGuid: bm.parentGuid,
+      tags: ["tag"],
       lastModified: bm.lastModified,
       source: Ci.nsINavBookmarksService.SOURCE_DEFAULT,
       isTagging: false,
     },
   ]);
 });
 
 add_task(async function update_bookmark_lastModified() {
@@ -573,16 +574,17 @@ add_task(async function remove_bookmark_
     },
     {
       type: "bookmark-tags-changed",
       id: itemId,
       itemType: bm.type,
       url: bm.url,
       guid: bm.guid,
       parentGuid: bm.parentGuid,
+      tags: [],
       lastModified: bm.lastModified,
       source: Ci.nsINavBookmarksService.SOURCE_DEFAULT,
       isTagging: false,
     },
   ]);
 });
 
 add_task(async function remove_folder_notification() {
@@ -663,16 +665,104 @@ add_task(async function remove_folder_no
       parentGuid: folder1.parentGuid,
       source: Ci.nsINavBookmarksService.SOURCE_DEFAULT,
       itemType: PlacesUtils.bookmarks.TYPE_FOLDER,
       isTagging: false,
     },
   ]);
 });
 
+add_task(async function multiple_tags() {
+  const BOOKMARK_URL = "http://multipletags.example.com/";
+  const TAG_NAMES = ["tag1", "tag2", "tag3", "tag4", "tag5", "tag6"];
+
+  const bm = await PlacesUtils.bookmarks.insert({
+    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    url: new URL(BOOKMARK_URL),
+  });
+  const itemId = await PlacesUtils.promiseItemId(bm.guid);
+
+  info("Register all tags");
+  const tagFolders = await Promise.all(
+    TAG_NAMES.map(tagName =>
+      PlacesUtils.bookmarks.insert({
+        type: PlacesUtils.bookmarks.TYPE_FOLDER,
+        parentGuid: PlacesUtils.bookmarks.tagsGuid,
+        title: tagName,
+      })
+    )
+  );
+
+  info("Test adding tags to bookmark");
+  for (let i = 0; i < tagFolders.length; i++) {
+    const tagFolder = tagFolders[i];
+    const expectedTagNames = TAG_NAMES.slice(0, i + 1);
+
+    const observer = expectPlacesObserverNotifications([
+      "bookmark-tags-changed",
+    ]);
+
+    await PlacesUtils.bookmarks.insert({
+      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+      parentGuid: tagFolder.guid,
+      url: new URL(BOOKMARK_URL),
+    });
+
+    observer.check([
+      {
+        type: "bookmark-tags-changed",
+        id: itemId,
+        itemType: bm.type,
+        url: bm.url,
+        guid: bm.guid,
+        parentGuid: bm.parentGuid,
+        tags: expectedTagNames,
+        lastModified: bm.lastModified,
+        source: Ci.nsINavBookmarksService.SOURCE_DEFAULT,
+        isTagging: false,
+      },
+    ]);
+  }
+
+  info("Test removing tags from bookmark");
+  for (const removedLength of [1, 2, 3]) {
+    const removedTags = tagFolders.splice(0, removedLength);
+
+    const observer = expectPlacesObserverNotifications([
+      "bookmark-tags-changed",
+    ]);
+
+    // We can remove multiple tags at one time.
+    await PlacesUtils.bookmarks.remove(removedTags);
+
+    const expectedResults = [];
+
+    for (let i = 0; i < removedLength; i++) {
+      TAG_NAMES.splice(0, 1);
+      const expectedTagNames = [...TAG_NAMES];
+
+      expectedResults.push({
+        type: "bookmark-tags-changed",
+        id: itemId,
+        itemType: bm.type,
+        url: bm.url,
+        guid: bm.guid,
+        parentGuid: bm.parentGuid,
+        tags: expectedTagNames,
+        lastModified: bm.lastModified,
+        source: Ci.nsINavBookmarksService.SOURCE_DEFAULT,
+        isTagging: false,
+      });
+    }
+
+    observer.check(expectedResults);
+  }
+});
+
 add_task(async function eraseEverything_notification() {
   // Let's start from a clean situation.
   await PlacesUtils.bookmarks.eraseEverything();
 
   let folder1 = await PlacesUtils.bookmarks.insert({
     type: PlacesUtils.bookmarks.TYPE_FOLDER,
     parentGuid: PlacesUtils.bookmarks.unfiledGuid,
   });