Mark Banner (:standard8) Assignee
	Description • 7 years ago

This is the "All Bookmarks" part of bug 1310295 - we need to transition the "All Bookmarks" special folder from that to a query, to aid making the left pane of the library completely virtual.

	Mark Banner (:standard8) Assignee
	Comment 5 • 7 years ago

WIP patches attached. A lot of it is mostly functional, however there's a lot of issues with the edit bookmarks dialog that I've not been able to resolve yet.

Still also need to figure out the mobile bookmarks story, and the remaining XXXs, which I'll look into next year.

	Lina Butler [:lina]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8938396 [details]
Bug 1423896 - Make the All Bookmarks folder for the left pane of the Library a virtual query.

https://reviewboard.mozilla.org/r/209102/#review214866

::: toolkit/components/places/nsNavHistory.cpp:1945
(Diff revision 1)
>  }
>  
>  nsresult
> +PlacesSQLQueryBuilder::SelectAsRoots()
> +{
> +  // XXX Mobile Folder???

Mobile is an odd one: we only want to show it if you have bookmarks in the mobile root, and hide if it's empty.

There's some complexity now to make that work, including https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/toolkit/components/places/PlacesSyncUtils.jsm#1050 and https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/browser/base/content/browser-places.js#1507,1522.

Would this be easier to handle now that "All Bookmarks" are virtual?

	Mark Banner (:standard8) Assignee
	Comment 7 • 7 years ago
mozreview-review-reply

Comment on attachment 8938396 [details]
Bug 1423896 - Make the All Bookmarks folder for the left pane of the Library a virtual query.

https://reviewboard.mozilla.org/r/209102/#review214866

> Mobile is an odd one: we only want to show it if you have bookmarks in the mobile root, and hide if it's empty.
> 
> There's some complexity now to make that work, including https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/toolkit/components/places/PlacesSyncUtils.jsm#1050 and https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/browser/base/content/browser-places.js#1507,1522.
> 
> Would this be easier to handle now that "All Bookmarks" are virtual?

Marco and I discussed this today, and we've decided that the all bookmarks query can currently use UPDATE_COMPLEX_WITH_BOOKMARKS which will cause it to update any time a bookmark is added/changed/inserted. When the query is built, it can check the value of the pref to decide if to display the mobile folder or not.

If it proves too costly later on, we could move to a dom attribute and hide the mobile folder if it is empty.

	Lina Butler [:lina]
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8938396 [details]
Bug 1423896 - Make the All Bookmarks folder for the left pane of the Library a virtual query.

https://reviewboard.mozilla.org/r/209102/#review214866

> Marco and I discussed this today, and we've decided that the all bookmarks query can currently use UPDATE_COMPLEX_WITH_BOOKMARKS which will cause it to update any time a bookmark is added/changed/inserted. When the query is built, it can check the value of the pref to decide if to display the mobile folder or not.
> 
> If it proves too costly later on, we could move to a dom attribute and hide the mobile folder if it is empty.

Interesting! Could you tell me more about `UPDATE_COMPLEX_WITH_BOOKMARKS`?

Currently, we count the children of the mobile root after each sync, update the pref, then either update or remove the mobile query. So, if you've never signed in to Sync, or you're signed in but haven't bookmarked anything on iOS or Android, we hide the query. If you've added some mobile bookmarks, regardless of whether you're currently signed in to Sync, we show the query. (In theory; we've also seen, but haven't been able to reproduce, duplicate mobile queries like https://i.imgur.com/c7b9uyr.png). If you move all bookmarks out of the mobile root, we hide the query again.

Does `UPDATE_COMPLEX_WITH_BOOKMARKS` mean we wouldn't have to manually count children or set the pref, and the UI code could look at `childCount` for the query to decide whether to show it? Would this let the UI code take over managing the mobile query, instead of having Sync do it after every bookmark sync?

	Lina Butler [:lina]
	Comment 9 • 7 years ago

(In reply to Mark Banner (:standard8) from comment #7)
> Marco and I discussed this today, and we've decided that the all bookmarks
> query can currently use UPDATE_COMPLEX_WITH_BOOKMARKS which will cause it to
> update any time a bookmark is added/changed/inserted. When the query is
> built, it can check the value of the pref to decide if to display the mobile
> folder or not.

Oh, I completely misunderstood, sorry. I think you're saying the all bookmarks query would use `UPDATE_COMPLEX_WITH_BOOKMARKS`, Sync would still call `ensureMobileQuery` to upsert or remove the mobile query, and then all bookmarks would update itself?

	Marco Bonardo [:mak]
	Comment 10 • 7 years ago

(In reply to Kit Cambridge (he/him) [:kitcambridge] from comment #9)
> Oh, I completely misunderstood, sorry. I think you're saying the all
> bookmarks query would use `UPDATE_COMPLEX_WITH_BOOKMARKS`, Sync would still
> call `ensureMobileQuery` to upsert or remove the mobile query, and then all
> bookmarks would update itself?

That's right, Sync would still have to do its job.
The other alternatives are:
1. always show the mobile root and stop caring
2. have an "hideifempty" attribute that can apply to Places nodes and hides the node if it's empty (hard to do for trees?)

	Mark Banner (:standard8) Assignee
	Comment 11 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #10)
> (In reply to Kit Cambridge (he/him) [:kitcambridge] from comment #9)
> > Oh, I completely misunderstood, sorry. I think you're saying the all
> > bookmarks query would use `UPDATE_COMPLEX_WITH_BOOKMARKS`, Sync would still
> > call `ensureMobileQuery` to upsert or remove the mobile query, and then all
> > bookmarks would update itself?
> 
> That's right, Sync would still have to do its job.

I've just been looking at the code again. Sync doesn't need to create the query any more - as the newer RESULTS_AS_ROOTS_QUERY will manage it for us. However, at the moment with my patch the new query isn't always guaranteed to be poked at the right time when the pref changes, so I've filed bug 1428350 to fix that, and hopefully improve the overall performance of the query a bit.

	Mark Banner (:standard8) Assignee
	Comment 17 • 7 years ago

The updated patch set isn't quite complete yet (still a few XXX to resolve & test failures to fix). These also depend on the patch for bug 1428342.

	Mark Banner (:standard8) Assignee
	Comment 23 • 7 years ago

This week's updates resolves the remaining XXX. There's still one known test failure, for which I need to work out how to handle rebuilding the left pane tree and maintaining open/closed state. We have some ideas for that, just have to decide which is best.

	Mark Banner (:standard8) Assignee
	Comment 29 • 7 years ago
mozreview-review

Comment on attachment 8938396 [details]
Bug 1423896 - Make the All Bookmarks folder for the left pane of the Library a virtual query.

https://reviewboard.mozilla.org/r/209102/#review219772

::: browser/components/places/content/editBookmarkOverlay.js:61
(Diff revision 2)
>        }
>        let parent = node.parent;
>        isParentReadOnly = !PlacesUtils.nodeIsFolder(parent);
>        if (!isParentReadOnly) {
>          let folderId = PlacesUtils.getConcreteItemId(parent);
> +        // XXX What should we do here? Can we actually hit this with allBookmarksFolderId ????

Note to self/reviewer: We no longer need to check for allBookmarksFolderId (or equivalent) here - the toolbar/menu/other virtual folders are folder shortcuts, which `readOnly()` checks for already, so we do the right thing.

	Mark Banner (:standard8) Assignee
	Comment 30 • 7 years ago

Comment on attachment 8938393 [details]
Bug 1423896 - When excluding queries within places queries, we shouldn't exclude folder shortcuts.

I'm currently looking into a small issue with this - we're not filtering queries during searches any more. I think I know how we can get around it.

	Mark Banner (:standard8) Assignee
	Comment 36 • 7 years ago

The updated patches fix the previously mentioned issue and some of the try server issues. There's one more failure in the sync tests - test_bookmark_repair_responder.js - that I'm currently discussing with Kit. Shouldn't affect the rest of the patches.

	Marco Bonardo [:mak]
	Comment 39 • 7 years ago
mozreview-review

Comment on attachment 8938393 [details]
Bug 1423896 - When excluding queries within places queries, we shouldn't exclude folder shortcuts.

https://reviewboard.mozilla.org/r/209096/#review221718

::: toolkit/components/places/nsNavHistory.cpp:3567
(Diff revision 5)
> +  bool excludeQueries = aOptions->ExcludeQueries();
>    for (int32_t nodeIndex = 0; nodeIndex < aSet.Count(); nodeIndex++) {
> -    // exclude-queries is implicit when searching, we're only looking at
> -    // plan URI nodes
> -    if (!aSet[nodeIndex]->IsURI())
> +    if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_TAG_CONTENTS &&
> +        !aSet[nodeIndex]->IsURI() ) {
> +      // exclude-queries is implicit when searching, if we're only looking at
> +      // plain URI nodes.

this comment looks confusing, it was already... The whole code here is.
I'd probably merge the 2 if about tags and do something like:

if (excludeQueries && aSet[nodeIndex]->IsQuery()) {
  continue;
}

// RESULTS_AS_TAG_CONTENTS returns a set of uri nodes ordered by place_id
// and lastModified. The set may contain duplicates, and to remove them
// we can just retain the first result
if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_TAG_CONTENTS &&
    (!aSet[nodeIndex]->IsURI() ||
     nodeIndex > 0 && aSet[nodeIndex]->mURI == aSet[nodeIndex-1]->mURI)) {
  continue;
}

...

// If there are search terms, we are already getting only uri nodes,
// thus we don't need to filter node types. Though, we must check for
// matching terms.
bool appendNode = false;
...

::: toolkit/components/places/tests/queries/test_excludeQueries.js:1
(Diff revision 5)
> +"use strict";

please add a pd license header.

	Marco Bonardo [:mak]
	Comment 40 • 7 years ago
mozreview-review

Comment on attachment 8938394 [details]
Bug 1423896 - Rename PlacesOrganizer.selectLeftPaneQuery to selectLeftPaneBuiltIn to better reflect what it is actually selecting.

https://reviewboard.mozilla.org/r/209098/#review222002

	Marco Bonardo [:mak]
	Comment 41 • 7 years ago
mozreview-review

Comment on attachment 8938395 [details]
Bug 1423896 - Rewrite browser_library_panel_leak.js to use async & await.

https://reviewboard.mozilla.org/r/209100/#review222004

::: browser/components/places/tests/browser/browser_library_panel_leak.js:15
(Diff revision 5)
>   * https://bugzilla.mozilla.org/show_bug.cgi?id=433231
>   *
>   * STRs: Open Library, select an history entry in History, close Library.
>   * ISSUE: We were adding a bookmarks observer when editing a bookmark, when
>   *        selecting an history entry the panel was not un-initialized, and
>   *        since an histroy entry does not have an itemId, the observer was

while here, typo: histroy

::: browser/components/places/tests/browser/browser_library_panel_leak.js:24
(Diff revision 5)
>  const TEST_URI = "http://www.mozilla.org/";
>  
> -function test() {
> -  function onLibraryReady(organizer) {
> +add_task(async function test_no_leak_closing_library_with_history_selected() {
> +  // Add an history entry.
> +  await PlacesTestUtils.addVisits(
> +    {uri: PlacesUtils._uri(TEST_URI), visitDate: Date.now() * 1000,

_uri is not necessary here, just pass TEST_URI.
Also, visitDate can be a Date object and is optional.

::: browser/components/places/tests/browser/browser_library_panel_leak.js:25
(Diff revision 5)
>  
> -function test() {
> -  function onLibraryReady(organizer) {
> +add_task(async function test_no_leak_closing_library_with_history_selected() {
> +  // Add an history entry.
> +  await PlacesTestUtils.addVisits(
> +    {uri: PlacesUtils._uri(TEST_URI), visitDate: Date.now() * 1000,
> +      transition: PlacesUtils.history.TRANSITION_TYPED}

Unless it's strictly required to be typed (doesn't look like), this can be omitted.
In practice you can
await PlacesTestUtils.addVisits(TEST_URI);

::: browser/components/places/tests/browser/browser_library_panel_leak.js:50
(Diff revision 5)
> -    // Clean up history.
> -    PlacesTestUtils.clearHistory().then(finish);
> -  }
>  
> -  waitForExplicitFinish();
> -  // Add an history entry.
> +  // Clean up history.
> +  await PlacesTestUtils.clearHistory();

Please use await PlacesUtils.history.clear();

clearHistory() is deprecated (I'll now file a bug to remove it, I thought we had one)

	Marco Bonardo [:mak]
	Comment 42 • 7 years ago

I reproduced a strange bug in the Star panel tree.
1. Select Bookmarks Menu
2. Click New Folder
3. Click  Unsorted Bookmarks (will dismiss the new folder name)
4. Click Bookmarks Menu

At this point the tree shows 2 Unsorted Bookmarks folders :\

	Marco Bonardo [:mak]
	Comment 43 • 7 years ago

Ah, from this point on, every time I open and close Bookmarks menu, a new Other bookmarks appears. Looks like a bug in the treeview

	Marco Bonardo [:mak]
	Comment 44 • 7 years ago
mozreview-review

Comment on attachment 8938396 [details]
Bug 1423896 - Make the All Bookmarks folder for the left pane of the Library a virtual query.

https://reviewboard.mozilla.org/r/209102/#review222354

Sorry, lots of comments, but this looks good overall!
I also probably found a bug, see the previous comment.

::: browser/components/places/PlacesUIUtils.jsm:527
(Diff revision 6)
>        // canUserRemove doesn't accept root nodes.
>        return false;
>      }
>  
> +    // Is it a query pointing to one of the special root folders?
> +    if (PlacesUtils.nodeIsFolder(aNode)) {

let's restrict this check to PlacesUtils.nodeIsQuery(parentNode) && PlacesUtils.nodeIsFolder(aNode)

::: browser/components/places/PlacesUIUtils.jsm:531
(Diff revision 6)
> +    // Is it a query pointing to one of the special root folders?
> +    if (PlacesUtils.nodeIsFolder(aNode)) {
> +      let guid = PlacesUtils.getConcreteItemGuid(aNode);
> +      // If the parent folder is not a folder, it is a query, and so this node
> +      // cannot be removed.
> +      if (PlacesUtils.isRootItem(guid) && !PlacesUtils.nodeIsFolder(parentNode)) {

...and remove the nodeIsFolder check from here.

::: browser/components/places/PlacesUIUtils.jsm:875
(Diff revision 6)
> -        { title: "",
> -          concreteTitle: PlacesUtils.getString("OtherBookmarksFolderTitle"),
> -          concreteId: PlacesUtils.unfiledBookmarksFolderId },
>      };
>      // All queries but PlacesRoot.
>      const EXPECTED_QUERY_COUNT = 7;

This looks broken now? I suspect due to this the code is recreating the left pane folder at every opening.

::: browser/components/places/PlacesUIUtils.jsm:1062
(Diff revision 6)
>                            Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY +
>                            "&sort=" +
>                            Ci.nsINavHistoryQueryOptions.SORT_BY_TITLE_ASCENDING);
>  
>          // All Bookmarks Folder.
> -        allBookmarksId = this.create_folder("AllBookmarks", leftPaneRoot, false);
> +        // allBookmarksId = this.create_folder("AllBookmarks", leftPaneRoot, false);

leftover

::: browser/components/places/content/controller.js:1381
(Diff revision 6)
>     *        A node unwrapped by PlacesUtils.unwrapNodes().
>     * @return True if the node can be moved, false otherwise.
>     */
>    canMoveUnwrappedNode(unwrappedNode) {
> -    if (unwrappedNode.id <= 0 || PlacesUtils.isRootItem(unwrappedNode.id)) {
> +    if ((unwrappedNode.concreteGuid && PlacesUtils.isRootItem(unwrappedNode.concreteGuid)) ||
> +        (unwrappedNode.id <= 0 || PlacesUtils.isRootItem(unwrappedNode.id))) {

You can probably lose the parenthesis on the second branch

Off-hand this may not be correct, indeed a node can be moved even if it's a folder shortcut to a root (it could just be a shortcut the user created somewhere, and that's indeed a good test to make), the original check was to avoid potentially moving a root, that in reality cannot ever happen because we don't expose those in the UI. It was an overzealous check.

Is not enough the fact that the Library shortcuts parent is a query to take the right path earlier without this check? I mean, the operation should already be set to a copy, this is just a fallback path for a move operation.

TL;DR the added check on concreteGuid may not be necessary, we should gather the feature from the fact parent is a query.

::: browser/components/places/content/places.js:52
(Diff revision 6)
> +   * @param {String} item The built-in item to select, may be one of (case sensitive):
> +   *                      AllBookmarks, BookmarksMenu, BookmarksToolbar,
> +   *                      History, Downloads, Tags, UnfiledBookmarks.
> +   */
> +  selectLeftPaneBuiltIn(item) {
> +    if (!this._selectLeftPaneBuiltInInternal(item)) {

I'd remove the internal version, the only consumer below that is using it like a boolean could just try/catch.

::: browser/components/places/content/places.js:87
(Diff revision 6)
> +      case "BookmarksToolbar":
> +        this._places.selectItems([PlacesUtils.bookmarks.virtualToolbarGuid]);
> +        break;
> +      case "UnfiledBookmarks":
> +        this._places.selectItems([PlacesUtils.bookmarks.virtualUnfiledGuid]);
> +        break;

We could speed this up a bit by opening AllBookmarks, and then passing false as second argument to selectItems.
Tht way in case the user has lots of history/tags, we won't pass through all of their children to find these items.

::: browser/components/places/content/places.js:102
(Diff revision 6)
>     *
>     * @param aHierarchy A single container or an array of containers, sorted from
>     *                   the outmost to the innermost in the hierarchy. Each
>     *                   container may be either an item id, a Places URI string,
> -   *                   or a named query.
> +   *                   or a named query, or one of:
> +   *                   "BookmarksMenu", "BookmarksToolbar", "UnfiledBookmarks", "AllBookmarks".

Is not "a named query" the same thing as "BookmarksMenu" and so on?
So maybe s/, or one of:/, like:/

::: browser/components/places/content/places.js:123
(Diff revision 6)
>            case "string":
> -            if (container.substr(0, 6) == "place:")
> +            if (!this._selectLeftPaneBuiltInInternal(container)) {
> +              if (container.substr(0, 6) == "place:") {
> -              this._places.selectPlaceURI(container);
> +                this._places.selectPlaceURI(container);
> -            else if (container in PlacesUIUtils.leftPaneQueries)
> -              this.selectLeftPaneBuiltIn(container);
> +              } else {
> +                this._places.selectItems([container], false);

Add a comment like: "// May be a guid."

::: browser/components/places/content/tree.xml:630
(Diff revision 6)
> -            // Only follow a query if it has been been explicitly opened by the caller.
> -            let shouldOpen = aOpenContainers && PlacesUtils.nodeIsFolder(node);
> +            // Only follow a query if it has been been explicitly opened by the caller, or if we're
> +            // the "AllBookmarks" query which used to be a folder, but is now a query. We support
> +            // the "AllBookmarks" case to allow the callers to continue to specify just the top-level
> +            // bookmark folders.
> +            let shouldOpen = aOpenContainers && (PlacesUtils.nodeIsFolder(node) ||
> +              (PlacesUtils.nodeIsQuery(node) && PlacesUIUtils.leftPaneQueries.AllBookmarks));

I'm not sure I understand the second check. I understand it wants to know if it's the AllBookmarks query, but it doesn't really?

::: browser/components/places/content/treeView.js:1306
(Diff revision 6)
> +          if (node.uri.endsWith("TOOLBAR")) {
> +            properties += ` queryFolder_${PlacesUtils.bookmarks.toolbarGuid}`;
> +          } else if (node.uri.endsWith("BOOKMARKS_MENU")) {
> +            properties += ` queryFolder_${PlacesUtils.bookmarks.menuGuid}`;
> +          } else if (node.uri.endsWith("UNFILED_BOOKMARKS")) {
> +            properties += ` queryFolder_${PlacesUtils.bookmarks.unfiledGuid}`;

This shouldn't be valid for any query like place:folder=TOOLBAR/MENU/ETC. IT actually depends if their parent is AllBookmarks, since the user can create similar shortcuts in other places.
You could likely check that itemId == -1 for these items, to simplify, or maybe you could check their fake-guid?

::: browser/components/places/content/treeView.js:1783
(Diff revision 6)
>      //
>      // Note that concrete itemIds aren't used intentionally.  For example, we
>      // have no reason to disallow renaming a shortcut to the Bookmarks Toolbar,
>      // except for the one under All Bookmarks.
> -    if (PlacesUtils.nodeIsSeparator(node) || PlacesUtils.isRootItem(itemGuid))
> +    if (PlacesUtils.nodeIsSeparator(node) || PlacesUtils.isRootItem(itemGuid) ||
> +        PlacesUtils.isVirtualRootItem(itemGuid))

I'd prefer something more generic like IsQueryGeneratedFolder, and I'd prefer if it would check nodeIsFolder(node) && nodeIsQuery(node.parent).

::: browser/components/places/tests/browser/browser_library_commands.js:107
(Diff revision 6)
>                                                     title: "special_query",
>                                                     parentGuid: PlacesUtils.bookmarks.toolbarGuid,
>                                                     index: 0 });
>  
> +  let toolbarNode = PlacesUtils.asContainer(PO._places.selectedNode);
> +  toolbarNode.containerOpen = true;

why this change?
Looks like the test before was also checking auto-update since it was opening the toolbar, and then adding a query to it, with the change it will not test auto-update anymore.
I'd not want this to hide a bug

::: browser/components/places/tests/chrome/test_0_bug510634.xul:82
(Diff revision 6)
>        // Open All Bookmarks
>        tree.selectItems([PlacesUIUtils.leftPaneQueries["AllBookmarks"]]);
>        PlacesUtils.asContainer(tree.selectedNode).containerOpen = true;
> -      is(PlacesUIUtils.allBookmarksFolderId, tree.selectedNode.itemId,
> +      is(tree.selectedNode.uri,
> +         "place:type=" + Ci.nsINavHistoryQueryOptions.RESULTS_AS_ROOTS_QUERY +
> +         "&queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS,

PlacesUIUtils doesn't define queryType... how does it appear here?

::: browser/components/preferences/selectBookmark.js:22
(Diff revision 6)
>   * closes.
>   */
>  var SelectBookmarkDialog = {
>    init: function SBD_init() {
>      document.getElementById("bookmarks").place =
> -      "place:queryType=1&folder=" + PlacesUIUtils.allBookmarksFolderId;
> +      "place:queryType=1&type=" + Ci.nsINavHistoryQueryOptions.RESULTS_AS_ROOTS_QUERY;

why queryType, is it necessary?

::: browser/themes/shared/places/tree-icons.inc.css:63
(Diff revision 6)
>    list-style-image: url("chrome://browser/skin/places/folder-smart.svg");
>  }
>  
> +treechildren::-moz-tree-image(query, OrganizerQuery_AllBookmarks) {
> +  list-style-image: url("chrome://browser/skin/places/allBookmarks.png");
> +}

why move this rule down in the file? It doesn't look changed

::: toolkit/components/places/Bookmarks.jsm:157
(Diff revision 6)
>      * The GUIDs of the user content root folders that we support, for easy access
>      * as a set.
>      */
>     userContentRoots: ["toolbar_____", "menu________", "unfiled_____", "mobile______"],
>  
> +   virtualMenuGuid: "menu_______v",

This needs some kind of documentation

::: toolkit/components/places/PlacesUtils.jsm:120
(Diff revision 6)
>    data.instanceId = PlacesUtils.instanceId;
>  
>    let guid = aNode.bookmarkGuid;
> -  if (guid) {
> +  // Some nodes, e.g. the unfiled/menu/toolbar ones can have a virtual guid, so
> +  // we ignore any that are a folder shortcut. These will be handled below.
> +  if (guid && aNode.type != Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT) {

Maybe you could use !isVirtualRoot here?

::: toolkit/components/places/PlacesUtils.jsm:1220
(Diff revision 6)
> +   * Checks if a guid is a virtual root.
> +   *
> +   * @param {String} guid The guid of the item to look for.
> +   * @returns {Boolean} true if guid is a virtual root, false otherwise.
> +   */
> +  isVirtualRootItem(guid) {

Maybe we could put this into Bookmarks.jsm.
Now that Bookmarks and History are modules, we can add some dedicated utils to them and stop pushing everything to PlacesUtils

::: toolkit/components/places/nsNavHistory.cpp:141
(Diff revision 6)
>  
>  // Initial length of the recent events cache.
>  #define RECENT_EVENTS_INITIAL_CACHE_LENGTH 64
>  
> +// Places string bundle, contains internationalized bookmark root names.
> +#define PLACES_BUNDLE "chrome://places/locale/places.properties"

You can use nsNavHistory::GetBundle

::: toolkit/components/places/nsNavHistory.cpp:844
(Diff revision 6)
>    }
>  
>    if (aOptions->ResultType() ==
> -      nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY)
> +      nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY ||
> +      aOptions->ResultType() ==
> +      nsINavHistoryQueryOptions::RESULTS_AS_ROOTS_QUERY)

nit: the second part of the == should be indented once more, in both cases

::: toolkit/components/places/nsNavHistory.cpp:1949
(Diff revision 6)
> +{
> +  nsCOMPtr<nsIStringBundleService> bundleService =
> +    services::GetStringBundleService();
> +  NS_ENSURE_STATE(bundleService);
> +  nsCOMPtr<nsIStringBundle> bundle;
> +  nsresult rv = bundleService->CreateBundle(PLACES_BUNDLE, getter_AddRefs(bundle));

ditto for GetBundle

::: toolkit/components/places/nsNavHistory.cpp:1961
(Diff revision 6)
> +  rv = bundle->GetStringFromName("BookmarksToolbarFolderTitle", toolbarTitle);
> +  if (NS_FAILED(rv)) return rv;
> +  rv = bundle->GetStringFromName("BookmarksMenuFolderTitle", menuTitle);
> +  if (NS_FAILED(rv)) return rv;
> +  rv = bundle->GetStringFromName("OtherBookmarksFolderTitle", unfiledTitle);
> +  if (NS_FAILED(rv)) return rv;

There is also nsNavHistory::GetStringFromName

	Marco Bonardo [:mak]
	Comment 45 • 7 years ago
mozreview-review

Comment on attachment 8942655 [details]
Bug 1423896 - Make the new All Bookmarks folder query only update on the mobile folder status change for better performance.

https://reviewboard.mozilla.org/r/212914/#review222650

::: browser/components/places/content/places.js:136
(Diff revision 5)
> +        // To ensure the left-pane node stays open if it is refreshed, we
> +        // must call into toggleOpenState so that the state is saved in the xulstore.
> +        // This is so that we get the right state for the all bookmarks/virtual
> +        // queries which are QUERYUPDATE_COMPLEX_WITH_BOOKMARKS.
> +        let row = this._places.view.treeIndexForNode(this._places.selectedNode);
> +        this._places.view.toggleOpenState(row);

For now it will be ok, my biggest fear is that on large operations (migrations, restore) the continuous rebuilding on the left pane may slow down things quite a bit. We should file that but about having either:
a. QUERYUPDATE_PREFERENCE (but I'm not sure how to define which pref the result should listen to)
b. QUERYUPDATE_NONE, that will not register any listener, but the result can check if the root node is AllBookmarks and in such a case register a pref observer.

	Mark Banner (:standard8) Assignee
	Comment 46 • 7 years ago
mozreview-review-reply

Comment on attachment 8938396 [details]
Bug 1423896 - Make the All Bookmarks folder for the left pane of the Library a virtual query.

https://reviewboard.mozilla.org/r/209102/#review222354

> You can probably lose the parenthesis on the second branch
> 
> Off-hand this may not be correct, indeed a node can be moved even if it's a folder shortcut to a root (it could just be a shortcut the user created somewhere, and that's indeed a good test to make), the original check was to avoid potentially moving a root, that in reality cannot ever happen because we don't expose those in the UI. It was an overzealous check.
> 
> Is not enough the fact that the Library shortcuts parent is a query to take the right path earlier without this check? I mean, the operation should already be set to a copy, this is just a fallback path for a move operation.
> 
> TL;DR the added check on concreteGuid may not be necessary, we should gather the feature from the fact parent is a query.

This is an unwrapped node - so we've only got limited data. To get the parent, or even more details, we'd need to do a database query, and I don't think we want to do that here.

> I'm not sure I understand the second check. I understand it wants to know if it's the AllBookmarks query, but it doesn't really?

I'd missed out checking it against the item id, I've clarified the comment as well.

> why this change?
> Looks like the test before was also checking auto-update since it was opening the toolbar, and then adding a query to it, with the change it will not test auto-update anymore.
> I'd not want this to hide a bug

I think I left it in by mistake, tbh, I thought we'd fixed it along the way somewhere. I'm still investigating this one.

> PlacesUIUtils doesn't define queryType... how does it appear here?

It is set on line 72 within this test.

> why move this rule down in the file? It doesn't look changed

As AllBookmarks is now a query, rather than a folder, so the `treechildren::-moz-tree-image(query) {` rule would be taking precedence as it was later in the file.

	Marco Bonardo [:mak]
	Comment 52 • 7 years ago

To sum up the status, the remaining problems to look at are:
1. The bug I reported in comment 42
2. A possible issue with browser_library_commands.js 
3. We need a follow-up bug filed for the QUERYUPDATE problem in comment 45

	Marco Bonardo [:mak]
	Comment 64 • 7 years ago
mozreview-review

Comment on attachment 8938396 [details]
Bug 1423896 - Make the All Bookmarks folder for the left pane of the Library a virtual query.

https://reviewboard.mozilla.org/r/209102/#review223868

::: browser/components/places/PlacesUIUtils.jsm:521
(Diff revision 9)
>      }
>  
> +    // Is it a query pointing to one of the special root folders?
> +    if (PlacesUtils.nodeIsQuery(parentNode) && PlacesUtils.nodeIsFolder(aNode)) {
> +      let guid = PlacesUtils.getConcreteItemGuid(aNode);
> +      // If the parent folder is not a folder, it is a query, and so this node

nit: s/it is a query/it must be a query/ (I *think* it's easier to understand)

::: browser/components/places/content/places.js:81
(Diff revision 9)
> +      case "UnfiledBookmarks":
> +        this._places.selectItems([PlacesUtils.bookmarks.virtualUnfiledGuid]);
> +        // this.selectLeftPaneContainerByHierarchy([
> +        //   PlacesUIUtils.leftPaneQueries.AllBookmarks,
> +        //   PlacesUtils.bookmarks.virtualUnfiledGuid
> +        // ]);

hmm?

::: browser/components/places/content/tree.xml:626
(Diff revision 9)
>              if (ids.length == 0 || !PlacesUtils.nodeIsContainer(node) ||
>                  checkedGuidsSet.has(concreteGuid))
>                return foundOne;
>  
> -            // Only follow a query if it has been been explicitly opened by the caller.
> -            let shouldOpen = aOpenContainers && PlacesUtils.nodeIsFolder(node);
> +            // Only follow a query if it has been been explicitly opened by the caller. We support
> +            // the "AllBookmarks" case to allow callers to specify just the top-level bookmark folders.

nit: while touching this comment please wrap at ~80

::: browser/components/places/content/tree.xml:628
(Diff revision 9)
>                return foundOne;
>  
> -            // Only follow a query if it has been been explicitly opened by the caller.
> -            let shouldOpen = aOpenContainers && PlacesUtils.nodeIsFolder(node);
> +            // Only follow a query if it has been been explicitly opened by the caller. We support
> +            // the "AllBookmarks" case to allow callers to specify just the top-level bookmark folders.
> +            let shouldOpen = aOpenContainers && (PlacesUtils.nodeIsFolder(node) ||
> +              (PlacesUtils.nodeIsQuery(node) && node.itemId == PlacesUIUtils.leftPaneQueries.AllBookmarks));

Something strange here, how can we compare node.itemId with something that doesn't have an itemId?

	Marco Bonardo [:mak]
	Comment 65 • 7 years ago
mozreview-review

Comment on attachment 8942655 [details]
Bug 1423896 - Make the new All Bookmarks folder query only update on the mobile folder status change for better performance.

https://reviewboard.mozilla.org/r/212914/#review223880

I didn't try to use it yet, but I think you can simplify some of this code using Preferences::RegisterCallback and avoiding the xpcom roundtrip with a static method. I think it's worth a try.

::: toolkit/components/places/nsNavHistoryResult.cpp:3638
(Diff revision 9)
> +  int32_t existingIndex = -1;
> +  for (int32_t i = 0; i < mChildren.Count(); i++) {
> +    if (mChildren[i]->mBookmarkGuid == "mobile____v") {
> +      existingIndex = i;
> +    }
> +  }

maybe it could be a good time to introduce a FindChildByGuid, similar to FindChildById?

	Mark Banner (:standard8) Assignee
	Comment 66 • 7 years ago
mozreview-review-reply

Comment on attachment 8938396 [details]
Bug 1423896 - Make the All Bookmarks folder for the left pane of the Library a virtual query.

https://reviewboard.mozilla.org/r/209102/#review223868

> hmm?

Oops, I'd been testing something and left the old version in.

> Something strange here, how can we compare node.itemId with something that doesn't have an itemId?

At the moment, maybeRebuildLeftPane() is still creating the "AllBookmarks" query and hence still allocating it an Id. When we do the next bug, we'll need to work out something different here - I think it should be a simple switch to guid, but at the moment I'm thinking its best to let this land.

I did get rid of the allBookmarksFolderId as I thought it'd be useful to do that here at the same time.

	Marco Bonardo [:mak]
	Comment 67 • 7 years ago

(In reply to Mark Banner (:standard8) from comment #66)
> At the moment, maybeRebuildLeftPane() is still creating the "AllBookmarks"
> query and hence still allocating it an Id. When we do the next bug, we'll
> need to work out something different here - I think it should be a simple
> switch to guid, but at the moment I'm thinking its best to let this land.

ooh, I was crazy, just confused allbookmarks with its children. Nvm, my fault.

	Marco Bonardo [:mak]
	Comment 73 • 7 years ago
mozreview-review

Comment on attachment 8942655 [details]
Bug 1423896 - Make the new All Bookmarks folder query only update on the mobile folder status change for better performance.

https://reviewboard.mozilla.org/r/212914/#review224020

::: toolkit/components/places/nsNavHistoryResult.cpp:1314
(Diff revision 10)
> + * @note Does not addref the node!
> + */
> +nsNavHistoryResultNode*
> +nsNavHistoryContainerResultNode::FindChildByGuid(const nsACString& guid,
> +    uint32_t* nodeIndex)
> +{

I'd suggest to init nodeIndex to -1, so it's not uninitialized... but it's unsigned for some reason. Maybe it shouldn't be. I know the other helpers do the same, I don't think it's completely correct.

::: toolkit/components/places/nsNavHistoryResult.cpp:4885
(Diff revision 10)
>  }
> +
> +void
> +nsNavHistoryResult::OnMobilePrefChanged()
> +{
> +  ENUMERATE_MOBILE_PREF_OBSERVERS(OnMobilePrefChanged(Preferences::GetBool(MOBILE_BOOKMARKS_PREF, false)));

Alternatively, instead of doing all the forwarding from the result to the node, in this case it would be possible to just register the specific nsNavHistoryQueryResultNode for the pref change in nsNavHistoryQueryResultNode::OpenContainer and Closecontainer (to be added), when the type is RESULTS_AS_ROOTS.
The reason is that in a result there is likely only one query interested in this pref notification, while bookmark/history notification may have multiple nodes interested in them (thus the forwarding is cheaper than registering each node).
I'm fine either way, it would save some forwarding boilerplate and some CC work, but I don't want to delay this further.  In any case I think it's good to point it out to spread knowledge.

	Mark Banner (:standard8) Assignee
	Comment 74 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #73)
> ::: toolkit/components/places/nsNavHistoryResult.cpp:1314
> I'd suggest to init nodeIndex to -1, so it's not uninitialized... but it's
> unsigned for some reason. Maybe it shouldn't be. I know the other helpers do
> the same, I don't think it's completely correct.

I've updated nodeIndex to be a signed integer, and initialised to -1. I think that's much better overall.

> ::: toolkit/components/places/nsNavHistoryResult.cpp:4885
> Alternatively, instead of doing all the forwarding from the result to the
> node, in this case it would be possible to just register the specific
> nsNavHistoryQueryResultNode for the pref change in
> nsNavHistoryQueryResultNode::OpenContainer and Closecontainer (to be added),
> when the type is RESULTS_AS_ROOTS.
> The reason is that in a result there is likely only one query interested in
> this pref notification, while bookmark/history notification may have
> multiple nodes interested in them (thus the forwarding is cheaper than
> registering each node).
> I'm fine either way, it would save some forwarding boilerplate and some CC
> work, but I don't want to delay this further.  In any case I think it's good
> to point it out to spread knowledge.

I've filed bug 1436312 as a follow-up to address this.

	Pulsebot
	Comment 80 • 7 years ago

Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6401e950298d
When excluding queries within places queries, we shouldn't exclude folder shortcuts. r=mak
https://hg.mozilla.org/integration/autoland/rev/569173c95238
Rename PlacesOrganizer.selectLeftPaneQuery to selectLeftPaneBuiltIn to better reflect what it is actually selecting. r=mak
https://hg.mozilla.org/integration/autoland/rev/abf61cd5fb77
Rewrite browser_library_panel_leak.js to use async & await. r=mak
https://hg.mozilla.org/integration/autoland/rev/12239646395f
Make the All Bookmarks folder for the left pane of the Library a virtual query. r=mak
https://hg.mozilla.org/integration/autoland/rev/4012bc74e900
Make the new All Bookmarks folder query only update on the mobile folder status change for better performance. r=mak

	Cristina Coroiu [:ccoroiu]
	Comment 81 • 7 years ago

Backed out 5 changesets (bug 1423896) for bustage at /src/toolkit/components/places/nsNavHistoryResult.cpp on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4012bc74e9001ece2b50f5d02a9c60106c0aad55

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=160812984&repo=autoland&lineNumber=26145

Backout: https://hg.mozilla.org/integration/autoland/rev/280770806be77a1b648141e1edd824f86dfa9e6d

	Code Review Bot [:reviewbot]
	Comment 82 • 7 years ago
mozreview-review

Comment on attachment 8942655 [details]
Bug 1423896 - Make the new All Bookmarks folder query only update on the mobile folder status change for better performance.

https://reviewboard.mozilla.org/r/212914/#review224148


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/places/nsNavHistoryResult.cpp:3657
(Diff revision 11)
> +    return Refresh();
> +  }
> +
> +  // We're removing the mobile folder, so find it.
> +  int32_t existingIndex;
> +  nsNavHistoryResultNode* node =

Warning: Value stored to 'node' during its initialization is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

	Pulsebot
	Comment 84 • 7 years ago

Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4321514ab05f
When excluding queries within places queries, we shouldn't exclude folder shortcuts. r=mak
https://hg.mozilla.org/integration/autoland/rev/2211efa3ab47
Rename PlacesOrganizer.selectLeftPaneQuery to selectLeftPaneBuiltIn to better reflect what it is actually selecting. r=mak
https://hg.mozilla.org/integration/autoland/rev/f2d20b78861e
Rewrite browser_library_panel_leak.js to use async & await. r=mak
https://hg.mozilla.org/integration/autoland/rev/32ec5531af09
Make the All Bookmarks folder for the left pane of the Library a virtual query. r=mak
https://hg.mozilla.org/integration/autoland/rev/c388570c330f
Make the new All Bookmarks folder query only update on the mobile folder status change for better performance. r=mak

	Andrei Ciure[:aciure]
	Comment 85 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4321514ab05f
https://hg.mozilla.org/mozilla-central/rev/2211efa3ab47
https://hg.mozilla.org/mozilla-central/rev/f2d20b78861e
https://hg.mozilla.org/mozilla-central/rev/32ec5531af09
https://hg.mozilla.org/mozilla-central/rev/c388570c330f