During analysis of bug 405765, I have found a problem that may be related, but it does exhibit on both panes of the library window. When a mouse is moved over a folder function PlacesTreeView.getCellProperties gets called. It gets called even if you move the mouse on the same cell by 1px. It is not uncommon, that while you are moving the mouse, it would get called 10 times for each folder. The problem is that, that for each call of this function, on a generic bookmark folder, two database queries will be performed to determine, whether the folder is a livemark or tagContainer. There are hundreds of queries performed when you simply move your mouse to pickup your folder.

There are many ways how to fix this, the annotation service could be caching the results, or the nodes may be caching the annotations when they are loaded from database ... But this is all getting little complex, for now I have solved this by simple javascript cache.

Dietrich, this sounds to me like it should block, but I don't know that code or how hard it would be. Can you give us a quick cost/benefit SWAG and renominate if you think the blocking decision is wrong?

Comment on attachment 302856 [details] [diff] [review] Simple fix using javascript cache Won't non-bookmark nodes (node.itemId = -1) properties overwrite each other in the cache? Also, what about live update? Maybe should clear the cache record when an item is removed, since sqlite reuses row ids.

probably you should create a single element cache, and if you don't exit from the node you use the cache, if you change node you update it... this way you'll at least solve the problem "It gets called even if you move the mouse on the same cell by 1px" but should not have other problems with live updates.

(In reply to comment #4) > probably you should create a single element cache, and if you don't exit from > the node you use the cache, if you change node you update it... this way you'll > at least solve the problem "It gets called even if you move the mouse on the > same cell by 1px" but should not have other problems with live updates. This sounds like a very reasonable compromise. I have created a patch for this.

Derotted. If itemId is -1 combination of URL and title is used to check whether we should use cached value. This code saves computer resources even when moving mouse over history sidebar.

mh, probably you could use a single hash value to recognize the node, a combination of uri and itemId should be enough since history uris are guaranteed to be unique, so like itemId... while having uri+title, i cannot see how can the same history uri have a different title if it's not bookmarked?

(In reply to comment #7) > mh, probably you could use a single hash value to recognize the node, a > combination of uri and itemId should be enough since history uris are > guaranteed to be unique, so like itemId... while having uri+title, i cannot see > how can the same history uri have a different title if it's not bookmarked? Good point Marco! I have renamed the hash to key and always use function to get this key. Could be little slower (I avoided one function call in previous patch), but is much more readable now.

Comment on attachment 306228 [details] [diff] [review] Use id or uri as cache key >Index: browser/components/places/content/treeView.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/components/places/content/treeView.js,v >retrieving revision 1.40 >diff -u -1 -2 -p -d -r1.40 treeView.js >--- browser/components/places/content/treeView.js 27 Feb 2008 04:48:52 -0000 1.40 >+++ browser/components/places/content/treeView.js 28 Feb 2008 09:55:41 -0000 >@@ -856,57 +856,73 @@ PlacesTreeView.prototype = { > switch (status) { > case this.SESSION_STATUS_NONE: > break; > case this.SESSION_STATUS_START: > aProperties.AppendElement(this._getAtomFor("session-start")); > break; > case this.SESSION_STATUS_CONTINUE: > aProperties.AppendElement(this._getAtomFor("session-continue")); > break > } > }, > >+ _getCellPropertiesCacheKey: function PTV__getCellPropertiesCacheKey(aNode) { >+ return (aNode.itemId!=-1?aNode.itemId:aNode.uri); >+ }, >+ - remove superfluous parentheses - spaces before/after operators > } >- else if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR) >- aProperties.AppendElement(this._getAtomFor("separator")); >+ for (var i=0; i<props.length; i++) spaces before after operators this looks fine to me, other than the nits above. mano, can you second r this, since you wrote this code?

Could you fix the code style before I'm reviewing this patch? We don't wrap single-line block with braces, nor we put the braces on the same line with the else/else if.

Hrm, for RESULT_TYPE_VISIT, you could have the same uri for different nodes. Why don't you cache with the node itself? That is, array of objects It seems like we should make _visibleElements store this data, i.e. rather than just an array of nodes, we'd have an array of { node: result node, propertires: lazily cached properties }

This seems to be the right place for the cache, thanks Mano for the idea.

