User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre Middle clicking on a tag in the Library cause the current tab to become blank Reproducible: Always Steps to Reproduce: 1. Open the Library (Bookmarks > Organize Bookmarks or Ctrl+Shift+B) 2. Middle click on a tag Actual Results: The current tab in the browser becomes blank. Expected Results: Items in the tag are opened as tabs like middle clicking on a folder. The back button still works on the blank tab.

confirmed Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008042806 Minefield/3.0pre do we have a regression range?

I see it only in the left pane. Since Bug 419731 you can expand "Recent Tags" in the left pane (it has a +). When I click on "Recent Tags" and middle click on the tag in the right pane it loads normally.

It is the patch which is not smart. However, this may fix this problem. chrome\browser\content\browser\places\utils.js openContainerNodeInTabs: function PU_openContainerInTabs(aNode, aEvent) { var urlsToOpen = PlacesUtils.getURLsForContainerNode(aNode); + if (urlsToOpen.length < 1)return; if (!this._confirmOpenInTabs(urlsToOpen.length)) return; this._openTabset(urlsToOpen, aEvent); },

(In reply to comment #3) > It is the patch which is not smart. However, this may fix this problem. > chrome\browser\content\browser\places\utils.js > > openContainerNodeInTabs: function PU_openContainerInTabs(aNode, aEvent) { > var urlsToOpen = PlacesUtils.getURLsForContainerNode(aNode); > + if (urlsToOpen.length < 1)return; > if (!this._confirmOpenInTabs(urlsToOpen.length)) > return; > > this._openTabset(urlsToOpen, aEvent); > }, well, i already put that some time ago, but then we went with a different fix (trying to open correct contents rather than hiding the problem). Hwv thank you for pointing that :)

- get contents manually when we inherit excludeItems from the root - avoid mixing up var and let - nullify and set the viewer in hasChildURIs like we do in getURLsForContainerNode before opening a container

don't need to check if nodeIsContainer since a early return

Comment on attachment 318400 [details] [diff] [review] patch r=mano

Marco, what about the performance if the user has hundreds of bookmarks set with the same tag? Middle-click will open a huge amount of tabs within the browser which will probably result in a totally hang of Firefox for a longer period. Also seen with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042922 Minefield/3.0pre

We have browser.tabs.warnOnOpen;true to warn on opening multiple tabs

(In reply to comment #9) > We have browser.tabs.warnOnOpen;true to warn on opening multiple tabs At least there is no warning when doing the same with a folder located on the Bookmarks Toolbar. All contained bookmarks are opened in their own tab. That's why it should be handled with caution.

browser.tabs.warnOnOpen;true browser.tabs.maxOpenBeforeWarn;5 (or less for testing. default value is 15)

i have the warning on folders in the toolbar. reminder for me: before asking approval add a check for folder||tagQuery since not all containers can be treated like folders

sorry Mano if i ask you another review, but i've seen that previous patch was not complete, this adds support for saved searches in the left pane, i'm considering 3 cases: - folder||tag && excludeItems - query && excludeItems - other containers the only thing that i cannot open in tabs are queries in the right pane, but i think that could be due to bug 430659

You could fix these containers by un-setting expandQueries.

this appear to be enough to open everything, i've created a saved search and opened it in tabs correctly. Should we also take a fix to openTabset to avoid opening an empty tab if there is nothing in the urls array?

(In reply to comment #15) > Created an attachment (id=318573) [details] > patch > > this appear to be enough to open everything, i've created a saved search and > opened it in tabs correctly. > > Should we also take a fix to openTabset to avoid opening an empty tab if there > is nothing in the urls array? > browser.tabs.loadFolderAndReplace should be considered to open Tabs.

(In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=318573) [details] [details] > > patch > > > > this appear to be enough to open everything, i've created a saved search and > > opened it in tabs correctly. > > > > Should we also take a fix to openTabset to avoid opening an empty tab if there > > is nothing in the urls array? > > > browser.tabs.loadFolderAndReplace should be considered to open Tabs. > Please forget comment #16. But, It should be considered bug 175124 comment 29; "middle click on folder appends tabset"

Comment on attachment 318573 [details] [diff] [review] patch most likely needs an unbitrot

i refactored a bit the two functions splitting them in 2 parts, the first part opens a result and returns the root, the second part executes the task (return at first uri node or push into uris array). works for all items in the left pane, tag containers and other generic queries.

Marco, do we use the same function as for bookmarks to open all uris in new tabs? If not, do we also use browser.tabs.maxOpenBeforeWarn here to warn the user?

(In reply to comment #20) > Marco, do we use the same function as for bookmarks to open all uris in new > tabs? If not, do we also use browser.tabs.maxOpenBeforeWarn here to warn the > user? sure, this only builds a list of urls, then those are passed to the common open function

Comment on attachment 362709 [details] [diff] [review] patch v3.0 >diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js >--- a/toolkit/components/places/src/utils.js >+++ b/toolkit/components/places/src/utils.js >@@ -914,93 +914,135 @@ var PlacesUtils = { > var livemarks = annosvc.getItemsWithAnnotation(LMANNO_FEEDURI, {}); > for (var i = 0; i < livemarks.length; i++) { > if (annosvc.getItemAnnotation(livemarks[i], LMANNO_FEEDURI) == feedSpec) > return livemarks[i]; > } > return -1; > }, > >- // Returns true if a container has uris in its first level >- // Has better performances than checking getURLsForContainerNode(node).length >+ /** >+ * Returns true if a container has uri nodes in its first level. >+ * Has better performance than (getURLsForContainerNode(node).length > 0). >+ * @param aNode >+ * The container node to search through. >+ * @returns true if the node contains uri nodes, false otherwise. >+ */ > hasChildURIs: function PU_hasChildURIs(aNode) { > if (!this.nodeIsContainer(aNode)) > return false; > >- // in the Library left pane we use excludeItems >- if (this.nodeIsFolder(aNode) && asQuery(aNode).queryOptions.excludeItems) { >- var itemId = PlacesUtils.getConcreteItemId(aNode); >- var contents = this.getFolderContents(itemId, false, false).root; >- for (var i = 0; i < contents.childCount; ++i) { >- var child = contents.getChild(i); >- if (this.nodeIsURI(child)) >- return true; >+ var root = null; >+ var wasOpen = false; >+ var oldViewer = null; >+ >+ // excludeItems is inherited by child containers in an excludeItems view. >+ var excludeItems = asQuery(aNode).queryOptions.excludeItems || >+ asQuery(aNode.parentResult.root).queryOptions.excludeItems; >+ // expandQueries is inherited by child containers in an expandQueries view. >+ var expandQueries = asQuery(aNode).queryOptions.expandQueries && >+ asQuery(aNode.parentResult.root).queryOptions.expandQueries; >+ >+ if (this.nodeIsFolder(aNode) && excludeItems) { >+ // Get folder contents manually. >+ root = this.getFolderContents(this.getConcreteItemId(aNode), >+ false, false).root; >+ } >+ else if (this.nodeIsQuery(aNode) && (excludeItems || !expandQueries)) { >+ // Get query contents manually. >+ var queries = {}, options = {}; >+ this.history.queryStringToQueries(aNode.uri, queries, {}, options); >+ root = this.history.executeQueries(queries.value, >+ queries.value.length, >+ options.value).root; >+ root.containerOpen = true; >+ } >+ else { >+ wasOpen = aNode.containerOpen; >+ root = aNode; >+ if (!wasOpen) { >+ oldViewer = root.parentResult.viewer; >+ root.parentResult.viewer = null; >+ root.containerOpen = true; > } >- return false; > } all this bit is shared with getURLsForContainerNode, right? possible to put it in something like _getUsableRootForContainerNode()? > >- var wasOpen = aNode.containerOpen; >- if (!wasOpen) >- aNode.containerOpen = true; > var found = false; >- for (var i = 0; i < aNode.childCount && !found; i++) { >- var child = aNode.getChild(i); >+ for (var i = 0; i < root.childCount && !found; i++) { >+ var child = root.getChild(i); > if (this.nodeIsURI(child)) > found = true; could optimize and break here, yeah? > } >+ > if (!wasOpen) > aNode.containerOpen = false; >+ if (oldViewer) >+ root.parentResult.viewer = oldViewer; >+ > return found; > }, > >- getURLsForContainerNode: function PU_getURLsForContainerNode(aNode) { >- let urls = []; >- if (this.nodeIsFolder(aNode) && asQuery(aNode).queryOptions.excludeItems) { >- // grab manually >- var itemId = this.getConcreteItemId(aNode); >- let contents = this.getFolderContents(itemId, false, false).root; >- for (let i = 0; i < contents.childCount; ++i) { >- let child = contents.getChild(i); >- if (this.nodeIsURI(child)) >- urls.push({uri: child.uri, isBookmark: this.nodeIsBookmark(child)}); >+ /** >+ * Returns an array containing all the uris in the first level of the >+ * passed in container. >+ * If you only need to know if the node contains uris, use hasChildURIs. >+ * @param aNode >+ * The container node to search through >+ * @returns array of uris in the first level of the container. >+ */ >+ getURLsForContainerNode: function PU_getURLsForContainerNod(aNode) { s/Nod/Node/ r=me, with a test added.

(In reply to comment #22) > > > >- var wasOpen = aNode.containerOpen; > >- if (!wasOpen) > >- aNode.containerOpen = true; > > var found = false; > >- for (var i = 0; i < aNode.childCount && !found; i++) { > >- var child = aNode.getChild(i); > >+ for (var i = 0; i < root.childCount && !found; i++) { > >+ var child = root.getChild(i); > > if (this.nodeIsURI(child)) > > found = true; > > could optimize and break here, yeah? there's no need, see the !found check in the loop condition

another refactoring, with a unit test

Comment on attachment 364185 [details] [diff] [review] patch v4.0 >+ /** >+ * Returns a nsNavHistoryContainerResultNode with forced excludeItems and >+ * expandQueries. >+ * @param aNode >+ * The node to convert >+ * @param [optional] excludeItems >+ * True to hide all items (individual bookmarks). This is used on >+ * the left places pane so you just get a folder hierarchy. >+ * @param [optional] expandQueries >+ * True to make query items expand as new containers. For managing, >+ * you want this to be false, for menus and such, you want this to >+ * be true. >+ * @returns A nsINavHistoryContainerResultNode containing the unfiltered >+ * contents of the container. >+ * @note The returned container node could be open or closed, we don't >+ * guarantee its status. >+ */ >+ getUnfilteredContainerNode: >+ function PU_getUnfilteredContainerNode(aNode, aExcludeItems, aExpandQueries) { "unfiltered" doesn't sound right, considering you can pass parameters that filter the result. r=me otherwise

addressed comment

Verified fixed on trunk with builds on OS X and Windows: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090310 Minefield/3.2a1pre Ubiquity/0.1.5 ID:20090310030639 Would the latest patch be ready to ask approval for 1.9.1?

Comment on attachment 365170 [details] [diff] [review] patch v4.1 sure, asking approval 1.9.1, low risk, comes with a test, a nice to have polish.

Comment on attachment 365170 [details] [diff] [review] patch v4.1 a191=beltzner

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre

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