Bug 1085291 - A bookmark node that is inserted by live-update code is missing bookmarkGuid value. r=mak.
☠☠ backed out by 5454e3c612c9 ☠ ☠
authorAsaf Romano <mano@mozilla.com>
Tue, 25 Nov 2014 09:06:37 +0200
changeset 241722 6db486ed2de1dad2999c791f53da136ab9ae874d
parent 241721 b4628cb58bb819b004a4ff96e5b1ae5ee5dc1af5
child 241723 1ad2becc23b3d64df6efad22021c77da217270fd
push id4311
push userraliiev@mozilla.com
push dateMon, 12 Jan 2015 19:37:41 +0000
treeherdermozilla-beta@150c9fed433b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1085291
milestone36.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 1085291 - A bookmark node that is inserted by live-update code is missing bookmarkGuid value. r=mak.
toolkit/components/places/nsNavBookmarks.h
toolkit/components/places/nsNavHistory.cpp
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/tests/unit/xpcshell.ini
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -205,16 +205,21 @@ public:
    * @param aFolderId
    *        The folder to fetch descendants from.
    * @param aDescendantFoldersArray
    *        Output array to put descendant folders id.
    */
   nsresult GetDescendantFolders(int64_t aFolderId,
                                 nsTArray<int64_t>& aDescendantFoldersArray);
 
+  static const int32_t kGetChildrenIndex_Guid;
+  static const int32_t kGetChildrenIndex_Position;
+  static const int32_t kGetChildrenIndex_Type;
+  static const int32_t kGetChildrenIndex_PlaceID;
+
 private:
   static nsNavBookmarks* gBookmarksService;
 
   ~nsNavBookmarks();
 
   /**
    * Checks whether or not aFolderId points to a live bookmark.
    *
@@ -351,21 +356,16 @@ private:
                                       nsTArray<int64_t>& aResult,
                                       bool aSkipTags);
 
   nsresult GetBookmarksForURI(nsIURI* aURI,
                               nsTArray<BookmarkData>& _bookmarks);
 
   int64_t RecursiveFindRedirectedBookmark(int64_t aPlaceId);
 
-  static const int32_t kGetChildrenIndex_Position;
-  static const int32_t kGetChildrenIndex_Type;
-  static const int32_t kGetChildrenIndex_PlaceID;
-  static const int32_t kGetChildrenIndex_Guid;
-
   class RemoveFolderTransaction MOZ_FINAL : public nsITransaction {
   public:
     explicit RemoveFolderTransaction(int64_t aID) : mID(aID) {}
 
     NS_DECL_ISUPPORTS
 
     NS_IMETHOD DoTransaction() {
       nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
--- a/toolkit/components/places/nsNavHistory.cpp
+++ b/toolkit/components/places/nsNavHistory.cpp
@@ -1507,17 +1507,18 @@ PlacesSQLQueryBuilder::SelectAsURI()
                            NS_LITERAL_CSTRING("b2.fk"),
                            mHasSearchTerms,
                            tagsSqlFragment);
 
         mQueryString = NS_LITERAL_CSTRING(
           "SELECT b2.fk, h.url, COALESCE(b2.title, h.title) AS page_title, "
             "h.rev_host, h.visit_count, h.last_visit_date, f.url, b2.id, "
             "b2.dateAdded, b2.lastModified, b2.parent, ") +
-            tagsSqlFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid "
+            tagsSqlFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid, "
+            "b2.guid, b2.position, b2.type, b2.fk "
           "FROM moz_bookmarks b2 "
           "JOIN (SELECT b.fk "
                 "FROM moz_bookmarks b "
                 // ADDITIONAL_CONDITIONS will filter on parent.
                 "WHERE b.type = 1 {ADDITIONAL_CONDITIONS} "
                 ") AS seed ON b2.fk = seed.fk "
           "JOIN moz_places h ON h.id = b2.fk "
           "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
@@ -1531,17 +1532,18 @@ PlacesSQLQueryBuilder::SelectAsURI()
         GetTagsSqlFragment(history->GetTagsFolder(),
                            NS_LITERAL_CSTRING("b.fk"),
                            mHasSearchTerms,
                            tagsSqlFragment);
         mQueryString = NS_LITERAL_CSTRING(
           "SELECT b.fk, h.url, COALESCE(b.title, h.title) AS page_title, "
             "h.rev_host, h.visit_count, h.last_visit_date, f.url, b.id, "
             "b.dateAdded, b.lastModified, b.parent, ") +
-            tagsSqlFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid "
+            tagsSqlFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid,"
+            "b.guid, b.position, b.type, b.fk "
           "FROM moz_bookmarks b "
           "JOIN moz_places h ON b.fk = h.id "
           "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
           "WHERE NOT EXISTS "
               "(SELECT id FROM moz_bookmarks "
                 "WHERE id = b.parent AND parent = ") +
                   nsPrintfCString("%lld", history->GetTagsFolder()) +
               NS_LITERAL_CSTRING(") "
@@ -3897,60 +3899,68 @@ nsNavHistory::RowToResult(mozIStorageVal
     if (itemParentId > 0) {
       // The Places root has parent == 0, but that item id does not really
       // exist. We want to set the parent only if it's a real one.
       parentId = itemParentId;
     }
   }
 
   if (IsQueryURI(url)) {
-    // special case "place:" URIs: turn them into containers
-
-    // We should never expose the history title for query nodes if the
-    // bookmark-item's title is set to null (the history title may be the
-    // query string without the place: prefix). Thus we call getItemTitle
-    // explicitly. Doing this in the SQL query would be less performant since
-    // it should be done for all results rather than only for queries.
-    if (itemId != -1) {
-      nsNavBookmarks *bookmarks = nsNavBookmarks::GetBookmarksService();
-      NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
-
-      rv = bookmarks->GetItemTitle(itemId, title);
-      NS_ENSURE_SUCCESS(rv, rv);
-    }
-
+    // Special case "place:" URIs: turn them into containers.
     nsRefPtr<nsNavHistoryResultNode> resultNode;
     rv = QueryRowToResult(itemId, url, title, accessCount, time, favicon,
                           getter_AddRefs(resultNode));
     NS_ENSURE_SUCCESS(rv,rv);
 
-    if (aOptions->ResultType() == nsNavHistoryQueryOptions::RESULTS_AS_TAG_QUERY) {
+    if (itemId != -1) {
+      rv = aRow->GetUTF8String(nsNavBookmarks::kGetChildrenIndex_Guid,
+                               resultNode->mBookmarkGuid);
+      NS_ENSURE_SUCCESS(rv, rv);
+
+      // We should never expose the history title for query nodes if the
+      // bookmark-item's title is set to null (the history title may be the
+      // query string without the place: prefix). Thus we call getItemTitle
+      // explicitly. Doing this in the SQL query would be less performant since
+      // it should be done for all results rather than only for queries.
+      nsNavBookmarks *bookmarks = nsNavBookmarks::GetBookmarksService();
+      NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
+
+      rv = bookmarks->GetItemTitle(itemId, resultNode->mTitle);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+
+    if (itemId != -1 ||
+        aOptions->ResultType() == nsNavHistoryQueryOptions::RESULTS_AS_TAG_QUERY) {
       // RESULTS_AS_TAG_QUERY has date columns
       resultNode->mDateAdded = aRow->AsInt64(kGetInfoIndex_ItemDateAdded);
       resultNode->mLastModified = aRow->AsInt64(kGetInfoIndex_ItemLastModified);
-    }
-    else if (resultNode->IsFolder()) {
-      // If it's a simple folder node (i.e. a shortcut to another folder), apply
-      // our options for it. However, if the parent type was tag query, we do not
-      // apply them, because it would not yield any results.
-      resultNode->GetAsContainer()->mOptions = aOptions;
+      if (resultNode->IsFolder()) {
+        // If it's a simple folder node (i.e. a shortcut to another folder), apply
+        // our options for it. However, if the parent type was tag query, we do not
+        // apply them, because it would not yield any results.
+        resultNode->GetAsContainer()->mOptions = aOptions;
+      }
     }
 
     resultNode.forget(aResult);
     return rv;
   } else if (aOptions->ResultType() == nsNavHistoryQueryOptions::RESULTS_AS_URI ||
              aOptions->ResultType() == nsNavHistoryQueryOptions::RESULTS_AS_TAG_CONTENTS) {
     nsRefPtr<nsNavHistoryResultNode> resultNode =
       new nsNavHistoryResultNode(url, title, accessCount, time, favicon);
 
     if (itemId != -1) {
       resultNode->mItemId = itemId;
       resultNode->mFolderId = parentId;
       resultNode->mDateAdded = aRow->AsInt64(kGetInfoIndex_ItemDateAdded);
       resultNode->mLastModified = aRow->AsInt64(kGetInfoIndex_ItemLastModified);
+
+      rv = aRow->GetUTF8String(nsNavBookmarks::kGetChildrenIndex_Guid,
+                               resultNode->mBookmarkGuid);
+      NS_ENSURE_SUCCESS(rv, rv);
     }
 
     resultNode->mFrecency = aRow->AsInt32(kGetInfoIndex_Frecency);
     resultNode->mHidden = !!aRow->AsInt32(kGetInfoIndex_Hidden);
 
     nsAutoString tags;
     rv = aRow->GetString(kGetInfoIndex_ItemTags, tags);
     NS_ENSURE_SUCCESS(rv, rv);
@@ -4130,17 +4140,18 @@ nsNavHistory::BookmarkIdToResultNode(int
   nsAutoCString tagsFragment;
   GetTagsSqlFragment(GetTagsFolder(), NS_LITERAL_CSTRING("h.id"),
                      true, tagsFragment);
   // Should match kGetInfoIndex_*
   nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(NS_LITERAL_CSTRING(
       "SELECT b.fk, h.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 "
+             ) + tagsFragment + NS_LITERAL_CSTRING(", h.frecency, h.hidden, h.guid, "
+             "b.guid, b.position, b.type, b.fk "
       "FROM moz_bookmarks b "
       "JOIN moz_places h ON b.fk = h.id "
       "LEFT JOIN moz_favicons f ON h.favicon_id = f.id "
       "WHERE b.id = :item_id ")
   );
   NS_ENSURE_STATE(stmt);
   mozStorageStatementScoper scoper(stmt);
 
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -3579,16 +3579,19 @@ nsNavHistoryFolderResultNode::OnItemAdde
     NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
     rv = bookmarks->ResultNodeForContainer(aItemId, mOptions, getter_AddRefs(node));
     NS_ENSURE_SUCCESS(rv, rv);
   }
   else if (aItemType == nsINavBookmarksService::TYPE_SEPARATOR) {
     node = new nsNavHistorySeparatorResultNode();
     NS_ENSURE_TRUE(node, NS_ERROR_OUT_OF_MEMORY);
     node->mItemId = aItemId;
+    node->mBookmarkGuid = aGUID;
+    node->mDateAdded = aDateAdded;
+    node->mLastModified = aDateAdded;
   }
 
   node->mBookmarkIndex = aIndex;
 
   if (aItemType == nsINavBookmarksService::TYPE_SEPARATOR ||
       GetSortType() == nsINavHistoryQueryOptions::SORT_BY_NONE) {
     // insert at natural bookmarks position
     return InsertChildAt(node, aIndex);
--- a/toolkit/components/places/tests/unit/xpcshell.ini
+++ b/toolkit/components/places/tests/unit/xpcshell.ini
@@ -50,16 +50,17 @@ skip-if = os == "android"
 [test_433317_query_title_update.js]
 [test_433525_hasChildren_crash.js]
 [test_452777.js]
 [test_454977.js]
 [test_463863.js]
 [test_485442_crash_bug_nsNavHistoryQuery_GetUri.js]
 [test_486978_sort_by_date_queries.js]
 [test_536081.js]
+[test_1085291.js]
 [test_adaptive.js]
 # Bug 676989: test hangs consistently on Android
 skip-if = os == "android"
 [test_adaptive_bug527311.js]
 [test_analyze.js]
 [test_annotations.js]
 [test_asyncExecuteLegacyQueries.js]
 # Bug 676989: test hangs consistently on Android