+++ This bug was initially created as a clone of Bug #387734 +++ mockup in the bug URL, contains the list of smart folders. the smart folders are in bug 385826.

Notes from the mockup: The name of the folder has not been finalized. The specific set of pre-populated smart folders has not been finalized, but these all seem reasonable.

Bug 387734 having been marked invalid, has the placement been decided, or is there still discussion in dev.apps or where ever, or should this summary be ambiguated? (I liked the toolbar, personally)

(In reply to comment #2) > Bug 387734 having been marked invalid, has the placement been decided, or is > there still discussion in dev.apps or where ever, or should this summary be > ambiguated? (I liked the toolbar, personally) > I re-opened bug 387734, per Alex's comment that we should try both.

taking, per discussion with Dietrich.

note, because of bug #397453, there may be duplicates in the "recently visited bookmarks" and "most visited bookmarks" menus, if you use tags.

new to this patch: 1) for non folders, we clear the _built boolean each time the popup is hidden. for these pre-defined queries (which are not folders) that causes us to remove the hidden (or non hidden) "Empty" menu item when we call _cleanMenu(). we keep a reference to that menu item, so we need to clear that reference if we've now orphaned it. alternatively, we could have skipped over the call to removeChild() (when the item was the cached emptyMenu item), but this seems cleaner to me. // our calls to removeChild() may have removed the static "empty" menu // in that case, clear our reference to it. if (this._emptyMenuItem && !this._emptyMenuItem.parentNode) this._emptyMenuItem = null; 2) when livemarks are loading (or failing to load), the temporary bookmark items could show up in the "recently created" query. I'm already excluding livemark items, so I've extended this code to also exclude any temporary livemark bookmark items (where uri begins with "about:livemark-" 3) not to dietrich, a while back you suggested I use annotationIsNot to prevent showing livemark items. in this case, that won't work, as the annotation is on the folder, and not on the individual bookmark items. to prevent showing these (when excludeLivemarks is true), I re-use the query and idea from autocomplete: I build up a hash of livemark folder ids, and if the item's parent is in the hash, I exclude it when filtering. 4) note, for a new profile, the "default bookmarks" will show up as recently created, but I believe this is correct behaviour (even though the user didn't really "star" those items manually. 5) another recent change: I'm specifying minVisit=1 for the "recently visited bookmarks" and "most visited bookmarks" queries, as we don't want items that you've never visited to show up (which can happen with a new profile) or after you've cleared your private data.

"sort by count + max results to do the right thing", but that I mean that sorting by count is a container sort and so we can't stop processing items (when filtering) just because we've hit the max results. instead, we need to process them all, and then, after sorting, remove the "extra" containers from mChildren. see nsNavHistory::FilterResultSet() and nsNavHistoryQueryResultNode::FillChildren()

