Bug 1432746 - Some folders in the Library may not show their bookmark children. r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 24 Jan 2018 17:55:42 +0100
changeset 737940 66eea325825007266e08424630b092b9e8d75b67
parent 737939 6602576987baec9edbaaad117114ba5227db6261
push id96799
push usermak77@bonardo.net
push dateThu, 25 Jan 2018 11:26:07 +0000
reviewersstandard8
bugs1432746
milestone60.0a1
Bug 1432746 - Some folders in the Library may not show their bookmark children. r=standard8 Don't use the parent node options when creating new folder nodes, since they should retain their original options. Additionally, we can filter nodes in the queries rather than building a lot of nodes that will be filtered out. MozReview-Commit-ID: MmlGDe5QgV
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavBookmarks.h
toolkit/components/places/nsNavHistoryResult.cpp
toolkit/components/places/tests/queries/test_options_inherit.js
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -2124,69 +2124,72 @@ nsNavBookmarks::ResultNodeForContainer(i
   NS_ADDREF(*aNode);
   return NS_OK;
 }
 
 
 nsresult
 nsNavBookmarks::QueryFolderChildren(
   int64_t aFolderId,
-  nsNavHistoryQueryOptions* aOriginalOptions,
   nsNavHistoryQueryOptions* aOptions,
   nsCOMArray<nsNavHistoryResultNode>* aChildren)
 {
-  NS_ENSURE_ARG_POINTER(aOriginalOptions);
   NS_ENSURE_ARG_POINTER(aOptions);
   NS_ENSURE_ARG_POINTER(aChildren);
 
   // Select all children of a given folder, sorted by position.
   // This is a LEFT JOIN because not all bookmarks types have a place.
   // We construct a result where the first columns exactly match those returned
   // by mDBGetURLPageInfo, and additionally contains columns for position,
   // item_child, and folder_child from moz_bookmarks.
   nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
     "SELECT h.id, h.url, b.title, h.rev_host, h.visit_count, "
            "h.last_visit_date, null, b.id, b.dateAdded, b.lastModified, "
            "b.parent, null, h.frecency, h.hidden, h.guid, null, null, null, "
            "b.guid, b.position, b.type, b.fk "
     "FROM moz_bookmarks b "
     "LEFT JOIN moz_places h ON b.fk = h.id "
     "WHERE b.parent = :parent "
+      "AND (NOT :excludeItems OR "
+           "b.type = :folder OR "
+           "h.url_hash BETWEEN hash('place', 'prefix_lo') AND hash('place', 'prefix_hi')) "
     "ORDER BY b.position ASC"
   );
   NS_ENSURE_STATE(stmt);
   mozStorageStatementScoper scoper(stmt);
 
   nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("parent"), aFolderId);
   NS_ENSURE_SUCCESS(rv, rv);