(In reply to comment #12) > Hrm, for RESULT_TYPE_VISIT, you could have the same uri for different nodes. but still should not properties be the same for the same url? until it contains + properties.push("query"); + properties.push("folder"); + properties.push("livemark"); + properties.push("tagContainer"); + properties.push("separator"); i doubt that for equal urls you can have this properties different. Still this could be useful for future caching

Comment on attachment 306497 [details] [diff] [review] Use _visibleElements array to lazily cache cell properties >Index: browser/components/places/content/treeView.js >=================================================================== > _buildVisibleList: function PTV__buildVisibleList() { > if (this._result) { > // Any current visible elements need to be marked as invisible. > for (var i = 0; i < this._visibleElements.length; i++) { >- this._visibleElements[i].viewIndex = -1; >+ this._visibleElements[i].node.viewIndex = -1; >+ this._visibleElements[i].properties = null; What's the point of nulling out "properties"? The array is spliced later anyway. > } > } > > var rootNode = this._result.root; > if (rootNode && this._tree) { > this._computeShowSessions(); > > asContainer(rootNode); > if (this._showRoot) { > // List the root node >- this._visibleElements.push(this._result.root); >+ this._visibleElements.push({node:this._result.root, properties: null}); spaces, please: this._visibleElements.push({ node: this._result.root, properties: null }); >+ aVisible.push({node:curChild, properties:null}); ditto. > // recursively do containers > if (!this._flatList && PlacesUtils.containerTypes.indexOf(curChildType) != -1) { > asContainer(curChild); > > var resource = this._getResourceForNode(curChild); > var isopen = resource != null && > PlacesUtils.localStore.HasAssertion(resource, openLiteral, > trueLiteral, true); > if (isopen != curChild.containerOpen) >- aToOpen.push(curChild); >+ aToOpen.push({node:curChild,properties:null}); Why change the strcut of this array, we just loop over it later for calling containerOpen... and you didn't update its usage to use .node. >@@ -287,49 +288,49 @@ PlacesTreeView.prototype = { > // Persist selection state > var previouslySelectedNodes = []; > var selection = this.selection; > var rc = selection.getRangeCount(); > for (var rangeIndex = 0; rangeIndex < rc; rangeIndex++) { > var min = { }, max = { }; > selection.getRangeAt(rangeIndex, min, max); > var lastIndex = Math.min(max.value, startReplacement + replaceCount -1); > if (min.value < startReplacement || min.value > lastIndex) > continue; > > for (var nodeIndex = min.value; nodeIndex <= lastIndex; nodeIndex++) >- previouslySelectedNodes.push({ node: this._visibleElements[nodeIndex], >- oldIndex: nodeIndex }); >+ previouslySelectedNodes.push( >+ { node: this._visibleElements[nodeIndex].node, oldIndex: nodeIndex }); Again, you changed the array struct, but not it's usage. I think this should be a simple node araay as well. > // nsITreeView >@@ -869,93 +872,100 @@ PlacesTreeView.prototype = { >+ if (!properties) { >+ properties = new Array(); >+ var nodeType = node.type; >+ if (PlacesUtils.containerTypes.indexOf(nodeType) != -1) { >+ if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY) { >+ properties.push("query"); >+ if (this._showQueryAsFolder) >+ properties.push("folder"); >+ } else if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER || >+ nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT) { nit: } else if >+ if (PlacesUtils.livemarks.isLivemark(node.itemId)) >+ properties.push("livemark"); >+ else if (PlacesUtils.bookmarks.getFolderIdForItem(node.itemId) == >+ PlacesUtils.tagsFolderId) >+ properties.push("tagContainer"); Could you please optimize the RESULLT_BY_TAG and node.parent.itemId == tags cases while you're at it? >+ } > } >+ else if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR) >+ properties.push("separator"); >+ >+ this._visibleElements[aRow].properties = properties; > } >- else if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR) >- aProperties.AppendElement(this._getAtomFor("separator")); >+ for (var i = 0; i < properties.length; i++) >+ aProperties.AppendElement(this._getAtomFor(properties[i])); > }, Why not simply keep references to the atoms within the properties array?

(In reply to comment #15) > (From update of attachment 306497 [details] [diff] [review]) > >Index: browser/components/places/content/treeView.js > >=================================================================== > > _buildVisibleList: function PTV__buildVisibleList() { > > if (this._result) { > > // Any current visible elements need to be marked as invisible. > > for (var i = 0; i < this._visibleElements.length; i++) { > >- this._visibleElements[i].viewIndex = -1; > >+ this._visibleElements[i].node.viewIndex = -1; > >+ this._visibleElements[i].properties = null; > > What's the point of nulling out "properties"? The array is spliced later > anyway. This is really not visible in the scope of this function that there would be any slicing later. But the rule should be that properties get set to null when they are made visible, not hidden. > > if (isopen != curChild.containerOpen) > >- aToOpen.push(curChild); > >+ aToOpen.push({node:curChild,properties:null}); > > Why change the strcut of this array, we just loop over it later for calling > containerOpen... and you didn't update its usage to use .node. Good point and shame on me. > > for (var nodeIndex = min.value; nodeIndex <= lastIndex; nodeIndex++) > >- previouslySelectedNodes.push({ node: this._visibleElements[nodeIndex], > >- oldIndex: nodeIndex }); > >+ previouslySelectedNodes.push( > >+ { node: this._visibleElements[nodeIndex].node, oldIndex: nodeIndex }); > > > Again, you changed the array struct, but not it's usage. I think this should be > a simple node araay as well. In this case I was not wrong. There was an array passed already before (watch the name of the second key. > Could you please optimize the RESULLT_BY_TAG and node.parent.itemId == tags > cases while you're at it? I have removed the changes for the livemark service (to be done with bug 420078) and left optimization of tag container detection to bug 420683 (there are more places where this should be changed). > >- else if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_SEPARATOR) > >- aProperties.AppendElement(this._getAtomFor("separator")); > >+ for (var i = 0; i < properties.length; i++) > >+ aProperties.AppendElement(this._getAtomFor(properties[i])); > > }, > > Why not simply keep references to the atoms within the properties array? Good point, thanks.

Comment on attachment 307027 [details] [diff] [review] Fix review comments r=mano.

Comment on attachment 307027 [details] [diff] [review] Fix review comments Too late in beta bake for this fix. Nominating for post-beta4.

Comment on attachment 307027 [details] [diff] [review] Fix review comments This is a blocker. No approval1.9 needed.

Checking in browser/components/places/content/treeView.js; /cvsroot/mozilla/browser/components/places/content/treeView.js,v <-- treeView.js new revision: 1.45; previous revision: 1.44 done

Still not good enough. Missing change on two places broke drag and drop.

Could this patch have caused bug 420905?

(In reply to comment #23) > Could this patch have caused bug 420905? > Yes, the attachment #307027 [details] [diff] [review] broke it and attachment #307234 [details] [diff] [review] fixes it.

Comment on attachment 307234 [details] [diff] [review] Fix problem with drag and drop r=mano

Checking in browser/components/places/content/treeView.js; /cvsroot/mozilla/browser/components/places/content/treeView.js,v <-- treeView.js new revision: 1.52; previous revision: 1.51 done

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