dietrich, my self review notes to help your review: 1) <menupopup id="bookmarksMenuPopup" type="places" asyncinit="true" - place="place:folder=2&amp;group=3&amp;expandQueries=1" + place="place:folder=2&amp;expandQueries=1" context="placesContext" group by folder (3) no longer means what it used to me, so this needed to be removed. 2) + <menuseparator/> + <menu id="places-menu" + type="menu" + container="true" + class="menu-iconic bookmark-item" + label="&placesMenu.label;" + accesskey="&placesMenu.accesskey;"> + <menupopup id="placesPopup"> + <menu id="recently-created-bookmarks-menu" + type="menu" container="true" + class="menu-iconic query-item" + label="&recentlyCreatedBookmarks.label;" + accesskey="&recentlyCreatedBookmarks.accesskey;"> + <menupopup id="recentlyCreatedBookmarksPopup" + type="places" + place="place:folder=2&amp;folder=4&amp;queryType=1&amp;sort=12&amp;excludeLivemarkItems=1&amp;maxResults=10"/> + </menu> translating that query: for the bookmark root and unfiled root, bookmark query, sort by date added descending, excluding livemark items, limit of 10 results + <menu id="recently-visited-bookmarks-menu" + type="menu" container="true" + class="menu-iconic query-item" + label="&recentlyVisitedBookmarks.label;" + accesskey="&recentlyVisitedBookmarks.accesskey;"> + <menupopup id="recentlyVisitedBookmarksPopup" + type="places" + place="place:folder=2&amp;folder=4&amp;queryType=1&amp;sort=4&amp;minVisits=1&amp;maxResults=10"/> + </menu> translating that query: for the bookmark root and unfiled root, bookmark query, sort by date (visited) descending, with a least one visit, limit of 10 results + <menu id="most-visited-bookmarks-menu" + type="menu" container="true" + class="menu-iconic query-item" + label="&mostVisitedBookmarks.label;" + accesskey="&mostVisitedBookmarks.accesskey;"> + <menupopup id="mostVisitedBookmarksPopup" + type="places" + place="place:folder=2&amp;folder=4&amp;queryType=1&amp;sort=8&amp;minVisits=1&amp;maxResults=10"/> + </menu> translating that query: for the bookmark root and unfiled root, bookmark query, sort by visit count descending, with a least one visit, limit of 10 results + <menu id="recently-used-tags-menu" + type="menu" container="true" + class="menu-iconic query-item" + label="&recentlyUsedTags.label;" + accesskey="&recentlyUsedTags.accesskey;"> + <menupopup id="recentlyUsedTagsPopup" + type="places" + place="place:folder=3&amp;group=3&amp;queryType=1&amp;sort=14&amp;maxResults=10"/> + </menu> translating that query: for the tag root, grouping by folders (the "new" way), bookmark query, sort by last modified descending, limit of 10 results fix me, what if you have more than 10 tags? + <menu id="most-used-tags-menu" + type="menu" container="true" + class="menu-iconic query-item" + label="&mostUsedTags.label;" + accesskey="&mostUsedTags.accesskey;"> + <menupopup id="mostUsedTagsPopup" + type="places" + place="place:folder=3&amp;group=3&amp;queryType=1&amp;sort=16&amp;maxResults=10"/> + </menu> translating that query: for the tag root, grouping by folders (the "new" way), bookmark query, sort by count, limit of 10 results note, see later for how "sort by count" with max results is handled. + <menu id="most-visited-sites-menu" + type="menu" container="true" + class="menu-iconic query-item" + label="&mostVisitedSites.label;" + accesskey="&mostVisitedSites.accesskey;"> + <menupopup id="mostVisitedSitesPopup" + type="places" + place="place:queryType=0&amp;sort=8&amp;maxResults=10"/> + </menu> translating that query: a history query, sort by visit count descending, limit of 10 results 3) - place="place:folder=2&amp;group=3&amp;excludeItems=1&amp;excludeQueries=1&amp;excludeReadOnlyFolders=1" + place="place:folder=2&amp;excludeItems=1&amp;excludeQueries=1&amp;excludeReadOnlyFolders=1" group by folder (3) no longer means what it used to me, so this needed to be removed. 4) -const ORGANIZER_ROOT_BOOKMARKS = "place:folder=2&group=3&excludeItems=1&queryType=1"; +const ORGANIZER_ROOT_BOOKMARKS = "place:folder=2&excludeItems=1&queryType=1"; group by folder (3) no longer means what it used to me, so this needed to be removed. 5) showRoot="true" - place="place:folder=2&amp;group=3&amp;excludeItems=1&amp;excludeQueries=1&amp;excludeReadOnlyFolders=1" + place="place:folder=2&amp;excludeItems=1&amp;excludeQueries=1&amp;excludeReadOnlyFolders=1" hidecolumnpicker="true" group by folder (3) no longer means what it used to me, so this needed to be removed. 6) + <field name="_ignoreInvalidateContainer">false</field> + part of the perf win, see bug #397117 for more details. 7) + // our calls to removeChild() may have removed the static "empty" menu + // in that case, clear our reference to it. + if (this._emptyMenuItem && !this._emptyMenuItem.parentNode) + this._emptyMenuItem = null; this is needed so that upon re-opening a non-folder menu (like a the queries I'm adding under "Places") that is empty, we will recreate the "(Empty)" item if we removed during _clearMenu() 8) + get ignoreInvalidateContainer() { + return this._ignoreInvalidateContainer; + }, + + set ignoreInvalidateContainer(val) { + this._ignoreInvalidateContainer = val; + }, + invalidateContainer: function PMV_invalidateContainer(aContainer) { + if (this._ignoreInvalidateContainer) + return; part of the perf win, see bug #397117 for more details. 9) - place="place:folder=2&amp;group=3&amp;excludeItems=1&amp;excludeReadOnlyFolders=1" + place="place:folder=2&amp;excludeItems=1&amp;excludeReadOnlyFolders=1" group by folder (3) no longer means what it used to me, so this needed to be removed. 10) + case "count": + sortingMode = aDirection == "descending" ? + NHQO.SORT_BY_COUNT_DESCENDING : NHQO.SORT_BY_COUNT_ASCENDING; + break; I've added a hidden "count" column to the places organizer. Note, at some point we'll be adding a "tags" tree to the places organizer, and we need a count column for that (According to faaborg's mockups) 11) - place="place:folder=2&amp;group=3&amp;excludeItems=1&amp;queryType=1" + place="place:folder=2&amp;excludeItems=1&amp;queryType=1" group by folder (3) no longer means what it used to me, so this needed to be removed. 12) + <splitter class="tree-splitter"/> + <treecol label="&col.count.label;" id="count" flex="1" hidden="true" + persist="width hidden ordinal sortActive sortDirection"/> I've added a hidden "count" column to the places organizer. Note, at some point we'll be adding a "tags" tree to the places organizer, and we need a count column for that (According to faaborg's mockups) 13) - + + <field name="_ignoreInvalidateContainer">false</field> + part of the perf win, see bug #397117 for more details. 14) + get ignoreInvalidateContainer() { + return this._ignoreInvalidateContainer; + }, + + set ignoreInvalidateContainer(val) { + this._ignoreInvalidateContainer = val; + }, + invalidateContainer: function TV_V_invalidateContainer(aContainer) { + if (this._ignoreInvalidateContainer) + return; + part of the perf win, see bug #397117 for more details. 15) - <!-- - Gets the nsINavHistoryResultNode adjacent to the specified InsertionPoint. - @param insertionPoint - The InsertionPoint where items are being inserted - @returns a nsINavHistoryResultNode that is adjacent to the specified - InsertionPoint - --> - <method name="_getInsertionNode"> - <parameter name="insertionPoint"/> - <body><![CDATA[ - // The InsertionPoint defines a place within the view hierarchy where - // items may be inserted. This function returns the node adjacent to - // that point. In order to get the right node, we must enumerate the - // contents of the containing folder, but we cannot construct a new - // query for this, because if we do so the node will be a member of - // another result node (that constructed by |getFolderContents| say, - // instead of the displayed tree view). So we need to walk the - // children of the result to find the right folder. - function findFolder(aItemId, aContainer) { - var wasOpen = aContainer.containerOpen; - aContainer.containerOpen = true; - var cc = aContainer.childCount; - var foundNode = null; - for (var i = 0; i < cc; ++i) { - var node = aContainer.getChild(i); - if (PlacesUtils.nodeIsFolder(node)) { - if (node.itemId == aItemId) { - foundNode = node; - break; - } - foundNode = findFolder(aItemId, node); - if (foundNode) - break; - } - } - aContainer.containerOpen = wasOpen; - return foundNode; - } - var folder = null; - var root = asContainer(this.getResultNode()); - if (PlacesUtils.nodeIsFolder(root) && - root.itemId == insertionPoint.itemId) - folder = root; - else - folder = findFolder(insertionPoint.itemId, root); - - // Since we find the folder manually, using findFolder instead of - // PlacesUtils.getFolderContents, the folder is not opened for - // us. We need to do that ourselves (and remember if it was closed, - // so that we can close it again later). - var folderWasOpen = folder.containerOpen; - folder.containerOpen = true; - - var index = insertionPoint.index; - if (insertionPoint.index == -1 || insertionPoint.index >= folder.childCount) - index = folder.childCount - 1; - NS_ASSERT(index < folder.childCount, - "index out of range: " + index + " > " + folder); - var insertionNode = index > -1 ? folder.getChild(index) : null; - - // Now close the folder again, if it was before. - folder.containerOpen = folderWasOpen; - - return insertionNode; - ]]></body> - </method> - - <!-- - Gets the tree-index of the node adjacent to the specified InsertionPoint - @param insertionPoint - The InsertionPoint where items are being inserted - @returns the tree-index of the node adjacent to the specified InsertionPoint - --> - <method name="_getInsertionIndex"> - <parameter name="insertionPoint"/> - <body><![CDATA[ - var node = this._getInsertionNode(insertionPoint); - // This is the insertion index of the pivot. - if (node) - return this.getResultView().treeIndexForNode(node); - else if (insertionPoint.orientation == NHRVO.DROP_ON) - return Ci.nsINavHistoryResultTreeViewer.INDEX_INVISIBLE; - return -1; - ]]></body> - </method> removing unused code. 16) - defaultBookmarksOptions.setGroupingMode([NHQO.GROUP_BY_FOLDER], 1); var defaultSubscriptionsOptions = history.getNewQueryOptions(); group by folder (3) no longer means what it used to me, so this needed to be removed. 17) + COLUMN_TYPE_COUNT: 10, I've added a hidden "count" column to the places organizer. Note, at some point we'll be adding a "tags" tree to the places organizer, and we need a count column for that (According to faaborg's mockups) 18) return this.COLUMN_TYPE_TAGS; + case "count": + return this.COLUMN_TYPE_COUNT; I've added a hidden "count" column to the places organizer. Note, at some point we'll be adding a "tags" tree to the places organizer, and we need a count column for that (According to faaborg's mockups) 19) case Ci.nsINavHistoryQueryOptions.SORT_BY_LASTMODIFIED_DESCENDING: return [this.COLUMN_TYPE_LASTMODIFIED, true]; + case Ci.nsINavHistoryQueryOptions.SORT_BY_COUNT_ASCENDING: + return [this.COLUMN_TYPE_COUNT, false]; + case Ci.nsINavHistoryQueryOptions.SORT_BY_COUNT_DESCENDING: + return [this.COLUMN_TYPE_COUNT, true]; I've added a hidden "count" column to the places organizer. Note, at some point we'll be adding a "tags" tree to the places organizer, and we need a count column for that (According to faaborg's mockups) 20) + get ignoreInvalidateContainer() { + return this._ignoreInvalidateContainer; + }, + + set ignoreInvalidateContainer(val) { + this._ignoreInvalidateContainer = val; + }, + invalidateContainer: function PTV_invalidateContainer(aItem) { + if (this._ignoreInvalidateContainer) + return; + part of the perf win, see bug #397117 for more details. 21) return ""; + case this.COLUMN_TYPE_COUNT: + if (!PlacesUtils.nodeIsContainer(node)) + return ""; + asContainer(node); + + // since we are just opening the container to get the proper childCount + // we can ignore the invalidateContainer() calls that happen when we + // open and close the container + this.ignoreInvalidateContainer = true; + var wasOpen = node.containerOpen; + if (!wasOpen) + node.containerOpen = true; + var count = node.childCount ? node.childCount : ""; + node.containerOpen = wasOpen; + this.ignoreInvalidateContainer = false; + return count; I've added a hidden "count" column to the places organizer. Note, at some point we'll be adding a "tags" tree to the places organizer, and we need a count column for that (According to faaborg's mockups) notice I'm using the "ignoreInvalidateContainer" for the perf win, see bug #397117 for more details. 22) + case this.COLUMN_TYPE_COUNT: + if (oldSort == NHQO.SORT_BY_COUNT_ASCENDING) + newSort = NHQO.SORT_BY_COUNT_DESCENDING; + else if (allowTriState && + oldSort == NHQO.SORT_BY_COUNT_DESCENDING) + newSort = NHQO.SORT_BY_NONE; + else + newSort = NHQO.SORT_BY_COUNT_ASCENDING; + + break; I've added a hidden "count" column to the places organizer. Note, at some point we'll be adding a "tags" tree to the places organizer, and we need a count column for that (According to faaborg's mockups) 23) + this._ignoreInvalidateContainer = false; part of the perf win, see bug #397117 for more details. 24) - options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1); options.excludeItems = aExcludeItems; options.expandQueries = aExpandQueries; group by folder (3) no longer means what it used to me, so this needed to be removed. 25) var options = this.history.getNewQueryOptions(); - options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1); var query = this.history.getNewQuery(); query.setFolders([aFolderId], 1); group by folder (3) no longer means what it used to me, so this needed to be removed. 26) style="height: 15em;" - place="place:folder=2&amp;group=3&amp;excludeQueries=1" + place="place:folder=2&amp;excludeQueries=1" hidecolumnpicker="true" group by folder (3) no longer means what it used to me, so this needed to be removed. 27) <!ENTITY viewMenu.label "View"> <!ENTITY viewMenu.accesskey "V"> <!ENTITY viewToolbarsMenu.label "Toolbars"> <!ENTITY viewToolbarsMenu.accesskey "T"> <!ENTITY viewSidebarMenu.label "Sidebar"> <!ENTITY viewSidebarMenu.accesskey "e"> <!ENTITY viewCustomizeToolbar.label "Customize..."> -<!ENTITY viewCustomizeToolbar.accesskey "C"> +<!ENTITY viewCustomizeToolbar.accesskey "C"> + +<!ENTITY placesMenu.label "Places"> +<!ENTITY placesMenu.accesskey "P"> + +<!ENTITY recentlyCreatedBookmarks.label "Recently Starred Pages"> +<!ENTITY recentlyCreatedBookmarks.accesskey "R"> +<!ENTITY recentlyVisitedBookmarks.label "Recently Visited Starred Pages"> +<!ENTITY recentlyVisitedBookmarks.accesskey "V"> +<!ENTITY mostVisitedBookmarks.label "Most Visited Starred Pages"> +<!ENTITY mostVisitedBookmarks.accesskey "M"> +<!ENTITY recentlyUsedTags.label "Recently Used Tags"> +<!ENTITY recentlyUsedTags.accesskey "U"> +<!ENTITY mostUsedTags.label "Most Used Tags"> +<!ENTITY mostUsedTags.accesskey "T"> +<!ENTITY mostVisitedSites.label "Most Visited Pages"> +<!ENTITY mostVisitedSites.accesskey "P"> new "Places" menu items and access keys 28) -<!-- XXX historyMenu entities are used with or without places (bug 336058) --> <!ENTITY historyMenu.label "History"> <!ENTITY historyMenu.accesskey "s"> <!ENTITY historyUndoMenu.label "Recently Closed Tabs"> -<!-- XXX places only --> <!ENTITY historyHomeCmd.label "Home"> -<!-- XXX remove below if places sticks --> -<!ENTITY goHomeCmd.label "Home"> -<!-- XXX remove above if places sticks --> <!ENTITY historyShowSidebarCmd.label "Show in Sidebar"> code cleanup 29) +<!ENTITY col.count.label "Count"> hidden "Count" column. 30) -<!ENTITY detailsPane.selectAnItemText.description - "Select an item to view and edit its properties"> +<!ENTITY detailsPane.selectAnItemText.description "Select an item to view and edit its properties"> code cleanup, should all be on one line. 31) +view.sortBy.count.label=Sort by Count +view.sortBy.count.accesskey=o hidden "Count" column. 32) +.query-item[container] { + list-style-image: url("chrome://browser/skin/places/query.png"); +} rules for query menu item 33) +/* ::::: query items ::::: */ + +.query-item[container] { + list-style-image: url("chrome://browser/skin/places/query.png"); +} + rules for query menu item 34) /** + * When this attribute is true, we will ignore calls to invalidateContainer() + */ + attribute boolean ignoreInvalidateContainer; + perf win. 35) /** - * Groping by exact host. The results will be an array of nsINavHistoryResults + * Grouping by exact host. The results will be an array of nsINavHistoryResults * with type = RESULT_TYPE_HOST, one for each unique host (for example, * "bugzilla.mozilla.org" and "www.mozilla.org" will be separate). The * children of these will correspond to the results for each host. */ const unsigned short GROUP_BY_HOST = 1; typo. 36) /** - * Group by bookmark folder. Since this determines the entire subtree - * hierarchy, it must be the last grouping option given. This option - * requires the query to have onlyBookmarked set, and for there to be - * at least one parent folder specified via nsINavHistoryQuery::setFolders. - * If all of the top-level results belong to a single folder, the folder will - * be omitted and its children will become the toplevel result nodes. + * Group by bookmark folder. Results from the query are grouped in folders + * containers. This option requires there to be at least one parent folder specified + * via nsINavHistoryQuery::setFolders. */ const unsigned short GROUP_BY_FOLDER = 3; group by folder is different than before. 37) - const unsigned short SORT_BY_ANNOTATION_ASCENDING = 15; - const unsigned short SORT_BY_ANNOTATION_DESCENDING = 16; + const unsigned short SORT_BY_COUNT_ASCENDING = 15; + const unsigned short SORT_BY_COUNT_DESCENDING = 16; + const unsigned short SORT_BY_ANNOTATION_ASCENDING = 17; + const unsigned short SORT_BY_ANNOTATION_DESCENDING = 18; sort by count. note, before SORT_BY_ANNOTATION_DESCENDING, which is always last. 38) /** + * This option excludes all livemark items from a bookmarks query. + * These are items which whose parent folders have the "livemark/feedURI" annotation. + * Ignored for queries over history. Defaults to false. + */ + attribute boolean excludeLivemarkItems; as mentioned previous in the bug comments, for some queries (like most recently created bookmarks) we don't want to see livemark items. 39) - rv = moveUnfiledBookmarks->BindInt32Parameter(2, mRoot); + rv = moveUnfiledBookmarks->BindInt64Parameter(2, mRoot); fix warning. 40) - // - // If you change this, change IsSimpleFolderURI which detects this string. nsCAutoString spec("place:folder="); spec.AppendInt(aFolder); - spec.AppendLiteral("&group=3"); // GROUP_BY_FOLDER return NS_NewURI(aURI, spec); } group by folder is different than before. and IsSimpleFolderURI() no longer exists. 41) +#define LMANNO_FEEDURI "livemark/feedURI" as mentioned previous in the bug comments, for some queries (like most recently created bookmarks) we don't want to see livemark items. just moving this code. 42) + // mDBGetTitleForTag + rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( + "SELECT b.title FROM moz_bookmarks a, moz_bookmarks b " + "WHERE a.id = ?1 AND b.id != ?1 AND b.fk = a.fk AND " + "a.parent IN (SELECT c.id FROM moz_bookmarks c WHERE c.parent = ?2)"), + getter_AddRefs(mDBGetTitleForTag)); + NS_ENSURE_SUCCESS(rv, rv); the query needed to get the title for a tag item 43) + // mLivemarkFeedsQuery + rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( + "SELECT annos.item_id, annos.content FROM moz_anno_attributes attrs " + "JOIN moz_items_annos annos ON attrs.id = annos.anno_attribute_id " + "WHERE attrs.name = \"" LMANNO_FEEDURI "\""), + getter_AddRefs(mLivemarkFeedsQuery)); + NS_ENSURE_SUCCESS(rv, rv); + as mentioned previous in the bug comments, for some queries (like most recently created bookmarks) we don't want to see livemark items. just moving this code. 44) + if (aOptions->ExcludeLivemarkItems()) + return PR_FALSE; + as mentioned previous in the bug comments, for some queries (like most recently created bookmarks) we don't want to see livemark items. 45) - if (groupCount != 0 || aOptions->ExcludeQueries()) + if (groupCount != 0 || aOptions->ExcludeQueries() || aOptions->ExcludeLivemarkItems()) return PR_TRUE; as mentioned previous in the bug comments, for some queries (like most recently created bookmarks) we don't want to see livemark items. 46) + case nsINavHistoryQueryOptions::SORT_BY_COUNT_ASCENDING: + // "count" of the items in a folder is not something we have in the database + // sorting is done at nsNavHistoryQueryResultNode::FillChildren + if (aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS) + break; + case nsINavHistoryQueryOptions::SORT_BY_COUNT_DESCENDING: + // "count" of the items in a folder is not something we have in the database + // sorting is done at nsNavHistoryQueryResultNode::FillChildren + if (aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS) + break; sorting by count (only valid for bookmark queries at the moment) 47) case nsINavHistoryQueryOptions::GROUP_BY_FOLDER: - // not yet supported (this code path is not reached for simple bookmark - // folder queries) - return NS_ERROR_NOT_IMPLEMENTED; + rv = GroupByFolder(aResultNode, aSource, aDest); + break; default: group by folder, thanks dietrich! 48) -const PRInt64 UNDEFINED_AGE_IN_DAYS = -1; +const PRInt64 UNDEFINED_URN_VALUE = -1; urn is no longer specific to history / age in days folder. generalizing. 49) // Create a urn (like // urn:places-persist:place:group=0&group=1&sort=1&type=1,,%28local%20files%29) // to be used to persist the open state of this container in localstore.rdf nsresult CreatePlacesPersistURN(nsNavHistoryQueryResultNode *aResultNode, - PRInt64 aAgeInDays, const nsCString& aTitle, nsCString& aURN) + PRInt64 aValue, const nsCString& aTitle, nsCString& aURN) { urn is no longer specific to history / age in days folder. generalizing. 50) aURN.Append(NS_LITERAL_CSTRING(",")); - if (aAgeInDays != UNDEFINED_AGE_IN_DAYS) - aURN.AppendInt(aAgeInDays); + if (aValue != UNDEFINED_URN_VALUE) + aURN.AppendInt(aValue); urn is no longer specific to history / age in days folder. generalizing. 51) - nsresult rv = CreatePlacesPersistURN(aResultNode, UNDEFINED_AGE_IN_DAYS, title, urn); + nsresult rv = CreatePlacesPersistURN(aResultNode, UNDEFINED_URN_VALUE, title, urn); urn is no longer specific to history / age in days folder. generalizing. 52) +// nsNavHistory::GroupByFolder +nsresult +nsNavHistory::GroupByFolder(nsNavHistoryQueryResultNode *aResultNode, + const nsCOMArray<nsNavHistoryResultNode>& aSource, + nsCOMArray<nsNavHistoryResultNode>* aDest) +{ + nsresult rv; + nsDataHashtable<nsTrimInt64HashKey, nsNavHistoryContainerResultNode*> folders; + if (!folders.Init(512)) + return NS_ERROR_OUT_OF_MEMORY; + + nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); + if (!bookmarks) + return NS_ERROR_OUT_OF_MEMORY; + + // iterate over source, creating container nodes for each + // entry's parent folder, adding to hash + for (PRInt32 i = 0; i < aSource.Count(); i ++) { + if (aSource[i]->mItemId == -1) + continue; // only bookmark nodes can be grouped by folder + + PRInt64 parentId; + rv = bookmarks->GetFolderIdForItem(aSource[i]->mItemId, &parentId); + NS_ENSURE_SUCCESS(rv, rv); + + // if parent id is not in the hash, add it + nsNavHistoryContainerResultNode* folderNode = nsnull; + if (!folders.Get(parentId, &folderNode)) { + // get parent folder title + nsAutoString title; + rv = bookmarks->GetItemTitle(parentId, title); + NS_ENSURE_SUCCESS(rv, rv); + + nsCAutoString urn; + rv = CreatePlacesPersistURN(aResultNode, parentId, NS_ConvertUTF16toUTF8(title), urn); + NS_ENSURE_SUCCESS(rv, rv); + + PRInt64 grandparentId; + rv = bookmarks->GetFolderIdForItem(parentId, &grandparentId); + NS_ENSURE_SUCCESS(rv, rv); + + PRInt64 tagRoot; + rv = bookmarks->GetTagRoot(&tagRoot); + NS_ENSURE_SUCCESS(rv, rv); + + // create parent node + // if the grandparent is the tag root, the parent is a tag container + // so use the tag container icon + folderNode = new nsNavHistoryContainerResultNode(urn, + NS_ConvertUTF16toUTF8(title), + (grandparentId == tagRoot) ? NS_LITERAL_CSTRING("chrome://mozapps/skin/places/tagContainerIcon.png") : EmptyCString(), + nsNavHistoryResultNode::RESULT_TYPE_FOLDER, + PR_TRUE, EmptyCString(), aResultNode->mOptions); + + if (!folders.Put(parentId, folderNode)) + return NS_ERROR_OUT_OF_MEMORY; + + rv = aDest->AppendObject(folderNode); + NS_ENSURE_SUCCESS(rv, rv); + } + + // add self to parent node + if (!folderNode->mChildren.AppendObject(aSource[i])) + return NS_ERROR_OUT_OF_MEMORY; + } + return NS_OK; +} dietrich's group by folder, with some cleanup, usage of CreatePlacesPersistURN(), and usage of the tag icon for the container. thanks dietrich! 53) +// - excludingLivemarkItems - // XXX only excludeQueries is supported at the moment + // XXX only excludeQueries and excludeLivemarkItems are supported at the moment PRBool excludeQueries = PR_FALSE; if (aQueryNode) { rv = aQueryNode->mOptions->GetExcludeQueries(&excludeQueries); NS_ENSURE_SUCCESS(rv, rv); } + PRBool excludeLivemarkItems = PR_FALSE; + nsTArray<PRInt64> livemarkFolders; + if (aQueryNode) { + rv = aQueryNode->mOptions->GetExcludeLivemarkItems(&excludeLivemarkItems); + NS_ENSURE_SUCCESS(rv, rv); + } + + if (excludeLivemarkItems) { + // find all the items that have the "livemark/feedURI" annotation + // and save off their item ids. when doing filtering, + // if a result's parent item id matches a saved item id, the result + // it is not really a bookmark, but a rss feed item. + mozStorageStatementScoper scope(mLivemarkFeedsQuery); + PRBool hasMore = PR_FALSE; + while (NS_SUCCEEDED(mLivemarkFeedsQuery->ExecuteStep(&hasMore)) && hasMore) { + PRInt64 feedFolderId = 0; + rv = mLivemarkFeedsQuery->GetInt64(0, &feedFolderId); + NS_ENSURE_SUCCESS(rv, rv); + livemarkFolders.AppendElement(feedFolderId); + } + } as mentioned previous in the bug comments, for some queries (like most recently created bookmarks) we don't want to see livemark items. 54) + PRBool sortByCount = (aQueryNode && + (aQueryNode->GetSortType() == nsINavHistoryQueryOptions::SORT_BY_COUNT_ASCENDING || + aQueryNode->GetSortType() == nsINavHistoryQueryOptions::SORT_BY_COUNT_DESCENDING)); sort by count is special, see below. 55) + // if we are excluding livemark items, exclude an items who's parent is a + // livemark folder or any items with a URI that begins with "about:livemark-" + if (excludeLivemarkItems && (livemarkFolders.IndexOf(parentId) != -1 || + IsTemporaryLivemarkURI(aSet[nodeIndex]->mURI))) + continue; as mentioned previous in the bug comments, for some queries (like most recently created bookmarks) we don't want to see livemark items. 56) - if (aOptions->MaxResults() > 0 && aFiltered->Count() >= aOptions->MaxResults()) + // if we are sorting by count (which is a "container" attribute, we can't stop once we've seen + // max results, because sorting by count has to happen on the container + // (after they are all created and filled) + if (aOptions->MaxResults() > 0 && aFiltered->Count() >= aOptions->MaxResults() && !sortByCount) break; } if sort by count and max results, need to process all results, sort, and then remove containers later. 57) + PRInt64 itemId = aRow->AsInt64(kGetInfoIndex_ItemId); + + // if there is no title, but we've got an item id, this might be a tag + // so attempt to get the title for this tag. + if (itemId != -1 && title.IsEmpty()) { + mozStorageStatementScoper scoper(mDBGetTitleForTag); + NS_ENSURE_SUCCESS(rv, rv); + + nsresult rv = mDBGetTitleForTag->BindInt64Parameter(0, itemId); + NS_ENSURE_SUCCESS(rv, rv); + + nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); + NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY); + + PRInt64 tagRoot; + rv = bookmarks->GetTagRoot(&tagRoot); + NS_ENSURE_SUCCESS(rv, rv); + + rv = mDBGetTitleForTag->BindInt64Parameter(1, tagRoot); + NS_ENSURE_SUCCESS(rv, rv); + + PRBool hasMore = PR_FALSE; + while (NS_SUCCEEDED(rv = mDBGetTitleForTag->ExecuteStep(&hasMore)) && hasMore) { + rv = mDBGetTitleForTag->GetUTF8String(0, title); + NS_ENSURE_SUCCESS(rv, rv); + } + } attempt to determine the title for a tag, but only if we have a itemId. 58) // +// A simple bookmarks query will result in a hierarchical tree of +// bookmark items, folders and separators. +// // Returns the folder ID if it is a simple folder query, 0 if not. - static PRInt64 GetSimpleBookmarksQueryFolder(const nsCOMArray<nsNavHistoryQuery>& aQueries, nsNavHistoryQueryOptions* aOptions) { if (aQueries.Count() != 1) return 0; + // if there are any groupings, including GROUP_BY_FOLDER, this is not a simple + // bookmarks query + PRUint32 groupCount; + const PRUint16 *groupings = aOptions->GroupingMode(&groupCount); + if (groupings) + return 0; + group by folder is different now. 59) nsresult nsNavHistory::CreateAutoCompleteQueries() { nsCString sql = NS_LITERAL_CSTRING( - "SELECT annos.item_id, annos.content FROM moz_anno_attributes attrs " - "JOIN moz_items_annos annos ON attrs.id = annos.anno_attribute_id " - "WHERE attrs.name = \"" LMANNO_FEEDURI "\";"); - nsresult rv = mDBConn->CreateStatement(sql, getter_AddRefs(mLivemarkFeedsQuery)); - NS_ENSURE_SUCCESS(rv, rv); - moving code. 60) +// nsNavHistoryContainerResultNode::SortComparison_Count* +// + +PRInt32 PR_CALLBACK nsNavHistoryContainerResultNode::SortComparison_CountLess( + nsNavHistoryResultNode* a, nsNavHistoryResultNode* b, void* closure) +{ + PRUint32 count1 = 0; + PRUint32 count2 = 0; + + if (a->IsContainer()) { + nsresult rv = a->GetAsContainer()->SetContainerOpen(PR_TRUE); + if (NS_SUCCEEDED(rv)) + rv = a->GetAsContainer()->GetChildCount(&count1); + if (NS_FAILED(rv)) + count1 = 0; + } + + if (b->IsContainer()) { + nsresult rv = b->GetAsContainer()->SetContainerOpen(PR_TRUE); + if (NS_SUCCEEDED(rv)) + rv = b->GetAsContainer()->GetChildCount(&count2); + if (NS_FAILED(rv)) + count2 = 0; + } + + PRInt32 value = CompareIntegers(count1, count2); + if (value == 0) { + value = SortComparison_StringLess(NS_ConvertUTF8toUTF16(a->mTitle), + NS_ConvertUTF8toUTF16(b->mTitle)); + if (value == 0) + value = nsNavHistoryContainerResultNode::SortComparison_Bookmark(a, b, closure); + } + return value; +} +PRInt32 PR_CALLBACK nsNavHistoryContainerResultNode::SortComparison_CountGreater( + nsNavHistoryResultNode* a, nsNavHistoryResultNode* b, void* closure) +{ + return -nsNavHistoryContainerResultNode::SortComparison_CountLess(a, b, closure); +} + when sorting by count, open container and get child count, if node is contianer. otherwise, compare title, otherwise compare as bookmark. 61) // item (because we've browsed a new page). - if (! aIsTemporary && aNode->IsContainer()) - aNode->GetAsContainer()->FillStats(); + if (! aIsTemporary && aNode->IsContainer()) { + // need to update all the new item's children + nsNavHistoryContainerResultNode* container = aNode->GetAsContainer(); + container->mResult = mResult; + container->FillStats(); + } matching the other call to fill stats, this one is called when items are insert and we're sorted. 61) + // if we are limiting our results and we are sorting by count ascending or descending, + // remove items from the end of the mChildren array after sorting. + // in this scenaro, in nsNavHistory::FilterResultSet(), we ignored maxResults so that we could + // handle it after sorting. note, if count < max results, we won't do anything. + if (mOptions->MaxResults() > 0 + (comparator == &SortComparison_CountLess || + comparator == &SortComparison_CountGreater)) { + while (mChildren.Count() > mOptions->MaxResults()) + mChildren.RemoveObjectAt(mChildren.Count() - 1); + } + sorting by count + max results is different 62) Index: toolkit/components/places/tests/bookmarks/test_bookmarks.js =================================================================== RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v retrieving revision 1.36 diff -u -8 -p -r1.36 test_bookmarks.js --- toolkit/components/places/tests/bookmarks/test_bookmarks.js 17 Sep 2007 01:42:16 -0000 1.36 +++ toolkit/components/places/tests/bookmarks/test_bookmarks.js 2 Oct 2007 23:14:49 -0000 @@ -114,20 +114,32 @@ var bmStartIndex = 1; function run_test() { // test roots do_check_true(bmsvc.placesRoot > 0); do_check_true(bmsvc.bookmarksRoot > 0); do_check_true(bmsvc.tagRoot > 0); do_check_true(bmsvc.toolbarFolder > 0); do_check_true(bmsvc.unfiledRoot > 0); + // test getFolderIdForItem() with bogus item id will throw + try { + var title = bmsvc.getFolderIdForItem(0); + do_throw("getFolderIdForItem accepted bad input"); + } catch(ex) {} + + // test getFolderIdForItem() with bogus item id will throw + try { + var title = bmsvc.getFolderIdForItem(-1); + do_throw("getFolderIdForItem accepted bad input"); + } catch(ex) {} + more tests, we should make sure that you can't bet the parent for 0 or -1. 63) var options = histsvc.getNewQueryOptions(); - options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1); group by folder is different now. 64) var options = histsvc.getNewQueryOptions(); - options.setGroupingMode([Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER], 1); group by folder is different now.

