419792 - Use optimized query to get tags for nsNavHistoryResultNode

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Ed Lee :Mardak]

Bug 415460 added a query mDBGetTags that gets the tags from the DB using GROUP_CONCAT. nsNavHistoryResultNode.tags can use that query.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: wip

fix. however, need to sort the tag list in the query, but the aggregate func doesn't wanna pull the contents of the subselect in a sorted manner.

eg, the comma-separated tags in this example are not sorted properly:

select group_concat(title) from (select title from moz_bookmarks order by title)

if you execute only the subselect portion, the titles are sorted properly.

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: fix + test

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 346005 [details] [diff] [review]
fix + test

diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
--- a/toolkit/components/places/src/nsNavHistory.cpp
+++ b/toolkit/components/places/src/nsNavHistory.cpp
@@ -1197,14 +1197,14 @@
 
   // mDBGetTags
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "SELECT GROUP_CONCAT(t.title, ' ') "
+      "SELECT GROUP_CONCAT(title, ?1) FROM (SELECT t.title AS title "

please, use tag_title or tag_name as alias, title is too widely used in this query

       "FROM moz_bookmarks b "
       "JOIN moz_bookmarks t ON t.id = b.parent "
-      "WHERE b.fk = IFNULL((SELECT id FROM moz_places_temp WHERE url = ?2), "
-                          "(SELECT id FROM moz_places WHERE url = ?2)) "
+      "WHERE b.fk = IFNULL((SELECT id FROM moz_places_temp WHERE url = ?3), "
+                          "(SELECT id FROM moz_places WHERE url = ?3)) "
         "AND b.type = ") +
           nsPrintfCString("%d", nsINavBookmarksService::TYPE_BOOKMARK) +
-        NS_LITERAL_CSTRING(" AND t.parent = ?1 "),
+        NS_LITERAL_CSTRING(" AND t.parent = ?2 ORDER BY t.title)"),

for reference: old versions of sqlite were requiring a double ordering here, see http://www.sqlite.org/cvstrac/tktview?tn=2943,
with latest version (from the February sqlite fix checkin) instead the internal sorting is enough.

Hwv, the query should be indented better to be more readable, i did notice the subquery only at a second read


diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp
--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
@@ -191,30 +191,25 @@
     return NS_OK;
   }
 
-  nsresult rv;
-  nsCOMPtr<nsITaggingService> svc =
-    do_GetService("@mozilla.org/browser/tagging-service;1", &rv);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  nsCOMPtr<nsIURI> uri;
-  rv = NS_NewURI(getter_AddRefs(uri), mURI);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // build the tags string
-  PRUnichar **tags;
-  PRUint32 count;
-  rv = svc->GetTagsForURI(uri, &count, &tags);
-  NS_ENSURE_SUCCESS(rv, rv);
-  if (count > 0) {
-    for (PRUint32 i=0; i < count; i++) {
-      mTags.Append(tags[i]);
-      if (i < count -1) { // separate with commas
-        mTags.Append(NS_LITERAL_STRING(", "));
-      }
-    }
-    NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(count, tags);
-  }
-  aTags.Assign(mTags);
+  // Fetch the tags
+  nsNavHistory* history = nsNavHistory::GetHistoryService();
+  NS_ENSURE_TRUE(history, 0);

i think we usually throw NS_ERROR_OUT_OF_MEMORY in these cases, even if there is not a common style for that.


+  mozIStorageStatement* getTagsStatement = history->DBGetTags();
+
+  mozStorageStatementScoper scoper(getTagsStatement);
+  nsresult rv = getTagsStatement->BindStringParameter(0, NS_LITERAL_STRING(", "));
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = getTagsStatement->BindInt32Parameter(1, history->GetTagsFolder());
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = getTagsStatement->BindUTF8StringParameter(2, mURI);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  PRBool hasTag = PR_FALSE;

hasTags maybe would be more consistent since we get all tags for this uri

+  if (NS_SUCCEEDED(getTagsStatement->ExecuteStep(&hasTag)) && hasTag) {
+    rv = getTagsStatement->GetString(0, mTags);
+    NS_ENSURE_SUCCESS(rv, rv);
+    aTags.Assign(mTags);
+  }

   // If this node is a child of a history, we need to make sure
   // bookmarks-liveupdate is turned on for this query
diff --git a/toolkit/components/places/tests/unit/test_419792_node_tags_property.js b/toolkit/components/places/tests/unit/test_419792_node_tags_property.js
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unit/test_419792_node_tags_property.js
@@ -0,0 +1,84 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is bug 419792 test code.
+ *
+ * The Initial Developer of the Original Code is
+ * Mozilla Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 2008
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Dietrich Ayala <[e-mail]> (Original Author)
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+// get services
+try {
+  var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
+                getService(Ci.nsINavHistoryService);
+  var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+              getService(Ci.nsINavBookmarksService);
+  var tagssvc = Cc["@mozilla.org/browser/tagging-service;1"].
+                getService(Ci.nsITaggingService);
+} catch(ex) {
+  do_throw("Could not get places services\n");
+}

i thought we were starting discarding the throw here


+
+  // add a tag
+  tagssvc.tagURI(bookmarkURI, ["foo"]);
+  do_check_eq(node.tags, "foo");
+
+  // add another tag, to test delimiter

to test delimiter and sorting

+  tagssvc.tagURI(bookmarkURI, ["bar"]);
+  do_check_eq(node.tags, "bar, foo");
+
+  // remove the tags, confirming the property is cleared
+  tagssvc.untagURI(bookmarkURI, null);
+  do_check_eq(node.tags, null); 
+}

I would close the container here to allow for correct cleanup

apart that, r=mak77

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: comments fixed

comments fixed, additional review from mano

Comment 5

[Asaf Romano (gone)]

Comment on attachment 346100 [details] [diff] [review]
comments fixed

r=mano

Comment 6

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/8f08b728aebf

Comment 7

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv