Bug 1062894 - Avoid batching tag changes if number of changed tags is low. r=mak, ttaubert
authorMohamed Waleed <mohamedwaleed2012@gmail.com>
Mon, 27 Apr 2015 21:24:21 +0200
changeset 242961 9a5bd1aa9bcef8d603de4aa684f34f0801a68e5f
parent 242960 c18720d4d55926541d62c6a2b4b5f69a3ab3aeaa
child 242962 0070705f844296d7d79a7dc0ae544dd44d100bab
push id28714
push userkwierso@gmail.com
push dateFri, 08 May 2015 17:29:48 +0000
treeherdermozilla-central@5e8adf0e7f2c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, ttaubert
bugs1062894
milestone40.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 1062894 - Avoid batching tag changes if number of changed tags is low. r=mak, ttaubert
toolkit/components/places/nsINavHistoryService.idl
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/nsNavHistoryResult.h
toolkit/components/places/nsTaggingService.js
toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js
toolkit/components/places/tests/queries/test_history_queries_tags_liveUpdate.js
--- a/toolkit/components/places/nsINavHistoryService.idl
+++ b/toolkit/components/places/nsINavHistoryService.idl
@@ -1410,12 +1410,12 @@ interface nsINavHistoryService : nsISupp
    * Clear all TRANSITION_EMBED visits.
    */
   void clearEmbedVisits();
 };
 
 /**
  * @see runInBatchMode of nsINavHistoryService/nsINavBookmarksService
  */
-[scriptable, uuid(5143f2bb-be0a-4faf-9acb-b0ed3f82952c)]
+[scriptable, function, uuid(5a5a9154-95ac-4e3d-90df-558816297407)]
 interface nsINavHistoryBatchCallback : nsISupports {
   void runBatched(in nsISupports aUserData);
 };
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -4181,20 +4181,23 @@ nsNavHistory::URIToResultNode(nsIURI* aU
                               nsNavHistoryQueryOptions* aOptions,
                               nsNavHistoryResultNode** aResult)
 {
   nsAutoCString tagsFragment;
   GetTagsSqlFragment(GetTagsFolder(), NS_LITERAL_CSTRING("h.id"),
                      true, tagsFragment);
   // Should match kGetInfoIndex_*
   nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(NS_LITERAL_CSTRING(
-    "SELECT h.id, :page_url, h.title, h.rev_host, h.visit_count, "
-           "h.last_visit_date, f.url, null, null, null, null, "
-           ) + tagsFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid "
+    "SELECT h.id, :page_url, COALESCE(b.title, h.title), "
+           "h.rev_host, h.visit_count, h.last_visit_date, f.url, "
+           "b.id, b.dateAdded, b.lastModified, b.parent, "
+           ) + tagsFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid, "
+           "b.guid, b.position, b.type, b.fk "
     "FROM moz_places h "
+    "LEFT JOIN moz_bookmarks b ON b.fk = h.id "
     "LEFT JOIN moz_favicons f ON h.favicon_id = f.id "
     "WHERE h.url = :page_url ")
   );
   NS_ENSURE_STATE(stmt);
   mozStorageStatementScoper scoper(stmt);
 
   nsresult rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), aURI);
   NS_ENSURE_SUCCESS(rv, rv);
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -2762,17 +2762,17 @@ nsNavHistoryQueryResultNode::NotifyIfTag
 
   // Find matching URI nodes.
   nsRefPtr<nsNavHistoryResultNode> node;
   nsNavHistory* history = nsNavHistory::GetHistoryService();
 
   nsCOMArray<nsNavHistoryResultNode> matches;
   RecursiveFindURIs(onlyOneEntry, this, spec, &matches);
 