(In reply to comment #21) > 2) > + > place="place:folder=2&amp;folder=4&amp;queryType=1&amp;sort=12&amp;excludeLivemarkItems=1&amp;maxResults=10"/> > + </menu> > > translating that query: for the bookmark root and unfiled root, bookmark > query, sort by date added descending, excluding livemark items, limit of 10 > results users upgrading from previous builds/releases will likely have a different id for the unfiled root. it should probably be set at runtime. this goes for the subsequent menu items using that value as well. another option would be to put in some static "special" values for roots, which is something we've talked about for a while to address this scenario.

> + <menupopup id="recentlyUsedTagsPopup" > + type="places" > + > place="place:folder=3&amp;group=3&amp;queryType=1&amp;sort=14&amp;maxResults=10"/> > + </menu> > > translating that query: for the tag root, grouping by folders (the "new" way), > bookmark query, sort by last modified descending, limit of 10 results > > fix me, what if you have more than 10 tags? i think showing the most recently used 10 tags is fine for a start. we can put it in front of users and get feedback to see if the number needs to be tweaked. > 10) > > + case "count": > + sortingMode = aDirection == "descending" ? > + NHQO.SORT_BY_COUNT_DESCENDING : NHQO.SORT_BY_COUNT_ASCENDING; > + break; > > I've added a hidden "count" column to the places organizer. Note, at some > point we'll be adding a "tags" tree to the places organizer, and we need a > count column for that (According to faaborg's mockups) hrm, at this stage i think i'd prefer that unnecessary changes get pulled out into a separate patch for that bug. > 36) > > /** > - * Group by bookmark folder. Since this determines the entire subtree > - * hierarchy, it must be the last grouping option given. This option > - * requires the query to have onlyBookmarked set, and for there to be > - * at least one parent folder specified via nsINavHistoryQuery::setFolders. > - * If all of the top-level results belong to a single folder, the folder > will > - * be omitted and its children will become the toplevel result nodes. > + * Group by bookmark folder. Results from the query are grouped in folders s/folders/folder/ > 38) > > /** > + * This option excludes all livemark items from a bookmarks query. > + * These are items which whose parent folders have the "livemark/feedURI" > annotation. > + * Ignored for queries over history. Defaults to false. > + */ > + attribute boolean excludeLivemarkItems; > > as mentioned previous in the bug comments, for some queries (like most recently > created bookmarks) we don't want to see livemark items. are there any other use-cases for "excludeParentWithAnnotation"? that might be more generally useful than this change. but this is kind of similar to excludeQueries. hrm, i'm on the fence w/ this one. > > + case nsINavHistoryQueryOptions::SORT_BY_COUNT_ASCENDING: > + // "count" of the items in a folder is not something we have in the > database > + // sorting is done at nsNavHistoryQueryResultNode::FillChildren > + if (aOptions->QueryType() == > nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS) > + break; > + case nsINavHistoryQueryOptions::SORT_BY_COUNT_DESCENDING: > + // "count" of the items in a folder is not something we have in the > database > + // sorting is done at nsNavHistoryQueryResultNode::FillChildren > + if (aOptions->QueryType() == > nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS) > + break; > > sorting by count (only valid for bookmark queries at the moment) can you group the case statements so the comment and the code for the query-type-check only occur once? > + > + // if parent id is not in the hash, add it > + nsNavHistoryContainerResultNode* folderNode = nsnull; > + if (!folders.Get(parentId, &folderNode)) { > + // get parent folder title > + nsAutoString title; > + rv = bookmarks->GetItemTitle(parentId, title); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCAutoString urn; > + rv = CreatePlacesPersistURN(aResultNode, parentId, > NS_ConvertUTF16toUTF8(title), urn); > + NS_ENSURE_SUCCESS(rv, rv); > + > + PRInt64 grandparentId; > + rv = bookmarks->GetFolderIdForItem(parentId, &grandparentId); > + NS_ENSURE_SUCCESS(rv, rv); > + > + PRInt64 tagRoot; > + rv = bookmarks->GetTagRoot(&tagRoot); > + NS_ENSURE_SUCCESS(rv, rv); please get the tag root outside of the loop, so only done once. > + > + // create parent node > + // if the grandparent is the tag root, the parent is a tag container > + // so use the tag container icon > + folderNode = new nsNavHistoryContainerResultNode(urn, > + NS_ConvertUTF16toUTF8(title), > + (grandparentId == tagRoot) ? > NS_LITERAL_CSTRING("chrome://mozapps/skin/places/tagContainerIcon.png") : > EmptyCString(), can you make that a #define instead? > + if (excludeLivemarkItems) { > + // find all the items that have the "livemark/feedURI" annotation > + // and save off their item ids. when doing filtering, > + // if a result's parent item id matches a saved item id, the result > + // it is not really a bookmark, but a rss feed item. s/it is/is/ > 55) > > + // if we are excluding livemark items, exclude an items who's parent is a s/an// > 56) > > - if (aOptions->MaxResults() > 0 && aFiltered->Count() >= > aOptions->MaxResults()) > + // if we are sorting by count (which is a "container" attribute, we can't > stop once we've seen > + // max results, because sorting by count has to happen on the container > + // (after they are all created and filled) > + if (aOptions->MaxResults() > 0 && aFiltered->Count() >= > aOptions->MaxResults() && !sortByCount) > break; > } > > if sort by count and max results, need to process all results, sort, and then > remove containers later. ugh. not sure of a better way to do this though (that isn't trading off for an increase in complexity). > 57) > > + PRInt64 itemId = aRow->AsInt64(kGetInfoIndex_ItemId); > + > + // if there is no title, but we've got an item id, this might be a tag > + // so attempt to get the title for this tag. > + if (itemId != -1 && title.IsEmpty()) { > + mozStorageStatementScoper scoper(mDBGetTitleForTag); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsresult rv = mDBGetTitleForTag->BindInt64Parameter(0, itemId); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); > + NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY); > + > + PRInt64 tagRoot; > + rv = bookmarks->GetTagRoot(&tagRoot); > + NS_ENSURE_SUCCESS(rv, rv); this never changes and is used in a few places now, maybe should cache it? > + > + rv = mDBGetTitleForTag->BindInt64Parameter(1, tagRoot); > + NS_ENSURE_SUCCESS(rv, rv); > + > + PRBool hasMore = PR_FALSE; > + while (NS_SUCCEEDED(rv = mDBGetTitleForTag->ExecuteStep(&hasMore)) && > hasMore) { > + rv = mDBGetTitleForTag->GetUTF8String(0, title); > + NS_ENSURE_SUCCESS(rv, rv); > + } > + } > > attempt to determine the title for a tag, but only if we have a itemId. hm, why did we decide to not do this at the query level? ok, up to here. going to do another pass.

...and more importantly, per discussion with dietrich, these are now dynamically created queries under a folder inserted at the beginning of the personal toolbar. (you can get at the folder from the bookmarks, menu and the tree now, too!)

not sure if I should exclude queries (or not): + var recentlyCreatedBookmarksItem = bmsvc.insertBookmark(placesFolder, + uri("place:folder=" + bookmarksRoot + "&folder=" + unfiledRoot + + "&queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS + + "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_DESCENDING + + "&excludeLivemarkItems=1&maxResults=" + maxResults + + "&excludeQueries=1"), + bmsvc.DEFAULT_INDEX, recentlyCreatedBookmarksTitle); I added that to avoid showing the queries I'm adding programmatically, but that will also exclude any queries the user adds. dietrich, what do you think?

I think "saved searches" would not be expected in the results, so is good to have excludeQueries=1 in this case.

Comment on attachment 283483 [details] [diff] [review] includes supplimental fix for the test case, adds the missing "&&" that caused mac to fail to build (windows only warned!) >+ >+ var iosvc = Cc["@mozilla.org/network/io-service;1"]. >+ getService(Ci.nsIIOService); >+ >+ function uri(spec) { >+ return iosvc.newURI(spec, null, null); >+ } could use IO.newURI() instead. also, the initialization of these folders might be more appropriately located in initBookmarksToolbar() in browser.js, where it'll be grouped with the toolbar initialization, as well as being able to take advantage of perf changes wrt the timing of when places gets loaded (eg: i'm planning on moving the other places code in nsBrowserGlue to delayedStartup() as well). http://mxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#305

