Bug 1150678 - Part 2: allow to change uri for a single bookmark keyword and remove the old keyword when needed. r=ttaubert

--HG--
extra : commitid : 1nWe1X4t8xb

browser/components/places/content/editBookmarkOverlay.js

@@ -130,7 +130,7 @@ let gEditItemOverlay = {
     let newKeyword = aNewKeyword;
     if (newKeyword === undefined) {
       let itemId = this._paneInfo.itemId;
-      newKeyword = PlacesUtils.bookmarks.getKeywordForBookmark(itemId);
+      this._keyword = newKeyword = PlacesUtils.bookmarks.getKeywordForBookmark(itemId);
     }
     this._initTextField(this._keywordField, newKeyword);
   }),
@@ -597,12 +597,16 @@ let gEditItemOverlay = {
     let itemId = this._paneInfo.itemId;
     let newKeyword = this._keywordField.value;
     if (!PlacesUIUtils.useAsyncTransactions) {
-      let txn = new PlacesEditBookmarkKeywordTransaction(itemId, newKeyword, this._paneInfo.postData);
+      let txn = new PlacesEditBookmarkKeywordTransaction(itemId, newKeyword,
+                                                         this._paneInfo.postData,
+                                                         this._keyword);
       PlacesUtils.transactionManager.doTransaction(txn);
       return;
     }
     let guid = this._paneInfo.itemGuid;
-    PlacesTransactions.EditKeyword({ guid, keyword: newKeyword })
+    PlacesTransactions.EditKeyword({ guid, keyword: newKeyword,
+                                     postData: this._paneInfo.postData,
+                                     oldKeyword: this._keyword })
                       .transact().catch(Components.utils.reportError);
   },
 

toolkit/components/places/PlacesTransactions.jsm

@@ -883,7 +883,7 @@ DefineTransaction.defineInputProps(["guid", "parentGuid", "newParentGuid"],
                                    DefineTransaction.guidValidate);
 DefineTransaction.defineInputProps(["title"],
                                    DefineTransaction.strOrNullValidate, null);
-DefineTransaction.defineInputProps(["keyword", "postData", "tag",
+DefineTransaction.defineInputProps(["keyword", "oldKeyword", "postData", "tag",
                                     "excludingAnnotation"],
                                    DefineTransaction.strValidate, "");
 DefineTransaction.defineInputProps(["index", "newIndex"],
@@ -1335,14 +1335,30 @@ PT.Annotate.prototype = {
  *
  * Required Input Properties: guid, keyword.
  */
-PT.EditKeyword = DefineTransaction(["guid", "keyword"]);
+PT.EditKeyword = DefineTransaction(["guid", "keyword"],
+                                   ["postData", "oldKeyword"]);
 PT.EditKeyword.prototype = Object.seal({
-  execute: function* (aGuid, aKeyword) {
-    let itemId = yield PlacesUtils.promiseItemId(aGuid),
-        oldKeyword = PlacesUtils.bookmarks.getKeywordForBookmark(itemId);
-    PlacesUtils.bookmarks.setKeywordForBookmark(itemId, aKeyword);
-    this.undo = () => {
-      PlacesUtils.bookmarks.setKeywordForBookmark(itemId, oldKeyword);
+  execute: function* (aGuid, aKeyword, aPostData, aOldKeyword) {
+    let url;
+    let oldEntry;
+    if (aOldKeyword) {
+      oldEntry = yield PlacesUtils.keywords.fetch(aOldKeyword);
+      url = oldEntry.url;
+      yield PlacesUtils.keywords.remove(aOldKeyword);
+    }
+
+    if (!url)
+      url = (yield PlacesUtils.bookmarks.fetch(aGuid)).url;
+    yield PlacesUtils.keywords.insert({
+      url: url,
+      keyword: aKeyword,
+      postData: aPostData || (oldEntry ? oldEntry.postData : "")
+    });
+
+    this.undo = function* () {
+      yield PlacesUtils.keywords.remove(aKeyword);
+      if (oldEntry)
+        yield PlacesUtils.keywords.insert(oldEntry);
     };
   }
 });

toolkit/components/places/PlacesUtils.jsm

@@ -1864,7 +1864,8 @@ this.PlacesUtils = {
     }.bind(this);
 
     const QUERY_STR =
-      `WITH RECURSIVE
+      `/* do not warn (bug no): cannot use an index */
+       WITH RECURSIVE
        descendants(fk, level, type, id, guid, parent, parentGuid, position,
                    title, dateAdded, lastModified) AS (
          SELECT b1.fk, 0, b1.type, b1.id, b1.guid, b1.parent,
@@ -2349,29 +2350,49 @@ XPCOMUtils.defineLazyGetter(this, "gKeywordsCachePromise", () =>
           }).catch(Cu.reportError);
         },
 
-        onItemChanged(id, prop, isAnno, val, lastMod, itemType, parentId, guid) {
-          if (gIgnoreKeywordNotifications ||
-              prop != "keyword")
+        onItemChanged(id, prop, isAnno, val, lastMod, itemType, parentId, guid,
+                      parentGuid, oldVal) {
+          if (gIgnoreKeywordNotifications)
+            return;
+          if (prop == "keyword") {
+            this._onKeywordChanged(guid, val).catch(Cu.reportError);
+          } else if (prop == "uri") {
+            this._onUrlChanged(guid, val, oldVal).catch(Cu.reportError);
+          }
+        },
+
+        _onKeywordChanged: Task.async(function* (guid, keyword) {
+          let bookmark = yield PlacesUtils.bookmarks.fetch(guid);
+          // By this time the bookmark could be gone, there's nothing we can do.
+          if (!bookmark)
             return;
 
-          Task.spawn(function* () {
-            let bookmark = yield PlacesUtils.bookmarks.fetch(guid);
-            // By this time the bookmark could have gone, there's nothing we can do.
-            if (!bookmark)
-              return;
-
-            if (val.length == 0) {
-              // We are removing a keyword.
-              let keywords = keywordsForHref(bookmark.url.href)
-              for (let keyword of keywords) {
-                cache.delete(keyword);
-              }
-            } else {
-              // We are adding a new keyword.
-              cache.set(val, { keyword: val, url: bookmark.url });
+          if (keyword.length == 0) {
+            // We are removing a keyword.
+            let keywords = keywordsForHref(bookmark.url.href)
+            for (let kw of keywords) {
+              cache.delete(kw);
             }
-          }).catch(Cu.reportError);
-        }
+          } else {
+            // We are adding a new keyword.
+            cache.set(keyword, { keyword, url: bookmark.url });
+          }
+        }),
+
+        _onUrlChanged: Task.async(function* (guid, url, oldUrl) {
+          // Check if the old url is associated with keywords.
+          let entries = [];
+          yield PlacesUtils.keywords.fetch({ url: oldUrl }, e => entries.push(e));
+          if (entries.length == 0)
+            return;
+
+          // Move the keywords to the new url.
+          for (let entry of entries) {
+            yield PlacesUtils.keywords.remove(entry.keyword);
+            entry.url = new URL(url);
+            yield PlacesUtils.keywords.insert(entry);
+          }
+        }),
       };
 
       PlacesUtils.bookmarks.addObserver(observer, false);
@@ -3388,14 +3409,18 @@ PlacesSetPageAnnotationTransaction.prototype = {
  *        new keyword for the bookmark
  * @param aNewPostData [optional]
  *        new keyword's POST data, if available
+ * @param aOldKeyword [optional]
+ *        old keyword of the bookmark
  *
  * @return nsITransaction object
  */
 this.PlacesEditBookmarkKeywordTransaction =
- function PlacesEditBookmarkKeywordTransaction(aItemId, aNewKeyword, aNewPostData)
-{
+  function PlacesEditBookmarkKeywordTransaction(aItemId, aNewKeyword,
+                                                aNewPostData, aOldKeyword) {
   this.item = new TransactionItemCache();
   this.item.id = aItemId;
+  this.item.keyword = aOldKeyword;
+  this.item.href = (PlacesUtils.bookmarks.getBookmarkURI(aItemId)).spec;
   this.new = new TransactionItemCache();
   this.new.keyword = aNewKeyword;
   this.new.postData = aNewPostData
@@ -3406,22 +3431,51 @@ PlacesEditBookmarkKeywordTransaction.prototype = {
 
   doTransaction: function EBKTXN_doTransaction()
   {
-    // Store the current values.
-    this.item.keyword = PlacesUtils.bookmarks.getKeywordForBookmark(this.item.id);
-    if (this.item.keyword)
-      this.item.postData = PlacesUtils.getPostDataForBookmark(this.item.id);
+    let done = false;
+    Task.spawn(function* () {
+      if (this.item.keyword) {
+        let oldEntry = yield PlacesUtils.keywords.fetch(this.item.keyword);
+        this.item.postData = oldEntry.postData;
+        yield PlacesUtils.keywords.remove(this.item.keyword);
+      }
 
-    // Update the keyword.
-    PlacesUtils.bookmarks.setKeywordForBookmark(this.item.id, this.new.keyword);
-    if (this.new.keyword && this.new.postData)
-      PlacesUtils.setPostDataForBookmark(this.item.id, this.new.postData);
+      yield PlacesUtils.keywords.insert({
+        url: this.item.href,
+        keyword: this.new.keyword,
+        postData: this.new.postData || this.item.postData
+      });
+    }.bind(this)).catch(Cu.reportError)
+                 .then(() => done = true);
+    // TODO: This hack is needed until we can use PlacesTransactions.jsm.
+    let thread = Services.tm.currentThread;
+    while (!done) {
+      thread.processNextEvent(true);
+    }
   },
 
   undoTransaction: function EBKTXN_undoTransaction()
   {
-    PlacesUtils.bookmarks.setKeywordForBookmark(this.item.id, this.item.keyword);
-    if (this.item.postData)
-      PlacesUtils.setPostDataForBookmark(this.item.id, this.item.postData);
+
+    let done = false;
+    Task.spawn(function* () {
+      if (this.new.keyword) {
+        yield PlacesUtils.keywords.remove(this.new.keyword);
+      }
+
+      if (this.item.keyword) {
+        yield PlacesUtils.keywords.insert({
+          url: this.item.href,
+          keyword: this.item.keyword,
+          postData: this.item.postData
+        });
+      }
+    }.bind(this)).catch(Cu.reportError)
+                 .then(() => done = true);
+    // TODO: This hack is needed until we can use PlacesTransactions.jsm.
+    let thread = Services.tm.currentThread;
+    while (!done) {
+      thread.processNextEvent(true);
+    }
   }
 };
 

toolkit/components/places/tests/unit/test_async_transactions.js

@@ -1021,16 +1021,24 @@ add_task(function* test_edit_keyword() {
   bm_info.guid = yield PT.NewBookmark(bm_info).transact();
 
   observer.reset();
-  yield PT.EditKeyword({ guid: bm_info.guid, keyword: KEYWORD }).transact();
+  yield PT.EditKeyword({ guid: bm_info.guid, keyword: KEYWORD, postData: "postData" }).transact();
   ensureKeywordChange(KEYWORD);
+  let entry = yield PlacesUtils.keywords.fetch(KEYWORD);
+  Assert.equal(entry.url.href, bm_info.url.spec);
+  Assert.equal(entry.postData, "postData");
 
   observer.reset();
   yield PT.undo();
   ensureKeywordChange();
+  entry = yield PlacesUtils.keywords.fetch(KEYWORD);
+  Assert.equal(entry, null);
 
   observer.reset();
   yield PT.redo();
   ensureKeywordChange(KEYWORD);
+  entry = yield PlacesUtils.keywords.fetch(KEYWORD);
+  Assert.equal(entry.url.href, bm_info.url.spec);
+  Assert.equal(entry.postData, "postData");
 
   // Cleanup
   observer.reset();
@@ -1043,6 +1051,68 @@ add_task(function* test_edit_keyword() {
   ensureUndoState();
 });
 
+add_task(function* test_edit_specific_keyword() {
+  let bm_info = { parentGuid: rootGuid
+                , url: NetUtil.newURI("http://test.edit.keyword") };
+  bm_info.guid = yield PT.NewBookmark(bm_info).transact();
+  function ensureKeywordChange(aCurrentKeyword = "", aPreviousKeyword = "") {
+    ensureItemsChanged({ guid: bm_info.guid
+                       , property: "keyword"
+                       , newValue: aCurrentKeyword
+                       });
+  }
+
+  yield PlacesUtils.keywords.insert({ keyword: "kw1", url: bm_info.url.spec, postData: "postData1" });
+  yield PlacesUtils.keywords.insert({ keyword: "kw2", url: bm_info.url.spec, postData: "postData2" });
+  bm_info.guid = yield PT.NewBookmark(bm_info).transact();
+
+  observer.reset();
+  yield PT.EditKeyword({ guid: bm_info.guid, keyword: "keyword", oldKeyword: "kw2" }).transact();
+  ensureKeywordChange("keyword", "kw2");
+  let entry = yield PlacesUtils.keywords.fetch("kw1");
+  Assert.equal(entry.url.href, bm_info.url.spec);
+  Assert.equal(entry.postData, "postData1");
+  entry = yield PlacesUtils.keywords.fetch("keyword");
+  Assert.equal(entry.url.href, bm_info.url.spec);
+  Assert.equal(entry.postData, "postData2");
+  entry = yield PlacesUtils.keywords.fetch("kw2");
+  Assert.equal(entry, null);
+
+  observer.reset();
+  yield PT.undo();
+  ensureKeywordChange("kw2", "keyword");
+  entry = yield PlacesUtils.keywords.fetch("kw1");
+  Assert.equal(entry.url.href, bm_info.url.spec);
+  Assert.equal(entry.postData, "postData1");
+  entry = yield PlacesUtils.keywords.fetch("kw2");
+  Assert.equal(entry.url.href, bm_info.url.spec);
+  Assert.equal(entry.postData, "postData2");
+  entry = yield PlacesUtils.keywords.fetch("keyword");
+  Assert.equal(entry, null);
+
+  observer.reset();
+  yield PT.redo();
+  ensureKeywordChange("keyword", "kw2");
+  entry = yield PlacesUtils.keywords.fetch("kw1");
+  Assert.equal(entry.url.href, bm_info.url.spec);
+  Assert.equal(entry.postData, "postData1");
+  entry = yield PlacesUtils.keywords.fetch("keyword");
+  Assert.equal(entry.url.href, bm_info.url.spec);
+  Assert.equal(entry.postData, "postData2");
+  entry = yield PlacesUtils.keywords.fetch("kw2");
+  Assert.equal(entry, null);
+
+  // Cleanup
+  observer.reset();
+  yield PT.undo();
+  ensureKeywordChange("kw2");
+  yield PT.undo();
+  ensureItemsRemoved(bm_info);
+
+  yield PT.clearTransactionsHistory();
+  ensureUndoState();
+});
+
 add_task(function* test_tag_uri() {
   // This also tests passing uri specs.
   let bm_info_a = { url: "http://bookmarked.uri"

toolkit/components/places/tests/unit/test_keywords.js

@@ -513,6 +513,24 @@ add_task(function* test_oldKeywordsAPI() {
   check_no_orphans();
 });
 
-function run_test() {
-  run_next_test();
-}
+add_task(function* test_bookmarkURLChange() {
+  let fc1 = yield foreign_count("http://example1.com/");
+  let fc2 = yield foreign_count("http://example2.com/");
+  let bookmark = yield PlacesUtils.bookmarks.insert({ url: "http://example1.com/",
+                                                      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+                                                      parentGuid: PlacesUtils.bookmarks.unfiledGuid });
+  yield PlacesUtils.keywords.insert({ keyword: "keyword",
+                                      url: "http://example1.com/" });
+
+  yield check_keyword(true, "http://example1.com/", "keyword");
+  Assert.equal((yield foreign_count("http://example1.com/")), fc1 + 2); // +1 bookmark +1 keyword
+
+  yield PlacesUtils.bookmarks.update({ guid: bookmark.guid,
+                                       url: "http://example2.com/"});
+  yield promiseKeyword("keyword", "http://example2.com/");
+
+  yield check_keyword(false, "http://example1.com/", "keyword");
+  yield check_keyword(true, "http://example2.com/", "keyword");
+  Assert.equal((yield foreign_count("http://example1.com/")), fc1); // -1 bookmark -1 keyword
+  Assert.equal((yield foreign_count("http://example2.com/")), fc2 + 2); // +1 bookmark +1 keyword
+});

toolkit/components/places/tests/unit/test_placesTxn.js

@@ -555,6 +555,40 @@ add_task(function* test_edit_keyword() {
   do_check_eq(PlacesUtils.getPostDataForBookmark(testBkmId), null);
 });
 
+add_task(function* test_edit_specific_keyword() {
+  const KEYWORD = "keyword-test_edit_keyword2";
+
+  let testURI = NetUtil.newURI("http://test_edit_keyword2.com");
+  let testBkmId = bmsvc.insertBookmark(root, testURI, bmsvc.DEFAULT_INDEX, "Test edit keyword");
+  // Add multiple keyword to this uri.
+  yield PlacesUtils.keywords.insert({ keyword: "kw1", url: testURI.spec, postData: "postData1" });
+  yield PlacesUtils.keywords.insert({keyword: "kw2", url: testURI.spec, postData: "postData2" });
+
+  // Try to change only kw2.
+  let txn = new PlacesEditBookmarkKeywordTransaction(testBkmId, KEYWORD, "postData2", "kw2");
+
+  txn.doTransaction();
+  do_check_eq(observer._itemChangedId, testBkmId);
+  do_check_eq(observer._itemChangedProperty, "keyword");
+  do_check_eq(observer._itemChangedValue, KEYWORD);
+  let entry = yield PlacesUtils.keywords.fetch("kw1");
+  Assert.equal(entry.url.href, testURI.spec);
+  Assert.equal(entry.postData, "postData1");
+  yield promiseKeyword(KEYWORD, testURI.spec, "postData2");
+  yield promiseKeyword("kw2", null);
+
+  txn.undoTransaction();
+  do_check_eq(observer._itemChangedId, testBkmId);
+  do_check_eq(observer._itemChangedProperty, "keyword");
+  do_check_eq(observer._itemChangedValue, "kw2");
+  do_check_eq(PlacesUtils.getPostDataForBookmark(testBkmId), "postData1");
+  entry = yield PlacesUtils.keywords.fetch("kw1");
+  Assert.equal(entry.url.href, testURI.spec);
+  Assert.equal(entry.postData, "postData1");
+  yield promiseKeyword("kw2", testURI.spec, "postData2");
+  yield promiseKeyword("keyword", null);
+});
+
 add_task(function* test_LoadInSidebar_transaction() {
   let testURI = NetUtil.newURI("http://test_LoadInSidebar_transaction.com");
   let testBkmId = bmsvc.insertBookmark(root, testURI, bmsvc.DEFAULT_INDEX, "Test LoadInSidebar transaction");