-  if (matches.Count() == 0 && mHasSearchTerms && !mRemovingURI) {
+  if (matches.Count() == 0 && mHasSearchTerms) {
     // A new tag has been added, it's possible it matches our query.
     NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
     rv = history->URIToResultNode(aURI, mOptions, getter_AddRefs(node));
     NS_ENSURE_SUCCESS(rv, rv);
     if (history->EvaluateQueryForNode(mQueries, mOptions, node)) {
       rv = InsertSortedChild(node);
       NS_ENSURE_SUCCESS(rv, rv);
     }
@@ -2833,17 +2833,16 @@ NS_IMETHODIMP
 nsNavHistoryQueryResultNode::OnItemRemoved(int64_t aItemId,
                                            int64_t aParentId,
                                            int32_t aIndex,
                                            uint16_t aItemType,
                                            nsIURI* aURI,
                                            const nsACString& aGUID,
                                            const nsACString& aParentGUID)
 {
-  mRemovingURI = aURI;
   if (aItemType == nsINavBookmarksService::TYPE_BOOKMARK &&
       mLiveUpdate != QUERYUPDATE_SIMPLE && mLiveUpdate != QUERYUPDATE_TIME) {
     nsresult rv = Refresh();
     NS_ENSURE_SUCCESS(rv, rv);
   }
   return NS_OK;
 }
 
--- a/toolkit/components/places/nsNavHistoryResult.h
+++ b/toolkit/components/places/nsNavHistoryResult.h
@@ -669,17 +669,16 @@ public:
   void ClearChildren(bool unregister);
   nsresult Refresh() override;
 
   virtual uint16_t GetSortType() override;
   virtual void GetSortingAnnotation(nsACString& aSortingAnnotation) override;
   virtual void RecursiveSort(const char* aData,
                              SortComparator aComparator) override;
 
-  nsCOMPtr<nsIURI> mRemovingURI;
   nsresult NotifyIfTagsChanged(nsIURI* aURI);
 
   uint32_t mBatchChanges;
 
   // Tracks transition type filters shared by all mQueries.
   nsTArray<uint32_t> mTransitions;
 
 protected:
--- a/toolkit/components/places/nsTaggingService.js
+++ b/toolkit/components/places/nsTaggingService.js
@@ -102,81 +102,84 @@ TaggingService.prototype = {
    *        Array of tags.  Entries can be tag names or concrete item id.
    * @param trim [optional]
    *        Whether to trim passed-in named tags. Defaults to false.
    * @return Array of tag objects like { id: number, name: string }.
    *
    * @throws Cr.NS_ERROR_INVALID_ARG if any element of the input array is not
    *         a valid tag.
    */
-  _convertInputMixedTagsArray: function TS__convertInputMixedTagsArray(aTags, trim=false)
-  {
-    return aTags.map(function (val)
-    {
-      let tag = { _self: this };
-      if (typeof(val) == "number" && this._tagFolders[val]) {
+  _convertInputMixedTagsArray(aTags, trim=false) {
+    // Handle sparse array with a .filter.
+    return aTags.filter(tag => tag !== undefined)
+                .map(idOrName => {
+      let tag = {};
+      if (typeof(idOrName) == "number" && this._tagFolders[idOrName]) {
         // This is a tag folder id.
-        tag.id = val;
+        tag.id = idOrName;
         // We can't know the name at this point, since a previous tag could
         // want to change it.
-        tag.__defineGetter__("name", function () this._self._tagFolders[this.id]);
+        tag.__defineGetter__("name", () => this._tagFolders[tag.id]);
       }
-      else if (typeof(val) == "string" && val.length > 0 && val.length <= Ci.nsITaggingService.MAX_TAG_LENGTH) {
+      else if (typeof(idOrName) == "string" && idOrName.length > 0 &&
+               idOrName.length <= Ci.nsITaggingService.MAX_TAG_LENGTH) {
         // This is a tag name.
-        tag.name = trim ? val.trim() : val;
+        tag.name = trim ? idOrName.trim() : idOrName;
         // We can't know the id at this point, since a previous tag could
         // have created it.
-        tag.__defineGetter__("id", function () this._self._getItemIdForTag(this.name));
+        tag.__defineGetter__("id", () => this._getItemIdForTag(tag.name));
       }
       else {
         throw Cr.NS_ERROR_INVALID_ARG;
       }
       return tag;
-    }, this);
+    });
   },
 
   // nsITaggingService
   tagURI: function TS_tagURI(aURI, aTags)
   {
     if (!aURI || !aTags || !Array.isArray(aTags)) {
       throw Cr.NS_ERROR_INVALID_ARG;
     }
 
     // This also does some input validation.
     let tags = this._convertInputMixedTagsArray(aTags, true);
 
-    let taggingService = this;
-    PlacesUtils.bookmarks.runInBatchMode({
-      runBatched: function (aUserData)
-      {
-        tags.forEach(function (tag)
-        {
-          if (tag.id == -1) {
-            // Tag does not exist yet, create it.
-            this._createTag(tag.name);
-          }
+    let taggingFunction = () => {
+      for (let tag of tags) {
+        if (tag.id == -1) {
+          // Tag does not exist yet, create it.
+          this._createTag(tag.name);
+        }
+
+        if (this._getItemIdForTaggedURI(aURI, tag.name) == -1) {
+          // The provided URI is not yet tagged, add a tag for it.
+          // Note that bookmarks under tag containers must have null titles.
+          PlacesUtils.bookmarks.insertBookmark(
+            tag.id, aURI, PlacesUtils.bookmarks.DEFAULT_INDEX, null
+          );
+        }
 
-          if (this._getItemIdForTaggedURI(aURI, tag.name) == -1) {
-            // The provided URI is not yet tagged, add a tag for it.
-            // Note that bookmarks under tag containers must have null titles.
-            PlacesUtils.bookmarks.insertBookmark(
-              tag.id, aURI, PlacesUtils.bookmarks.DEFAULT_INDEX, null
-            );
-          }
+        // Try to preserve user's tag name casing.
+        // Rename the tag container so the Places view matches the most-recent
+        // user-typed value.
+        if (PlacesUtils.bookmarks.getItemTitle(tag.id) != tag.name) {
+          // this._tagFolders is updated by the bookmarks observer.
+          PlacesUtils.bookmarks.setItemTitle(tag.id, tag.name);
+        }
+      }
+    };
 
-          // Try to preserve user's tag name casing.
-          // Rename the tag container so the Places view matches the most-recent
-          // user-typed value.
-          if (PlacesUtils.bookmarks.getItemTitle(tag.id) != tag.name) {
-            // this._tagFolders is updated by the bookmarks observer.
-            PlacesUtils.bookmarks.setItemTitle(tag.id, tag.name);
-          }
-        }, taggingService);
-      }
-    }, null);
+    // Use a batch only if creating more than 2 tags.
+    if (tags.length < 3) {
+      taggingFunction();
+    } else {
+      PlacesUtils.bookmarks.runInBatchMode(taggingFunction, null);
+    }
   },
 
   /**
    * Removes the tag container from the tags root if the given tag is empty.
    *
    * @param aTagId
    *        the itemId of the tag element under the tags root
    */
@@ -220,33 +223,35 @@ TaggingService.prototype = {
     let tags = this._convertInputMixedTagsArray(aTags);
 
     let isAnyTagNotTrimmed = tags.some(tag => /^\s|\s$/.test(tag.name));
     if (isAnyTagNotTrimmed) {
       Deprecated.warning("At least one tag passed to untagURI was not trimmed",
                          "https://bugzilla.mozilla.org/show_bug.cgi?id=967196");
     }
 
-    let taggingService = this;
-    PlacesUtils.bookmarks.runInBatchMode({
-      runBatched: function (aUserData)
-      {
-        tags.forEach(function (tag)
-        {
-          if (tag.id != -1) {
-            // A tag could exist.
-            let itemId = this._getItemIdForTaggedURI(aURI, tag.name);
-            if (itemId != -1) {
-              // There is a tagged item.
-              PlacesUtils.bookmarks.removeItem(itemId);
-            }
+    let untaggingFunction = () => {
+      for (let tag of tags) {
+        if (tag.id != -1) {
+          // A tag could exist.
+          let itemId = this._getItemIdForTaggedURI(aURI, tag.name);
+          if (itemId != -1) {
+            // There is a tagged item.
+            PlacesUtils.bookmarks.removeItem(itemId);
           }
-        }, taggingService);
+        }
       }
-    }, null);
+    };
+
+    // Use a batch only if creating more than 2 tags.
+    if (tags.length < 3) {
+      untaggingFunction();
+    } else {
+      PlacesUtils.bookmarks.runInBatchMode(untaggingFunction, null);
+    }
   },
 
   // nsITaggingService
   getURIsForTag: function TS_getURIsForTag(aTagName) {
     if (!aTagName || aTagName.length == 0)
       throw Cr.NS_ERROR_INVALID_ARG;
 
     if (/^\s|\s$/.test(aTagName)) {
--- a/toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js
+++ b/toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js
@@ -138,18 +138,16 @@ add_test(function onItemChanged_title_bo
 });
 
 add_test(function onItemChanged_tags_bookmark() {
   let id = PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.unfiledBookmarksFolderId, 0);
   let uri = PlacesUtils.bookmarks.getBookmarkURI(id);
   const TITLE = "New title";
   const TAG = "tag"
   gBookmarksObserver.expected = [
-    { name: "onBeginUpdateBatch", // Tag addition uses a batch.
-     args: [] },
     { name: "onItemAdded", // This is the tag folder.
       args: [
         { name: "itemId", check: function (v) typeof(v) == "number" && v > 0 },
         { name: "parentId", check: function (v) v === PlacesUtils.tagsFolderId },
         { name: "index", check: function (v) v === 0 },
         { name: "itemType", check: function (v) v === PlacesUtils.bookmarks.TYPE_FOLDER },
         { name: "uri", check: function (v) v === null },
         { name: "title", check: function (v) v === TAG },
@@ -176,20 +174,16 @@ add_test(function onItemChanged_tags_boo
         { name: "isAnno", check: function (v) v === false },
         { name: "newValue", check: function (v) v === "" },
         { name: "lastModified", check: function (v) typeof(v) == "number" && v > 0 },
         { name: "itemType", check: function (v) v === PlacesUtils.bookmarks.TYPE_BOOKMARK },
         { name: "parentId", check: function (v) v === PlacesUtils.unfiledBookmarksFolderId },
         { name: "guid", check: function (v) typeof(v) == "string" && /^[a-zA-Z0-9\-_]{12}$/.test(v) },
         { name: "parentGuid", check: function (v) typeof(v) == "string" && /^[a-zA-Z0-9\-_]{12}$/.test(v) },
       ] },
-    { name: "onEndUpdateBatch",
-      args: [] },
-    { name: "onBeginUpdateBatch", // Tag removal uses a batch.
-     args: [] },
     { name: "onItemRemoved", // This is the tag.
       args: [
         { name: "itemId", check: function (v) typeof(v) == "number" && v > 0 },
         { name: "parentId", check: function (v) typeof(v) == "number" && v > 0 },
         { name: "index", check: function (v) v === 0 },
         { name: "itemType", check: function (v) v === PlacesUtils.bookmarks.TYPE_BOOKMARK },
         { name: "uri", check: function (v) v instanceof Ci.nsIURI && v.equals(uri) },
         { name: "guid", check: function (v) typeof(v) == "string" && /^[a-zA-Z0-9\-_]{12}$/.test(v) },
@@ -212,18 +206,16 @@ add_test(function onItemChanged_tags_boo
         { name: "isAnno", check: function (v) v === false },
         { name: "newValue", check: function (v) v === "" },
         { name: "lastModified", check: function (v) typeof(v) == "number" && v > 0 },
         { name: "itemType", check: function (v) v === PlacesUtils.bookmarks.TYPE_BOOKMARK },
         { name: "parentId", check: function (v) v === PlacesUtils.unfiledBookmarksFolderId },
         { name: "guid", check: function (v) typeof(v) == "string" && /^[a-zA-Z0-9\-_]{12}$/.test(v) },
         { name: "parentGuid", check: function (v) typeof(v) == "string" && /^[a-zA-Z0-9\-_]{12}$/.test(v) },
       ] },
-    { name: "onEndUpdateBatch",
-      args: [] },
   ];
   PlacesUtils.tagging.tagURI(uri, [TAG]);
   PlacesUtils.tagging.untagURI(uri, [TAG]);
 });
 
 add_test(function onItemMoved_bookmark() {
   let id = PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.unfiledBookmarksFolderId, 0);
   let uri = PlacesUtils.bookmarks.getBookmarkURI(id);
--- a/toolkit/components/places/tests/queries/test_history_queries_tags_liveUpdate.js
+++ b/toolkit/components/places/tests/queries/test_history_queries_tags_liveUpdate.js
@@ -155,16 +155,20 @@ add_task(function visits_searchterm_quer
 add_task(function pages_searchterm_is_tag_query()
 {
   let [query, options] = newQueryWithOptions();
   query.searchTerms = "test-tag";
   testQueryContents(query, options, function (root) {
     compareArrayToResult([], root);
     gTestData.forEach(function (data) {
       let uri = NetUtil.newURI(data.uri);
+      PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
+                                           uri,
+                                           PlacesUtils.bookmarks.DEFAULT_INDEX,
+                                           data.title);
       PlacesUtils.tagging.tagURI(uri, ["test-tag"]);
       compareArrayToResult([data], root);
       PlacesUtils.tagging.untagURI(uri, ["test-tag"]);
       compareArrayToResult([], root);
     });
   });
 });
 
@@ -172,15 +176,19 @@ add_task(function visits_searchterm_is_t
 {
   let [query, options] = newQueryWithOptions();
   query.searchTerms = "test-tag";
   options.resultType = Ci.nsINavHistoryQueryOptions.RESULTS_AS_VISIT;
   testQueryContents(query, options, function (root) {
     compareArrayToResult([], root);
     gTestData.forEach(function (data) {
       let uri = NetUtil.newURI(data.uri);
+      PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
+                                           uri,
+                                           PlacesUtils.bookmarks.DEFAULT_INDEX,
+                                           data.title);
       PlacesUtils.tagging.tagURI(uri, ["test-tag"]);
       compareArrayToResult([data], root);
       PlacesUtils.tagging.untagURI(uri, ["test-tag"]);
       compareArrayToResult([], root);
     });
   });
 });