I think that having special behavior for (SORT_BY_COUNT + maxResults) makes the API more confusing. SORT_BY_COUNT already stands alone as the only sorting option that applies to containers, not items. Combine that with the contextual behavior change for maxResults and the scenario begins to seem a bit convoluted. More explicit and generalized functionality would be better, maybe something like NHQO.sortContainersByProperty=childCount and NHQO.maxContainers=10? Having a larger number of explicit query options seems preferable to having fewer options and less explicit side-effects. Or maybe we could use a remote container for that folder.

1) > users upgrading from previous builds/releases will likely have a different id > for the unfiled root. it should probably be set at runtime. this goes for the > subsequent menu items using that value as well. this is now fixed as the places folder and all the queries are generated programmatically, thanks again. 2) > make a note, folders with no count don't show up when group by folder comment added to the idl. 3) > another option would be to put in some static "special" values for roots, which > is something we've talked about for a while to address this scenario. I'll log a spin off bug about this. Note, now that I'm added the "Places" folder and the queries programmatically, this is not an issue with the code I'm adding, but it is an issue in these 8 places, where we assume folder 2 == bookmark root. C:\builds\trunk\mozilla\browser\base\content\browser-menubar.inc(378): place="place:folder=2&amp;expandQueries=1" C:\builds\trunk\mozilla\browser\components\places\content\bookmarkProperties.xul(167): place="place:folder=2&amp;excludeItems=1&amp;excludeQueries=1&amp;excludeReadOnlyFolders=1" C:\builds\trunk\mozilla\browser\components\places\content\bookmarksPanel.xul(73): place="place:folder=2&amp;queryType=1" C:\builds\trunk\mozilla\browser\components\places\content\controller.js(43):const ORGANIZER_ROOT_BOOKMARKS = "place:folder=2&excludeItems=1&queryType=1"; C:\builds\trunk\mozilla\browser\components\places\content\editBookmarkOverlay.xul(147): place="place:folder=2&amp;excludeItems=1&amp;excludeQueries=1&amp;excludeReadOnlyFolders=1" C:\builds\trunk\mozilla\browser\components\places\content\moveBookmarks.xul(72): place="place:folder=2&amp;excludeItems=1&amp;excludeReadOnlyFolders=1" C:\builds\trunk\mozilla\browser\components\places\content\places.xul(355): place="place:folder=2&amp;excludeItems=1&amp;queryType=1" C:\builds\trunk\mozilla\browser\components\preferences\selectBookmark.xul(28): place="place:folder=2&amp;excludeQueries=1" 4) >> I've added a hidden "count" column to the places organizer. Note, at some >> point we'll be adding a "tags" tree to the places organizer, and we need a >> count column for that (According to faaborg's mockups) > > hrm, at this stage i think i'd prefer that unnecessary changes get pulled out > into a separate patch for that bug. OK, I'll remove that and spin it off. 6) > - * be omitted and its children will become the toplevel result nodes. > + * Group by bookmark folder. Results from the query are grouped in folders s/folders/folder/ fixed, thanks. 7) > as mentioned previous in the bug comments, for some queries (like most recently > created bookmarks) we don't want to see livemark items. > are there any other use-cases for "excludeParentWithAnnotation"? that might be > more generally useful than this change. This sort of thing makes sense for folder annotations. The other folder annotation (besides livemarks) that we have is the "placesInternal/READ_ONLY" annotation. (And, we have an anno for which folder is the personal toolbar folder.) if we had something like "excludeItemIfParentHasAnnotation" you could use that to exclude any readonly items. Does that count as a use case? > but this is kind of similar to excludeQueries. hrm, i'm on the fence w/ this > one. right, this is similar to excludeQueries (which excludes items if uri starts with "place:") If you'd prefer a more generic "excludeItemIfParentHasAnnotation", I can implement it. I'm still on the fence, too. 8) > can you group the case statements so the comment and the code for the > query-type-check only occur once? fixed, thanks. 9) > please get the tag root outside of the loop, so only done once. fixed, thanks 10) > + folderNode = new nsNavHistoryContainerResultNode(urn, > + NS_ConvertUTF16toUTF8(title), > + (grandparentId == tagRoot) ? > NS_LITERAL_CSTRING("chrome://mozapps/skin/places/tagContainerIcon.png") : > EmptyCString(), > can you make that a #define instead? this was already const in the tagging service (implemented in js), so I've extended the interface to allow us to keep it defined in just one place (but get at it through the interface). fixed, thanks. 11) > + // it is not really a bookmark, but a rss feed item. > s/it is/is/ fixed, thanks. 12) > > + // if we are excluding livemark items, exclude an items who's parent is a > s/an// fixed, thanks. 13) > if sort by count and max results, need to process all results, sort, and then > remove containers later. > ugh. not sure of a better way to do this though (that isn't trading off for an > increase in complexity). yes, this was a tough one. we don't have "count" as a db column for folder, and I don't think we should. (it would be a nightmare to keep them up to date and it would be wasted space for non-folder items.) I couldn't think of a better way to implement this. as a consolation prize, you only pay this price when grouping by folder, sorting by count, and you've got max results, and more results than the max. 14) > + PRInt64 tagRoot; > + rv = bookmarks->GetTagRoot(&tagRoot); > + NS_ENSURE_SUCCESS(rv, rv); > this never changes and is used in a few places now, maybe should cache it? yes, good call. Especially since I'm doing this in RowToResult(), which gets called a lot. note, I can't simply cache it in nsNavHistory::Init(), as that will create the bookmark service which will initialize the bookmark service. fixed, thanks. 15) > attempt to determine the title for a tag, but only if we have a itemId. > hm, why did we decide to not do this at the query level? I started out that way, with something like: "COALESCE((SELECT b.title FROM moz_bookmarks b WHERE ....), a.title), " so that we'd get the title back all with one query. coalesce will return the first non null, so if a.title is "" (which is not null), it would not work to do: "COALESCE(a.title, (SELECT b.title FROM moz_bookmarks b WHERE ....)), " I wanted to postpone the "tag title" sub-query until we're absolutely we need it. On a related note, something I read recently about sub-queries For example, from http://katastrophos.net/andre/blog/2007/01/04/sqlite-performance-tuning-and-optimization-on-embedded-systems/ "Avoid sub-queries since they tend to create temporary tables and insertion of the intermediate results into those tables may be expensive." off topic, perf related, we should look into our sub-queries. I have a feeling some of the history sidebar perf issues might be sub-query related. 16) more changes made since your review: a) added a comment about why are excluding queries from the "recently bookmarked" query // exclude queries so that user created "saved searches" // and these queries (added automatically) are excluded b) fixed this bug in treeView.js: - if (aContainer.viewIndex < 0 && aContainer.viewIndex > this._visibleElements.length) + if (aContainer.viewIndex < 0 || aContainer.viewIndex > this._visibleElements.length) 17) TODO ITEMS: 1) how will you edit these queries if selected in the organizer? 2) log a spin off bug about "flat unfiltered list of bookmarks not possible without hackery" (like max results of 99999)" 3) log spin off bug about where we assume "folder 2 == bookmark root" 4) spin off bug about count column (and remove that from this patch) 5) log spin off bug about how we can make the "most visited pages" query fast (like we do for the history menu) 6) log spin off bug about this: when in the tree, opening up the most used tags the first time, get this error to console: I think it happens when calling open container to figure out count for most used tags, but they are not (yet) visible. ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'Trying to expand a node that is not visible' when calling method : [nsINavHistoryResultViewer::containerOpened]" nsresult: "0x8057001e (NS_ERROR _XPC_JS_THREW_STRING)" location: "JS frame :: chrome://browser/content/places/t reeView.js :: PTV_toggleOpenState :: line 1166" data: no] ************************************************************ PTV__refreshVisibleSection([object XPCWrappedNative_NoHelper])@chrome://browser/ content/places/treeView.js:305 PTV_invalidateContainer([object XPCWrappedNative_NoHelper])@chrome://browser/con tent/places/treeView.js:761 PTV_containerOpened([object XPCWrappedNative_NoHelper])@chrome://browser/content /places/treeView.js:734 containerOpen(true)@:0 PTV_toggleOpenState(8)@chrome://browser/content/places/treeView.js:1167 toggleOpenState(8)@:0 changeOpenState(8)@chrome://global/content/bindings/tree.xml:220 onxblclick([object MouseEvent])@chrome://global/content/bindings/tree.xml:984

note to dietrich, to review what changed since your last review, see: https://bugzilla.mozilla.org/attachment.cgi?oldid=283272&action=interdiff&newid=283587&headers=1

1) > could use IO.newURI() instead. fixed, thanks 2) also, the initialization of these folders might be more appropriately located in initBookmarksToolbar() in browser.js, where it'll be grouped with the toolbar initialization, as well as being able to take advantage of perf changes wrt the timing of when places gets loaded (eg: i'm planning on moving the other places code in nsBrowserGlue to delayedStartup() as well). Good call. I put them in another function in browser.js (See initPlacesDefaultQueries() in my patch) Instead of calling that from initBookmarksToolbar() [which gets called from DelayedStartup() and BrowserToolboxCustomizeDone()], I just call it from DelayedStartup() fixed, thanks. 3) <sspitzerMsgMe> did you decide if you want "exclude if parent has annotation" or "excludeLivemarkItems"? <dietrich> the first seems more generally applicable <dietrich> but that's meaningless if there aren't actual use-cases <dietrich> if the only use-case is this single one, then we might as well keep it specialized for now <sspitzerMsgMe> right, that's why I'm on the fence. <sspitzerMsgMe> but, something to think about: <sspitzerMsgMe> if we later generalize it <sspitzerMsgMe> queries created with excludeLivemarkItems would need to be supported. <sspitzerMsgMe> but if we generalize now, we don't have to worry about that. <dietrich> sspitzerMsgMe: true, sounds like that's the more forward-looking option <sspitzerMsgMe> dietrich: ok, let's do the right thing, even though we don't have a use case (yet). fixed, thanks. 4) <dietrich> so, the only question i have is about the tag title subqueries <dietrich> using subqueries only requires 1 round-trip db query (xpcom->mozstorage->sqlite->mozstorage->xpcom) <dietrich> the other way requires a separate db query for each record <sspitzerMsgMe> one of the items from mano was related to tag query <sspitzerMsgMe> he wants the query to not be over bookmarks, but over moz_places, as we are tagging uris, not bookmarks. <sspitzerMsgMe> I see what you mean about the round trip. <dietrich> so, a special query option for tags? <sspitzerMsgMe> I started down that path (see previous patches in the bug) that did just that. <sspitzerMsgMe> but, switched back to using group by folder. I could have an option, that if set, would do what you are describing: <sspitzerMsgMe> and use COALESE() in the query. <sspitzerMsgMe> but only if this new option was set. <sspitzerMsgMe> more like a boolean, "resolveNullTitles" <sspitzerMsgMe> which would do what mano wants: a sub query on moz_places <sspitzerMsgMe> what do you think? <dietrich> hm, yep, that sounds good <sspitzerMsgMe> we'll only pay the COALESCE() / subquery tax if we have to, which I'll only specify when the root folder is the tag root. fixed, thanks. see change to nsNavHistory::ConstructQueryString() and the ResolveEmptyBookmarkTitles option per mano, getting the title from moz_places (not moz_bookmarks) note, our tagging service sets the title for tagged bookmarked items to be "" (and not null).

note to dietrich: I removed IsTemporaryLivemarkURI() and the tests for about:livemark-* as they are not needed, because those temporary bookmark items are children of folders with the livemark/feedURI annotation, so we are already excluding them. I'm not sure why I thought I needed to do that, but I don't.