+  rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("folder"), TYPE_FOLDER);
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("excludeItems"), aOptions->ExcludeItems());
+  NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<mozIStorageValueArray> row = do_QueryInterface(stmt, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
 
   int32_t index = -1;
   bool hasResult;
   while (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) {
-    rv = ProcessFolderNodeRow(row, aOriginalOptions, aOptions, aChildren, index);
+    rv = ProcessFolderNodeRow(row, aOptions, aChildren, index);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   return NS_OK;
 }
 
 
 nsresult
 nsNavBookmarks::ProcessFolderNodeRow(
   mozIStorageValueArray* aRow,
-  nsNavHistoryQueryOptions* aOriginalOptions,
   nsNavHistoryQueryOptions* aOptions,
   nsCOMArray<nsNavHistoryResultNode>* aChildren,
   int32_t& aCurrentIndex)
 {
   NS_ENSURE_ARG_POINTER(aRow);
-  NS_ENSURE_ARG_POINTER(aOriginalOptions);
   NS_ENSURE_ARG_POINTER(aOptions);
   NS_ENSURE_ARG_POINTER(aChildren);
 
   // The results will be in order of aCurrentIndex. Even if we don't add a node
   // because it was excluded, we need to count its index, so do that before
   // doing anything else.
   aCurrentIndex++;
 
@@ -2199,24 +2202,20 @@ nsNavBookmarks::ProcessFolderNodeRow(
 
   RefPtr<nsNavHistoryResultNode> node;
 
   if (itemType == TYPE_BOOKMARK) {
     nsNavHistory* history = nsNavHistory::GetHistoryService();
     NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
     rv = history->RowToResult(aRow, aOptions, getter_AddRefs(node));
     NS_ENSURE_SUCCESS(rv, rv);
-
     uint32_t nodeType;
     node->GetType(&nodeType);
-    if ((nodeType == nsINavHistoryResultNode::RESULT_TYPE_QUERY &&
-         aOptions->ExcludeQueries()) ||
-        (nodeType != nsINavHistoryResultNode::RESULT_TYPE_QUERY &&
-         nodeType != nsINavHistoryResultNode::RESULT_TYPE_FOLDER_SHORTCUT &&
-         aOptions->ExcludeItems())) {
+    if (nodeType == nsINavHistoryResultNode::RESULT_TYPE_QUERY &&
+        aOptions->ExcludeQueries()) {
       return NS_OK;
     }
   }
   else if (itemType == TYPE_FOLDER) {
     // ExcludeReadOnlyFolders currently means "ExcludeLivemarks" (to be fixed in
     // bug 1072833)
     if (aOptions->ExcludeReadOnlyFolders()) {
       if (IsLivemark(id))
@@ -2227,34 +2226,33 @@ nsNavBookmarks::ProcessFolderNodeRow(
     bool isNull;
     rv = aRow->GetIsNull(nsNavHistory::kGetInfoIndex_Title, &isNull);
     NS_ENSURE_SUCCESS(rv, rv);
     if (!isNull) {
       rv = aRow->GetUTF8String(nsNavHistory::kGetInfoIndex_Title, title);
       NS_ENSURE_SUCCESS(rv, rv);
     }
 
-    node = new nsNavHistoryFolderResultNode(title, aOriginalOptions, id);
+    // Don't use options from the parent to build the new folder node, it will
+    // inherit those later when it's inserted in the result.
+    node = new nsNavHistoryFolderResultNode(title, new nsNavHistoryQueryOptions(), id);
 
     rv = aRow->GetUTF8String(kGetChildrenIndex_Guid, node->mBookmarkGuid);
     NS_ENSURE_SUCCESS(rv, rv);
     node->GetAsFolder()->mTargetFolderGuid = node->mBookmarkGuid;
 
     rv = aRow->GetInt64(nsNavHistory::kGetInfoIndex_ItemDateAdded,
                         reinterpret_cast<int64_t*>(&node->mDateAdded));
     NS_ENSURE_SUCCESS(rv, rv);
     rv = aRow->GetInt64(nsNavHistory::kGetInfoIndex_ItemLastModified,
                         reinterpret_cast<int64_t*>(&node->mLastModified));
     NS_ENSURE_SUCCESS(rv, rv);
   }
   else {
     // This is a separator.
-    if (aOptions->ExcludeItems()) {
-      return NS_OK;
-    }
     node = new nsNavHistorySeparatorResultNode();
 
     node->mItemId = id;
     rv = aRow->GetUTF8String(kGetChildrenIndex_Guid, node->mBookmarkGuid);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = aRow->GetInt64(nsNavHistory::kGetInfoIndex_ItemDateAdded,
                         reinterpret_cast<int64_t*>(&node->mDateAdded));
     NS_ENSURE_SUCCESS(rv, rv);
@@ -2270,17 +2268,16 @@ nsNavBookmarks::ProcessFolderNodeRow(
   NS_ENSURE_TRUE(aChildren->AppendObject(node), NS_ERROR_OUT_OF_MEMORY);
   return NS_OK;
 }
 
 
 nsresult
 nsNavBookmarks::QueryFolderChildrenAsync(
   nsNavHistoryFolderResultNode* aNode,
-  int64_t aFolderId,
   mozIStoragePendingStatement** _pendingStmt)
 {
   NS_ENSURE_ARG_POINTER(aNode);
   NS_ENSURE_ARG_POINTER(_pendingStmt);
 
   // Select all children of a given folder, sorted by position.
   // This is a LEFT JOIN because not all bookmarks types have a place.
   // We construct a result where the first columns exactly match those returned
@@ -2289,21 +2286,28 @@ nsNavBookmarks::QueryFolderChildrenAsync
   nsCOMPtr<mozIStorageAsyncStatement> stmt = mDB->GetAsyncStatement(
     "SELECT h.id, h.url, b.title, h.rev_host, h.visit_count, "
            "h.last_visit_date, null, b.id, b.dateAdded, b.lastModified, "
            "b.parent, null, h.frecency, h.hidden, h.guid, null, null, null, "
            "b.guid, b.position, b.type, b.fk "
     "FROM moz_bookmarks b "
     "LEFT JOIN moz_places h ON b.fk = h.id "
     "WHERE b.parent = :parent "
+      "AND (NOT :excludeItems OR "
+           "b.type = :folder OR "
+           "h.url_hash BETWEEN hash('place', 'prefix_lo') AND hash('place', 'prefix_hi')) "
     "ORDER BY b.position ASC"
   );
   NS_ENSURE_STATE(stmt);
 
-  nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("parent"), aFolderId);
+  nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("parent"), aNode->mTargetFolderItemId);
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("folder"), TYPE_FOLDER);
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("excludeItems"), aNode->mOptions->ExcludeItems());
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<mozIStoragePendingStatement> pendingStmt;
   rv = stmt->ExecuteAsync(aNode, getter_AddRefs(pendingStmt));
   NS_ENSURE_SUCCESS(rv, rv);
 
   NS_IF_ADDREF(*_pendingStmt = pendingStmt);
   return NS_OK;
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -124,56 +124,50 @@ public:
   nsresult ResultNodeForContainer(int64_t aID,
                                   nsNavHistoryQueryOptions* aOptions,
                                   nsNavHistoryResultNode** aNode);
 
   // Find all the children of a folder, using the given query and options.
   // For each child, a ResultNode is created and added to |children|.
   // The results are ordered by folder position.
   nsresult QueryFolderChildren(int64_t aFolderId,
-                               nsNavHistoryQueryOptions* aOriginalOptions,
                                nsNavHistoryQueryOptions* aOptions,
                                nsCOMArray<nsNavHistoryResultNode>* children);
 
   /**
    * Turns aRow into a node and appends it to aChildren if it is appropriate to
    * do so.
    *
    * @param aRow
    *        A Storage statement (in the case of synchronous execution) or row of
    *        a result set (in the case of asynchronous execution).
-   * @param aOriginalOptions
-   *        The original options of the parent folder node.  These are the
-   *        options used to define the parent node.
    * @param aOptions
    *        The options of the parent folder node. These are the options used
    *        to fill the parent node.
    * @param aChildren
    *        The children of the parent folder node.
    * @param aCurrentIndex
    *        The index of aRow within the results.  When called on the first row,
    *        this should be set to -1.
    */
   nsresult ProcessFolderNodeRow(mozIStorageValueArray* aRow,
-                                nsNavHistoryQueryOptions* aOriginalOptions,
                                 nsNavHistoryQueryOptions* aOptions,
                                 nsCOMArray<nsNavHistoryResultNode>* aChildren,
                                 int32_t& aCurrentIndex);
 
   /**
    * The async version of QueryFolderChildren.
    *
    * @param aNode
    *        The folder node that will receive the children.
    * @param _pendingStmt
    *        The Storage pending statement that will be used to control async
    *        execution.
    */
   nsresult QueryFolderChildrenAsync(nsNavHistoryFolderResultNode* aNode,
-                                    int64_t aFolderId,
                                     mozIStoragePendingStatement** _pendingStmt);
 
   /**
    * @return index of the new folder in aIndex, whether it was passed in or
    *         generated by autoincrement.
    *
    * @note If aFolder is -1, uses the autoincrement id for folder index.
    * @note aTitle will be truncated to TITLE_LENGTH_MAX
--- a/toolkit/components/places/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/nsNavHistoryResult.cpp
@@ -3151,17 +3151,16 @@ nsNavHistoryFolderResultNode::FillChildr
   NS_ASSERTION(mChildren.Count() == 0,
                "We are trying to fill children when there already are some");
 
   nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
   NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
 
   // Actually get the folder children from the bookmark service.
   nsresult rv = bookmarks->QueryFolderChildren(mTargetFolderItemId,
-                                               mOriginalOptions,
                                                mOptions,
                                                &mChildren);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // PERFORMANCE: it may be better to also fill any child folders at this point
   // so that we can draw tree twisties without doing a separate query later.
   // If we don't end up drawing twisties a lot, it doesn't matter. If we do
   // this, we should wrap everything in a transaction here on the bookmark
@@ -3242,18 +3241,17 @@ nsNavHistoryFolderResultNode::FillChildr
 
   // ProcessFolderNodeChild, called in HandleResult, increments this for every
   // result row it processes.  Initialize it here as we begin async execution.
   mAsyncBookmarkIndex = -1;
 
   nsNavBookmarks* bmSvc = nsNavBookmarks::GetBookmarksService();
   NS_ENSURE_TRUE(bmSvc, NS_ERROR_OUT_OF_MEMORY);
   nsresult rv =
-    bmSvc->QueryFolderChildrenAsync(this, mTargetFolderItemId,
-                                    getter_AddRefs(mAsyncPendingStmt));
+    bmSvc->QueryFolderChildrenAsync(this, getter_AddRefs(mAsyncPendingStmt));
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Register with the result for updates.  All updates during async execution
   // will cause it to be restarted.
   EnsureRegisteredAsFolderObserver();
 
   return NS_OK;
 }
@@ -3275,18 +3273,17 @@ nsNavHistoryFolderResultNode::HandleResu
   if (!bmSvc) {
     CancelAsyncOpen(false);
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   // Consume all the currently available rows of the result set.
   nsCOMPtr<mozIStorageRow> row;
   while (NS_SUCCEEDED(aResultSet->GetNextRow(getter_AddRefs(row))) && row) {
-    nsresult rv = bmSvc->ProcessFolderNodeRow(row, mOriginalOptions,
-                                              mOptions, &mChildren,
+    nsresult rv = bmSvc->ProcessFolderNodeRow(row, mOptions, &mChildren,
                                               mAsyncBookmarkIndex);
     if (NS_FAILED(rv)) {
       CancelAsyncOpen(false);
       return rv;
     }
   }
 
   return NS_OK;
@@ -3572,17 +3569,19 @@ nsNavHistoryFolderResultNode::OnItemAdde
     nsNavHistory* history = nsNavHistory::GetHistoryService();
     NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
     rv = history->BookmarkIdToResultNode(aItemId, mOptions, getter_AddRefs(node));
     NS_ENSURE_SUCCESS(rv, rv);
   }
   else if (aItemType == nsINavBookmarksService::TYPE_FOLDER) {
     nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
     NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
-    rv = bookmarks->ResultNodeForContainer(aItemId, mOptions, getter_AddRefs(node));
+    rv = bookmarks->ResultNodeForContainer(aItemId,
+                                           new nsNavHistoryQueryOptions(),
+                                           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;
--- a/toolkit/components/places/tests/queries/test_options_inherit.js
+++ b/toolkit/components/places/tests/queries/test_options_inherit.js
@@ -17,51 +17,78 @@ add_task(async function() {
         type: PlacesUtils.bookmarks.TYPE_FOLDER,
         children: [
           { title: "query",
             url: "place:queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS,
           },
           { title: "bm",
             url: "http://example.com",
           },
+          {
+            type: PlacesUtils.bookmarks.TYPE_SEPARATOR,
+          },
         ]
       },
       { title: "bm",
         url: "http://example.com",
       },
+      {
+        type: PlacesUtils.bookmarks.TYPE_SEPARATOR,
+      },
     ]
   });
 
-  await test_query({}, 2, 2, 3);
-  await test_query({ expandQueries: false }, 2, 2, 0);
+  await test_query({}, 3, 3, 3);
+  await test_query({ expandQueries: false }, 3, 3, 0);
   await test_query({ excludeItems: true }, 1, 1, 0);
   await test_query({ excludeItems: true, expandQueries: false }, 1, 1, 0);
   await test_query({ excludeItems: true, excludeQueries: true }, 1, 0, 0);
 });
 
-
 async function test_query(opts, expectedRootCc, expectedFolderCc, expectedQueryCc) {
   let query = PlacesUtils.history.getNewQuery();
   query.setFolders([PlacesUtils.unfiledBookmarksFolderId], 1);
   let options = PlacesUtils.history.getNewQueryOptions();
   for (const [o, v] of Object.entries(opts)) {
-    info(`setting ${o} to ${v}`);
+    info(`Setting ${o} to ${v}`);
     options[o] = v;
   }
   let root = PlacesUtils.history.executeQuery(query, options).root;
   root.containerOpen = true;
 
   Assert.equal(root.childCount, expectedRootCc, "Checking root child count");
   if (root.childCount > 0) {
     let folder = root.getChild(0);
     Assert.equal(folder.title, "folder", "Found the expected folder");
+
+    // Check the folder uri doesn't reflect the root options, since those
+    // options are inherited and not part of this node declaration.
+    checkURIOptions(folder.uri);
+
     PlacesUtils.asContainer(folder).containerOpen = true;
     Assert.equal(folder.childCount, expectedFolderCc, "Checking folder child count");
     if (folder.childCount) {
       let placeQuery = folder.getChild(0);
       PlacesUtils.asQuery(placeQuery).containerOpen = true;
       Assert.equal(placeQuery.childCount, expectedQueryCc, "Checking query child count");
       placeQuery.containerOpen = false;
     }
     folder.containerOpen = false;
   }
+  let f = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+    type: PlacesUtils.bookmarks.TYPE_FOLDER
+  });
+  checkURIOptions(root.getChild(root.childCount - 1).uri);
+  await PlacesUtils.bookmarks.remove(f);
+
   root.containerOpen = false;
 }
+
+function checkURIOptions(uri) {
+  info("Checking options for uri " + uri);
+  let folderOptions = { };
+  PlacesUtils.history.queryStringToQueries(uri, {}, {}, folderOptions);
+  folderOptions = folderOptions.value;
+  Assert.equal(folderOptions.excludeItems, false, "ExcludeItems should not be changed");
+  Assert.equal(folderOptions.excludeQueries, false, "ExcludeQueries should not be changed");
+  Assert.equal(folderOptions.expandQueries, true, "ExpandQueries should not be changed");
+}