1) recently used tags is now a query of the items (note, not folders) under the tag root sorted by date added descending (with applyOptionsToContainers set to true). 2) when grouping by folders, we dynamically create nsNavHistoryContainerResultNodes (note, not nsNavHistoryFolderResultNodes), so we need to propagate up the max date added and last modified for sorting to work. > + // when grouping by folders, we create new nsNavHistoryContainerResultNode > + // nodes. set the date added and last modified values for that node > + // to be the greatest value from the children in that group. > + if (aSource[i]->mDateAdded > folderNode->mDateAdded) > + folderNode->mDateAdded = aSource[i]->mDateAdded; > + > + if (aSource[i]->mLastModified > folderNode->mLastModified) > + folderNode->mLastModified = aSource[i]->mLastModified; 3) < + // if we are sorting by count (which is a "container" attribute), < + // we can't stop once we've seen max results, because sorting by count has to happen < + // on the container (after they are all created and filled) < + if (aOptions->MaxResults() > 0 && aFiltered->Count() >= aOptions->MaxResults() && !sortByCount) --- > + // stop once we've seen max results > + // unless our options apply to containers, in which case we need to > + // handle max results after sorting, see FillChildren() > + if (!aOptions->ApplyOptionsToContainers() && > + aOptions->MaxResults() > 0 && > + aFiltered->Count() >= aOptions->MaxResults()) per irc discussion, no longer special casing by sort by count. implemented "applyOptionsToContainers" boolean instead. x) < + // if we are limiting our results and we are sorting by count ascending or descending, < + // remove items from the end of the mChildren array after sorting. < + // in this scenaro, in nsNavHistory::FilterResultSet(), we ignored maxResults so that we could < + // handle it after sorting. note, if count < max results, we won't do anything. < + if (mOptions->MaxResults() > 0 && < + (comparator == &SortComparison_CountLess || < + comparator == &SortComparison_CountGreater)) { --- > + // if our options apply to containers and we are limiting our results > + // remove items from the end of the mChildren array after sorting. > + // note, if count < max results, we won't do anything. > + if (mOptions->ApplyOptionsToContainers() && mOptions->MaxResults()) { per irc discussion, no longer special casing by sort by count. implemented "applyOptionsToContainers" boolean instead. 5) > hrm, at this stage i think i'd prefer that unnecessary changes get pulled out into a separate patch for that bug. per dietrich, I've removed the count column changes (note, we still have sort by count, needed for queries). also note, without this column, the ignoreInvalidateContainer is not needed. I'll log a new bug (which will block the tag-tree-in-the-places-organizer bug) about the column and refer back to a previous patch. 6) > I think that having special behavior for (SORT_BY_COUNT + maxResults) makes the > API more confusing. as discussed over irc, added "applyOptionsToContainers" boolean option. 7) > // hide the tag selector if it was previously visible > var tagsSelector = this._element("tagsSelector"); > if (!tagsSelector.collapsed) > - this._toggleTagsSelector(); > + this.toggleTagsSelector(); > } > I've fixed a problem I found with tagging: ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "this._toggleTagsSelector is not a function" {file: "chrome://browser/content/places/editBookmarkOverlay.js" line: 409}]' whe n calling method: [nsIDOMEventListener::handleEvent]" nsresult: "0x80570021 (NS _ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://glob al/content/bindings/popup.xml :: hidePopup :: line 110" data: yes] ************************************************************

last week over irc, mano asked for two things: 1) GetTitle() to get the title from history 2) and fix "recently used tags" to be a simple folder query (that uses max results) implemented in nsNavHistoryFolderResultNode() For GetTitle(), I'm getting the title from history (but in one query, per dietrich) only if resolveEmptyBookmarkTitles is true. Note, when tagging, we insert bookmarks with a title of "" (and not null). for "recently used tags" (and "most used tags"), I'm not implementing this as a simple bookmark query (and limiting results and sorting on last modified of the containers) in nsNavHistoryFolderResultNode. while it is true that inserting bookmarks updated the last modified of the tag container, so does remove item (which we'd call when untagging). so, clearning a tag from a uri would affect our last modified value, and this would not give us the expected results when doing the "recently used tags" query. as metnioned in comment #38 item 1 of this bug, I'm implementing "Recently used tags" as follows: query all the items (note, not folders) under the tag root, sort by date added descending. group by folder, and on the dynamicly created nsNavHistoryContainerResultNode, use the max date added value (of the items in the group) for the node's date added value. limit the number of container nodes to max result, since applyOptionsToContainers is set to true. additionally, nsNavHistoryFolderResultNodes have different update characteristics than nsNavHistoryContainerResultNodes (and we want the latter, with QUERYUPDATE_COMPLEX_WITH_BOOKMARKS)

> I've fixed a problem I found with tagging please ignore the change to editBookmarkOverlay.js. I've it spun off to bug #399253 (and landed it, thanks dietrich.)

dietrich: I've logged the seven spin off bugs mentioned in the comments above or during our irc conversations: 1) log spin off bug about count column and refer back to this bug. bug #399260 – add "Count" column to places organizer (for tags) 2) how will you edit these queries if selected in the organizer? bug #399261 – unable to edit pre-populated queries (using the places organizer "saved search" editor/builder) 3) log spin off bug about where we assume "folder 2 == bookmark root" bug #399264 – stop hard coding folder roots in place: urls 4) log spin off bug about how we can make the "most visited pages" query fast (like we do for the history menu) bug #399266 – improve perf of the "most visited pages" pre-defined query 5) log spin off bug about when in the tree, opening up the most used tags the first time, get this error to console. bug #399267 – when in the places organizer tree, opening up the most used tags the first time, get an error in the console 6) var maxResults = 10; // should this be a pref? bug #399268 – determine proper "max result" value for the pre-defined queries 7) log a spin off bug about "flat unfiltered list of bookmarks not possible without hackery" (like max results of 99999)" bug #399270 – query that results in a flat unfiltered list of bookmarks is not possible (without hackery)

Comment on attachment 284251 [details] [diff] [review] updated to the trunk (and excluding fix for bug #399253) >+ try { >+ var bundleService = Cc["@mozilla.org/intl/stringbundle;1"]. >+ getService(Ci.nsIStringBundleService); >+ var placesBundle = bundleService.createBundle("chrome://browser/locale/places/places.properties"); >+ var placesFolderTitle = placesBundle.GetStringFromName("placesFolderTitle"); >+ var recentlyCreatedBookmarksTitle = placesBundle.GetStringFromName("recentlyCreatedBookmarksTitle"); >+ var recentlyVisitedBookmarksTitle = placesBundle.GetStringFromName("recentlyVisitedBookmarksTitle"); >+ var mostVisitedBookmarksTitle = placesBundle.GetStringFromName("mostVisitedBookmarksTitle"); >+ var recentlyUsedTagsTitle = placesBundle.GetStringFromName("recentlyUsedTagsTitle"); >+ var mostUsedTagsTitle = placesBundle.GetStringFromName("mostUsedTagsTitle"); >+ var mostVisitedSitesTitle = placesBundle.GetStringFromName("mostVisitedSitesTitle"); >+ see PlacesUtils.getString >+ var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]. >+ getService(Ci.nsINavBookmarksService); >+ >+ var bookmarksRoot = bmsvc.bookmarksRoot; >+ var unfiledRoot = bmsvc.unfiledRoot; >+ var tagRoot = bmsvc.tagRoot; >+ PlacesUtils.bookmarksRootId/unfiledRootId/tagRootId >+ var placesFolder = bmsvc.createFolder(bmsvc.toolbarFolder, placesFolderTitle, >+ 0 /* index = 0, make it the first folder */); >+ >+ var maxResults = 10; // should this be a pref? >+ >+ // exclude queries so that user created "saved searches" >+ // and these queries (added automatically) are excluded >+ var recentlyCreatedBookmarksItem = bmsvc.insertBookmark(placesFolder, >+ IO.newURI("place:folder=" + bookmarksRoot + "&folder=" + unfiledRoot + >+ "&queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS + >+ "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_DESCENDING + >+ "&excludeItemIfParentHasAnnotation=livemark%2FfeedURI" + >+ "&maxResults=" + maxResults + >+ "&excludeQueries=1"), Any chance you could create queries here and then do queriesToQueryString? >+ bmsvc.DEFAULT_INDEX, recentlyCreatedBookmarksTitle); >+ >+ var recentlyVisitedBookmarksItem = bmsvc.insertBookmark(placesFolder, >+ IO.newURI("place:folder=" + bookmarksRoot + "&folder=" + unfiledRoot + >+ "&queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS + >+ "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING + >+ "&minVisits=1&maxResults=" + maxResults), >+ bmsvc.DEFAULT_INDEX, recentlyVisitedBookmarksTitle); >+ >+ var mostVisitedBookmarksItem = bmsvc.insertBookmark(placesFolder, >+ IO.newURI("place:folder=" + bookmarksRoot + "&folder=" + unfiledRoot + >+ "&queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS + >+ "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_VISITCOUNT_DESCENDING + >+ "&minVisits=1&maxResults=" + maxResults), >+ bmsvc.DEFAULT_INDEX, mostVisitedBookmarksTitle); >+ >+ var recentlyUsedTagsItem = bmsvc.insertBookmark(placesFolder, >+ IO.newURI("place:folder=" + tagRoot + >+ "&group=" + Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER + >+ "&queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS + >+ "&applyOptionsToContainers=1" + >+ "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_DATEADDED_DESCENDING + >+ "&resolveEmptyBookmarkTitles=1" + >+ "&maxResults=" + maxResults), >+ bmsvc.DEFAULT_INDEX, recentlyUsedTagsTitle); >+ >+ var mostUsedTagsItem = bmsvc.insertBookmark(placesFolder, >+ IO.newURI("place:folder=" + tagRoot + >+ "&group=" + Ci.nsINavHistoryQueryOptions.GROUP_BY_FOLDER + >+ "&queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS + >+ "&applyOptionsToContainers=1" + >+ "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_COUNT_DESCENDING + >+ "&resolveEmptyBookmarkTitles=1" + >+ "&maxResults=" + maxResults), >+ bmsvc.DEFAULT_INDEX, mostUsedTagsTitle); >+ >+ var mostVisitedSitesItem = bmsvc.insertBookmark(placesFolder, >+ IO.newURI("place:queryType=" + Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY + >+ "&sort=" + Ci.nsINavHistoryQueryOptions.SORT_BY_VISITCOUNT_DESCENDING + >+ "&maxResults=" + maxResults), >+ bmsvc.DEFAULT_INDEX, mostVisitedSitesTitle); >+ } >+ catch(ex) { >+ dump(ex); >+ } finally { >+ prefBranch.setBoolPref("browser.places.createdDefaultQueries", true); >+ } >+} >+ Also, shouldn't this all live in nsBrowserGlue? >+ // our calls to removeChild() may have removed the static "empty" menu >+ // in that case, clear our reference to it. >+ if (this._emptyMenuItem && !this._emptyMenuItem.parentNode) >+ this._emptyMenuItem = null; where do we remove the emptyMenuItem? we should rather fix that. >Index: browser/components/places/content/tree.xml >=================================================================== hrm? >Index: browser/components/places/content/treeView.js >=================================================================== >@@ -367,17 +367,20 @@ PlacesTreeView.prototype = { > this._tree.rowCountChanged(startReplacement, -replaceCount); > if (newElements.length) > this._tree.rowCountChanged(startReplacement, newElements.length); > this._tree.endUpdateBatch(); > > // now, open any containers that were persisted > for (var i = 0; i < toOpenElements.length; i++) { > var item = asContainer(toOpenElements[i]); >- item.containerOpen = !item.containerOpen; >+ if (PlacesUtils.nodeIsQuery(item)) >+ dump("don't open any query containers, as they might be recursive\n"); hrm, so you cannot keep the smart folders in the bookmarks sidebar open? :-/

I would prefer not to persist the open state of containers which are _children_ of query containers.

I also don't like the ResolveEmptyBookmarkTitles option, couldn't you distinguish void titles (and fix the tagging service to pass null when adding items)?.

1) > see PlacesUtils.getString Fixed, thanks. 2) > PlacesUtils.bookmarksRootId/unfiledRootId/tagRootId Fixed, thanks. 3) > Any chance you could create queries here and then do queriesToQueryString? Can you elaborate on why that would be better? 4) > Also, shouldn't this all live in nsBrowserGlue? See comment #30 (from dietrich) about why this lives in browser.js and not nsBrowserGlue.js 5) >+ // our calls to removeChild() may have removed the static "empty" menu >+ // in that case, clear our reference to it. >+ if (this._emptyMenuItem && !this._emptyMenuItem.parentNode) >+ this._emptyMenuItem = null; > where do we remove the emptyMenuItem? we should rather fix that. _cleanMenu() in menu.xml. fixed, thanks. 6) >Index: browser/components/places/content/tree.xml >=================================================================== hrm? - <method name="_getInsertionNode"> - <method name="_getInsertionIndex"> These methods were unused, so I removed them. 7) >- item.containerOpen = !item.containerOpen; >+ if (PlacesUtils.nodeIsQuery(item)) >+ dump("don't open any query containers, as they might be recursive\n"); > hrm, so you cannot keep the smart folders in the bookmarks sidebar open? That is correct. > I would prefer not to persist the open state of containers which are _children_ > of query containers. I've fixed it to do that instead (and removed the dump), thanks. 8) I've also added back support for ignoreInvalidateContainer (see bug #397117) and fixed SortComparison_CountLess() to use it. 9) mano wrote: "I also don't like the ResolveEmptyBookmarkTitles option, couldn't you distinguish void titles (and fix the tagging service to pass null when adding items)?." Investigating that now.

> Investigating that now. 1) fix the tagging service to specify null instead of "" for the title 2) fix InsertBookmark() to build null parameter (if title is void) 3) fix the query in ConstructQueryString() [no need to use NULLIF, just use COALESCE] 4) rename resolveEmptyBookmarkTitles to resolveNullBookmarkTitles. note, any uris tagged before this change to the tagging service will show up without titles under "Most Used Tags" and "Most Recently Used Tags" because their titles are "" and not null, and I've removed the NULLIF.

Comment on attachment 284366 [details] [diff] [review] updated patch (but would also accept review from mano)

What's the use case for resolveNullBookmarkTitles=false?

+ // don't open any children of query containers + // to avoid recursively opening containers + if (!parent || !PlacesUtils.nodeIsQuery(parent)) hrm, actually, this might regress the history sidebar (the previous approach likely did too).

> What's the use case for resolveNullBookmarkTitles=false? the COALESCE and more importantly the sub-query to find the bookmark's title from the moz_places table is not free. by default (resolveNullBookmarkTitles=false) we don't do that work. > hrm, actually, this might regress the history sidebar (the previous approach > likely did too). you are right, it does.

+ // don't open any children of query containers + // to avoid recursively opening containers + if (!parent || !PlacesUtils.nodeIsQuery(parent)) currently, I'm doing: place:folder=2&folder=4&queryType=1&sort=12&excludeItemIfParentHasAnnotation=livemark%2FfeedURI&maxResults=10&excludeQueries=1 by excluding queries, recursion doesn't happen. but if you create a similar bookmark without "excludeQueries=1", you can run into recursion. ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "too much recursion" {file: "chrome://browser /content/places/treeView.js" line: 749}]' when calling method: [nsINavHistoryRes ultViewer::containerOpened]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERR OR_WITH_DETAILS)" location: "JS frame :: chrome://browser/content/places/treeVi ew.js :: PTV__refreshVisibleSection :: line 380" data: yes] ************************************************************

perhaps the right solution is not to see if the parent is a query, but if any ancestor is the same item, and if so, stop. working on that now.

Comment on attachment 284388 [details] [diff] [review] updated patch, prevent recursion but don't break history sidebar (and allow for persistance of container state for queries that don't recurse) would gladly take r from mano or dietrich, who have both eyeballed this pig.

Comment on attachment 284388 [details] [diff] [review] updated patch, prevent recursion but don't break history sidebar (and allow for persistance of container state for queries that don't recurse) >Index: browser/components/places/content/toolbar.xml >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/places/content/toolbar.xml,v >retrieving revision 1.104 >diff -u -8 -p -r1.104 toolbar.xml >--- browser/components/places/content/toolbar.xml 22 Sep 2007 22:54:42 -0000 1.104 >+++ browser/components/places/content/toolbar.xml 11 Oct 2007 00:14:37 -0000 >@@ -133,17 +133,19 @@ > <field name="_dropIndicatorBar">document.getAnonymousElementByAttribute(this, "class", "toolbar-drop-indicator-bar")</field> > <field name="_chevron">document.getAnonymousElementByAttribute(this, "class", "chevron")</field> > > <field name="_selection">null</field> > > <field name="_openedMenuButton">null</field> > > <field name="_result">null</field> >- >+ >+ <field name="_ignoreInvalidateContainer">false</field> >+ What's this for? The field used inside _viewer is set on the viewer object itself. > <!-- nsIPlacesView --> > <method name="getResult"> > <body><![CDATA[ > return this._result; > ]]></body> > </method> > > <!-- nsIPlacesView --> >@@ -571,17 +573,28 @@ > containerOpened: function TV_V_containerOpened(aNode) { > this.invalidateContainer(aNode); > }, > > containerClosed: function TV_V_containerClosed(aNode) { > this.invalidateContainer(aNode); > }, > >+ get ignoreInvalidateContainer() { >+ return this._ignoreInvalidateContainer; i.e. this. != this._self. >+ }, >+ >+ set ignoreInvalidateContainer(val) { >+ this._ignoreInvalidateContainer = val; nit: return this._ignoreInvalidateContainer = val; >Index: browser/components/places/content/treeView.js >=================================================================== >- invalidateContainer: function PTV_invalidateContainer(aItem) { >+ get ignoreInvalidateContainer() { >+ return this._ignoreInvalidateContainer; >+ }, >+ >+ set ignoreInvalidateContainer(val) { >+ this._ignoreInvalidateContainer = val; >+ }, >+ ditto.

Comment on attachment 284388 [details] [diff] [review] updated patch, prevent recursion but don't break history sidebar (and allow for persistance of container state for queries that don't recurse) >Index: toolkit/components/places/public/nsINavHistoryService.idl >=================================================================== > /** > * Allows clients to observe what is happening to a result as it updates itself > * according to history and bookmark system events. Register this observer on a > * result using registerView > * > * @see nsINavHistoryResult for where this fits in > */ >-[scriptable, uuid(2a709a8e-34c3-4c80-b559-789b99a4bbe6)] >+[scriptable, uuid(39c6b2d5-c061-4f2f-a068-235d02dadc91)] > interface nsINavHistoryResultViewer : nsISupports > { > /** > * Called when 'item' is inserted into 'parent' at index 'newIndex'. The item > * previously at index (if any) and everything below it will have been > * shifted down by one. The item may be a container or a leaf. > */ > void itemInserted(in nsINavHistoryContainerResultNode parent, >@@ -521,16 +521,21 @@ interface nsINavHistoryResultViewer : ns > > /** > * Called after a container node went from opened to closed. This will be > * called for the topmost container that is closing, and implies that any > * child containers have closed as well. > */ > void containerClosed(in nsINavHistoryContainerResultNode item); > >+ /** >+ * When this attribute is true, we will ignore calls to invalidateContainer() >+ */ >+ attribute boolean ignoreInvalidateContainer; >+ Please file a bug on figuring out a better solution for this use case. > /** >+ * This option excludes items from a bookmarks query >+ * if the parent of the item has this annotation. >+ * an example is to exclude livemark items nit: a/an/An >+ * (parent folders have the "livemark/feedURI" annotation) >+ >+ /** >+ * if a bookmark title is null (note, not empty), >+ * attempt to use moz_places to resolve it nit: s/moz_places to resolve it/the history title/ maybe? >+ */ >+ attribute boolean resolveNullBookmarkTitles; >+ please note here that it's disabled by default. >+ /** >+ * only apply our sort options to the containers >+ */ >+ attribute boolean applyOptionsToContainers; > s/options/sort/ maybe? >Index: toolkit/components/places/public/nsITaggingService.idl >=================================================================== > /** > * Retrieves all tags used to tag URIs in the data-base (sorted by name). > */ > readonly attribute nsIVariant allTags; >+ >+ /** >+ * Retrieves the URL spec for the tag container icon . r=mano otherwise.

1) >+ >+ <field name="_ignoreInvalidateContainer">false</field> >+ > What's this for? The field used inside _viewer is set on the viewer object > itself. >+ get ignoreInvalidateContainer() { >+ return this._ignoreInvalidateContainer; > i.e. this. != this._self. I'm using it as the default value. But, as you pointed out, I should have been using this._self. (and not this.) elsewhere in menu.xml and toolbar.xml. fixed, thanks. 2) >+ set ignoreInvalidateContainer(val) { >+ this._ignoreInvalidateContainer = val; > nit: return this._ignoreInvalidateContainer = val; fixed, thanks. 3) >+ set ignoreInvalidateContainer(val) { >+ this._ignoreInvalidateContainer = val; >+ }, >+ > ditto. fixed, thanks. 4) >+ /** >+ * When this attribute is true, we will ignore calls to invalidateContainer() >+ */ >+ attribute boolean ignoreInvalidateContainer; >+ > Please file a bug on figuring out a better solution for this use case. Will do. 5) > /** >+ * This option excludes items from a bookmarks query >+ * if the parent of the item has this annotation. >+ * an example is to exclude livemark items > nit: a/an/An fixed, thanks. 6) >+ >+ /** >+ * if a bookmark title is null (note, not empty), >+ * attempt to use moz_places to resolve it > nit: s/moz_places to resolve it/the history title/ maybe? fixed, thanks. 7) >+ */ >+ attribute boolean resolveNullBookmarkTitles; >+ > please note here that it's disabled by default. note added, thanks. 8) >+ /** >+ * only apply our sort options to the containers >+ */ >+ attribute boolean applyOptionsToContainers; > s/options/sort/ maybe? Note, max results and sort (which are query options) apply to containers, so I left it as is, but changed the comment to: * only apply our query options to the containers

fixed. Checking in browser/base/content/browser-menubar.inc; /cvsroot/mozilla/browser/base/content/browser-menubar.inc,v <-- browser-menuba r.inc new revision: 1.117; previous revision: 1.116 done Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.867; previous revision: 1.866 done Checking in browser/components/places/content/bookmarkProperties.xul; /cvsroot/mozilla/browser/components/places/content/bookmarkProperties.xul,v <-- bookmarkProperties.xul new revision: 1.29; previous revision: 1.28 done Checking in browser/components/places/content/controller.js; /cvsroot/mozilla/browser/components/places/content/controller.js,v <-- control ler.js new revision: 1.182; previous revision: 1.181 done Checking in browser/components/places/content/editBookmarkOverlay.xul; /cvsroot/mozilla/browser/components/places/content/editBookmarkOverlay.xul,v <- - editBookmarkOverlay.xul new revision: 1.7; previous revision: 1.6 done Checking in browser/components/places/content/menu.xml; /cvsroot/mozilla/browser/components/places/content/menu.xml,v <-- menu.xml new revision: 1.86; previous revision: 1.85 done Checking in browser/components/places/content/moveBookmarks.xul; /cvsroot/mozilla/browser/components/places/content/moveBookmarks.xul,v <-- mov eBookmarks.xul new revision: 1.7; previous revision: 1.6 done Checking in browser/components/places/content/places.xul; /cvsroot/mozilla/browser/components/places/content/places.xul,v <-- places.xul new revision: 1.91; previous revision: 1.90 done Checking in browser/components/places/content/toolbar.xml; /cvsroot/mozilla/browser/components/places/content/toolbar.xml,v <-- toolbar.x ml new revision: 1.105; previous revision: 1.104 done Checking in browser/components/places/content/tree.xml; /cvsroot/mozilla/browser/components/places/content/tree.xml,v <-- tree.xml new revision: 1.83; previous revision: 1.82 done Checking in browser/components/places/content/treeHelpers.js; /cvsroot/mozilla/browser/components/places/content/treeHelpers.js,v <-- treeHe lpers.js new revision: 1.9; previous revision: 1.8 done Checking in browser/components/places/content/treeView.js; /cvsroot/mozilla/browser/components/places/content/treeView.js,v <-- treeView. js new revision: 1.17; previous revision: 1.16 done Checking in browser/components/places/content/utils.js; /cvsroot/mozilla/browser/components/places/content/utils.js,v <-- utils.js new revision: 1.71; previous revision: 1.70 done Checking in browser/components/preferences/selectBookmark.xul; /cvsroot/mozilla/browser/components/preferences/selectBookmark.xul,v <-- selec tBookmark.xul new revision: 1.7; previous revision: 1.6 done Checking in browser/locales/en-US/chrome/browser/browser.dtd; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v <-- browse r.dtd new revision: 1.74; previous revision: 1.73 done Checking in browser/locales/en-US/chrome/browser/places/places.dtd; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v <-- places.dtd new revision: 1.36; previous revision: 1.35 done Checking in browser/locales/en-US/chrome/browser/places/places.properties; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v <-- places.properties new revision: 1.30; previous revision: 1.29 done Checking in browser/themes/pinstripe/browser/browser.css; /cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v <-- browser.cs s new revision: 1.87; previous revision: 1.86 done Checking in browser/themes/winstripe/browser/browser.css; /cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v <-- browser.cs s new revision: 1.107; previous revision: 1.106 done Checking in toolkit/components/places/public/nsINavHistoryService.idl; /cvsroot/mozilla/toolkit/components/places/public/nsINavHistoryService.idl,v <- - nsINavHistoryService.idl new revision: 1.69; previous revision: 1.68 done Checking in toolkit/components/places/public/nsITaggingService.idl; /cvsroot/mozilla/toolkit/components/places/public/nsITaggingService.idl,v <-- nsITaggingService.idl new revision: 1.7; previous revision: 1.6 done Checking in toolkit/components/places/src/nsNavBookmarks.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v <-- nsNavB ookmarks.cpp new revision: 1.122; previous revision: 1.121 done Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHis tory.cpp new revision: 1.177; previous revision: 1.176 done Checking in toolkit/components/places/src/nsNavHistory.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v <-- nsNavHisto ry.h new revision: 1.107; previous revision: 1.106 done Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v < -- nsNavHistoryAutoComplete.cpp new revision: 1.24; previous revision: 1.23 done Checking in toolkit/components/places/src/nsNavHistoryQuery.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp,v <-- nsN avHistoryQuery.cpp new revision: 1.34; previous revision: 1.33 done Checking in toolkit/components/places/src/nsNavHistoryQuery.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryQuery.h,v <-- nsNav HistoryQuery.h new revision: 1.17; previous revision: 1.16 done Checking in toolkit/components/places/src/nsNavHistoryResult.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v <-- ns NavHistoryResult.cpp new revision: 1.116; previous revision: 1.115 done Checking in toolkit/components/places/src/nsNavHistoryResult.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v <-- nsNa vHistoryResult.h new revision: 1.47; previous revision: 1.46 done Checking in toolkit/components/places/src/nsTaggingService.js; /cvsroot/mozilla/toolkit/components/places/src/nsTaggingService.js,v <-- nsTag gingService.js new revision: 1.4; previous revision: 1.3 done Checking in toolkit/components/places/tests/bookmarks/test_bookmarks.js; /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v <-- test_bookmarks.js new revision: 1.37; previous revision: 1.36 done RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_sort_b y_count.js,v done Checking in toolkit/components/places/tests/bookmarks/test_sort_by_count.js; /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_sort_by_count.js ,v <-- test_sort_by_count.js initial revision: 1.1 done Checking in toolkit/components/places/tests/chrome/test_371798.xul; /cvsroot/mozilla/toolkit/components/places/tests/chrome/test_371798.xul,v <-- test_371798.xul new revision: 1.7; previous revision: 1.6 done RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_331487.js,v done Checking in toolkit/components/places/tests/unit/test_331487.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_331487.js,v <-- tes t_331487.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_exclude_liv emarks.js,v done Checking in toolkit/components/places/tests/unit/test_exclude_livemarks.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_exclude_livemarks.js, v <-- test_exclude_livemarks.js initial revision: 1.1 done Checking in toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_nsINavHistoryViewer.j s,v <-- test_nsINavHistoryViewer.js new revision: 1.2; previous revision: 1.1 done RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_resolveNull BookmarkTitles.js,v done Checking in toolkit/components/places/tests/unit/test_resolveNullBookmarkTitles. js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_resolveNullBookmarkTi tles.js,v <-- test_resolveNullBookmarkTitles.js initial revision: 1.1 done Checking in toolkit/components/places/tests/unit/test_result_sort.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_result_sort.js,v <-- test_result_sort.js new revision: 1.7; previous revision: 1.6 done Checking in toolkit/components/places/tests/unit/test_tagging.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_tagging.js,v <-- te st_tagging.js new revision: 1.5; previous revision: 1.4 done Checking in toolkit/themes/winstripe/mozapps/jar.mn; /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/jar.mn,v <-- jar.mn new revision: 1.21; previous revision: 1.20 done

(In reply to comment #1) > Created an attachment (id=272137) [details] > Mockup of the proposed bookmarks menu Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007101100 Minefield/3.0a9pre ID:2007101100 "Places" is in "Bookmarks Toolbar Folder". according to mockup image, it is under "Bookmarks", not in "Bookmarks Toolbar Folder". is this intended ?

When I cut the Places folder to another place the new folder is not functional. When I cut it back it stays dysfunctional. No way to get it back.

dev-doc-needed keyword added for: behavior change to: nsINavHistoryQueryOptions::GROUP_BY_FOLDER http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/public/nsINavHistoryService.idl#975 new to nsINavHistoryQueryOptions: + const unsigned short SORT_BY_COUNT_ASCENDING = 15; + const unsigned short SORT_BY_COUNT_DESCENDING = 16; + const unsigned short SORT_BY_ANNOTATION_ASCENDING = 17; + const unsigned short SORT_BY_ANNOTATION_DESCENDING = 18; + attribute AUTF8String excludeItemIfParentHasAnnotation; http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/public/nsINavHistoryService.idl#1092 + attribute boolean resolveNullBookmarkTitles; http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/public/nsINavHistoryService.idl#1140 + attribute boolean applyOptionsToContainers; http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/public/nsINavHistoryService.idl#1145

pal-moz asked: > is this intended ? yes, at least, it was intentional on my part. see also bug #387734 where we wanted to add these pre-defined queries to the personal toolbar as well. (see faaborg's comment #5 in that bug) additionally, faaborg, dietrich and I wanted the folder and the items to be deletable and modifiable. all that said, the precise location isn't set in stone. if you think it should be somewhere else, let's start a new bug.

>+ attribute boolean ignoreInvalidateContainer; > Please file a bug on figuring out a better solution for this use case. filed bug #399472

> When I cut the Places folder to another place the new folder is not functional. > When I cut it back it stays dysfunctional. No way to get it back. I'ved started a new bug on this, see bug #399473. The problem appears to be that we convert queries to folders.

I'm seeing results in all the 'Starred' categories, although I have never starred a bookmark. Ever. And it;s an old profile, without the default bookmarks. Could it be because of this (in places.properties) ? recentlyCreatedBookmarksTitle=Recently Starred Pages recentlyVisitedBookmarksTitle=Recently Visited Starred Pages mostVisitedBookmarksTitle=Most Visited Starred Pages I can't imagine that nobody saw this :-)

> I'm seeing results in all the 'Starred' categories, > although I have never starred a bookmark Jo, that's intentional, as starred == bookmarked (and not starred == unfiled bookmarks.) You should notice that when you load a bookmarked page, the star is in the url bar, too. (and it would be starred in the url bar autocomplete results, too.) the wording of these queries isn't set in stone, and i'll log a spin off bug on faaborg to decide on the wording.

re-opening I've disabled this feature due to Ts, Txul, and RLk regressions, see bug #399418 and bug #399476 I've disabled it by setting the default pref to be true in browser/app/profile/firefox.js: +// disable adding the "Places" folder with pre-defined queries +pref("browser.places.createdDefaultQueries", true);

Removing doc needed keyword; this interface has not been documented at all and is already slated to be by the doc plan for Firefox 3 so this bug being tagged with that keyword is extraneous.

I was surprised when this was checked in and I work in Places as a tester on most days. Other than the screenshot in this bug, was this work documented somewhere? I'm wondering if there are other changes coming that I'm somehow unaware of before they suddenly appear. :-)

> Other than the screenshot in this bug, was this work documented somewhere? Al, it was documented here http://wiki.mozilla.org/Places:User_Interface/I6. It was not linked off the meta "Places UI Design" bug (see bug #393529), so I've just done that.

Thanks for adding it there. I do wonder about an area as complex as Places being documented only in a series of screenshots but I'll leave that for another day.

fwiw, this has been up for months, and is linked to in the Places section of the weekly Fx meeting wiki: http://wiki.mozilla.org/Places:Fx3UIPlan

Reopening until the feature is re-enabled.

I've re-enabled the feature, and I'll report back with the new RLk, Ts and Txul numbers.

after watching tinderbox, the cost seems to be zero new refcount leaks, approximately 10ms cost to Ts and approximately 10 ms to Txul. see bug #399476 comment #18 for full details. marking fixed, as the the feature is re-enabled, but will consult with perf gurus about if a 10 ms cost is too high.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007101304 Minefield/3.0a9pre ID:2007101304 with new/clean profile. "browser.places.createdDefaultQueries" is set to "true". see http://img524.imageshack.us/img524/8646/abcfq3.jpg according to bonsai, this is set/changed to "false". this should be "false" ?

(In reply to comment #64) > all that said, the precise location isn't set in stone. if you think it should > be somewhere else, let's start a new bug. Is there already a new bug on this topic? As the content of Bookmarks Toolbar is normally user defined there should be at least the possibility to deactivate the Places folder in Bookmarks Toolbar or much better the possibility to put in another destination via customizing toolbars.

over irc, mano points out some useless cruft in one of my queries: from nsNavHistory::ConstructQueryString() "COALESCE(b.title, " "(SELECT h.title FROM moz_bookmarks b2, moz_places h2 WHERE b2.fk = h2.id AND b2.id = b.id)), "); there is no reason for the sub select. originally, when I was trying to find the title for the tag in bookmarks, I was selecting b2.title. mano has a patch for this in another bug. thanks for catching my mistake.

note the wording has changed, see bug #399477

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