From 8f87a1a12b56930702f379131d79e59ebd0048b7 Mon Sep 17 00:00:00 2001 From: Asaf Romano Date: Sat, 2 Jun 2012 16:54:25 +0300 Subject: [PATCH] Bug 730340 - Don't use expando properties for storing data on places nodes. Part 3 - cache livemark-info object in the controller for each view instead of setting _feedURI and _siteURI exapndos on livemark nodes. Also fix a bug I introduced in the previous check for this bug, in PVB_nodeHistoryDetailsChanged. r=mak --- .../places/content/browserPlacesViews.js | 60 +++++++++---------- .../components/places/content/controller.js | 48 +++++++++++++-- browser/components/places/content/tree.xml | 11 ++-- browser/components/places/content/treeView.js | 44 +++++++------- 4 files changed, 100 insertions(+), 63 deletions(-) diff --git a/browser/components/places/content/browserPlacesViews.js b/browser/components/places/content/browserPlacesViews.js index 54eedf7fdf7..ee6e284116c 100644 --- a/browser/components/places/content/browserPlacesViews.js +++ b/browser/components/places/content/browserPlacesViews.js @@ -213,7 +213,7 @@ PlacesViewBase.prototype = { if (!resultNode.containerOpen) return; - if (resultNode._feedURI) { + if (this.controller.hasCachedLivemarkInfo(resultNode)) { this._setEmptyPopupStatus(aPopup, false); aPopup._built = true; this._populateLivemarkPopup(aPopup); @@ -315,10 +315,9 @@ PlacesViewBase.prototype = { #endif // Set an expando on the node, controller will use it to build // its metadata. - aPlacesNode._feedURI = aLivemark.feedURI; - aPlacesNode._siteURI = aLivemark.siteURI; + this.controller.cacheLivemarkInfo(aPlacesNode, aLivemark); } - } + }.bind(this) ); } @@ -365,8 +364,9 @@ PlacesViewBase.prototype = { _setLivemarkSiteURIMenuItem: function PVB__setLivemarkSiteURIMenuItem(aPopup) { - let siteUrl = aPopup._placesNode._siteURI ? aPopup._placesNode._siteURI.spec - : null; + let livemarkInfo = this.controller.getCachedLivemarkInfo(aPopup._placesNode); + let siteUrl = livemarkInfo && livemarkInfo.siteURI ? + livemarkInfo.siteURI.spec : null; if (!siteUrl && aPopup._siteURIMenuitem) { aPopup.removeChild(aPopup._siteURIMenuitem); aPopup._siteURIMenuitem = null; @@ -493,15 +493,13 @@ PlacesViewBase.prototype = { PlacesUtils.livemarks.getLivemark( { id: aPlacesNode.itemId }, - (function (aStatus, aLivemark) { + function (aStatus, aLivemark) { if (Components.isSuccessCode(aStatus)) { - // Set an expando on the node, controller will use it to build - // its metadata. - aPlacesNode._feedURI = aLivemark.feedURI; - aPlacesNode._siteURI = aLivemark.siteURI; + // Controller will use this to build the meta data for the node. + this.controller.cacheLivemarkInfo(aPlacesNode, aLivemark); this.invalidateContainer(aPlacesNode); } - }).bind(this) + }.bind(this) ); } }, @@ -533,7 +531,6 @@ PlacesViewBase.prototype = { nodeRemoved: function PVB_nodeRemoved(aParentPlacesNode, aPlacesNode, aIndex) { let parentElt = this._getDOMNodeForPlacesNode(aPlacesNode); - let elt = this._getDOMNodeForPlacesNode(aPlacesNode); // Here we need the . @@ -573,9 +570,10 @@ PlacesViewBase.prototype = { nodeHistoryDetailsChanged: function PVB_nodeHistoryDetailsChanged(aPlacesNode, aTime, aCount) { - if (aPlacesNode.parent && aPlacesNode.parent._feedURI) { + if (aPlacesNode.parent && + this.controller.hasCachedLivemarkInfo(aPlacesNode.parent)) { // Find the node in the parent. - let popup = this._getDOMNodeForPlacesNode(aPlacesNode); + let popup = this._getDOMNodeForPlacesNode(aPlacesNode.parent); for (let child = popup._startMarker.nextSibling; child != popup._endMarker; child = child.nextSibling) { @@ -652,11 +650,11 @@ PlacesViewBase.prototype = { } PlacesUtils.livemarks.getLivemark({ id: aPlacesNode.itemId }, - (function (aStatus, aLivemark) { + function (aStatus, aLivemark) { if (Components.isSuccessCode(aStatus)) { - let shouldInvalidate = !aPlacesNode._feedURI; - aPlacesNode._feedURI = aLivemark.feedURI; - aPlacesNode._siteURI = aLivemark.siteURI; + let shouldInvalidate = + !this.controller.hasCachedLivemarkInfo(aPlacesNode); + this.controller.cacheLivemarkInfo(aPlacesNode, aLivemark); if (aNewState == Ci.nsINavHistoryContainerResultNode.STATE_OPENED) { aLivemark.registerForUpdates(aPlacesNode, this); // Prioritize the current livemark. @@ -669,7 +667,7 @@ PlacesViewBase.prototype = { aLivemark.unregisterForUpdates(aPlacesNode); } } - }).bind(this) + }.bind(this) ); } } @@ -683,7 +681,7 @@ PlacesViewBase.prototype = { this._setLivemarkStatusMenuItem(aPopup, Ci.mozILivemark.STATUS_LOADING); PlacesUtils.livemarks.getLivemark({ id: aPopup._placesNode.itemId }, - (function (aStatus, aLivemark) { + function (aStatus, aLivemark) { let placesNode = aPopup._placesNode; if (!Components.isSuccessCode(aStatus) || !placesNode.containerOpen) return; @@ -702,7 +700,7 @@ PlacesViewBase.prototype = { else this._getDOMNodeForPlacesNode(child).removeAttribute("visited"); } - }).bind(this) + }.bind(this) ); }, @@ -1004,10 +1002,9 @@ PlacesToolbar.prototype = { button.setAttribute("livemark", "true"); // Set an expando on the node, controller will use it to build // its metadata. - aChild._feedURI = aLivemark.feedURI; - aChild._siteURI = aLivemark.siteURI; + this.controller.cacheLivemarkInfo(aChild, aLivemark); } - } + }.bind(this) ); } @@ -1253,15 +1250,14 @@ PlacesToolbar.prototype = { PlacesUtils.livemarks.getLivemark( { id: aPlacesNode.itemId }, - (function (aStatus, aLivemark) { + function (aStatus, aLivemark) { if (Components.isSuccessCode(aStatus)) { // Set an expando on the node, controller will use it to build // its metadata. - aPlacesNode._feedURI = aLivemark.feedURI; - aPlacesNode._siteURI = aLivemark.siteURI; + this.controller.cacheLivemarkInfo(aPlacesNode, aLivemark); this.invalidateContainer(aPlacesNode); } - }).bind(this) + }.bind(this) ); } } @@ -1681,7 +1677,8 @@ PlacesToolbar.prototype = { // so we don't rebuild its contents whenever the popup is reopened. // Though, we want to always close feed containers so their expiration // status will be checked at next opening. - if (!PlacesUtils.nodeIsFolder(placesNode) || placesNode._feedURI) { + if (!PlacesUtils.nodeIsFolder(placesNode) || + this.controller.hasCachedLivemarkInfo(placesNode)) { placesNode.containerOpen = false; } } @@ -1787,7 +1784,8 @@ PlacesMenu.prototype = { // so we don't rebuild its contents whenever the popup is reopened. // Though, we want to always close feed containers so their expiration // status will be checked at next opening. - if (!PlacesUtils.nodeIsFolder(placesNode) || placesNode._feedURI) + if (!PlacesUtils.nodeIsFolder(placesNode) || + this.controller.hasCachedLivemarkInfo(placesNode)) placesNode.containerOpen = false; // The autoopened attribute is set for folders which have been diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js index b9910174a33..e57cb18858a 100644 --- a/browser/components/places/content/controller.js +++ b/browser/components/places/content/controller.js @@ -79,6 +79,8 @@ function PlacesController(aView) { XPCOMUtils.defineLazyGetter(this, "profileName", function () { return Services.dirsvc.get("ProfD", Ci.nsIFile).leafName; }); + + this._cachedLivemarkInfoObjects = new WeakMap(); } PlacesController.prototype = { @@ -176,7 +178,7 @@ PlacesController.prototype = { case "placesCmd_reload": // Livemark containers var selectedNode = this._view.selectedNode; - return selectedNode && !!selectedNode._feedURI; + return selectedNode && this.hasCachedLivemarkInfo(selectedNode); case "placesCmd_sortBy:name": var selectedNode = this._view.selectedNode; return selectedNode && @@ -469,7 +471,7 @@ PlacesController.prototype = { if (parentNode) { if (PlacesUtils.nodeIsTagQuery(parentNode)) nodeData["tagChild"] = true; - else if (parentNode._feedURI) + else if (this.hasCachedLivemarkInfo(parentNode)) nodeData["livemarkChild"] = true; } } @@ -1038,8 +1040,9 @@ PlacesController.prototype = { addData(PlacesUtils.TYPE_X_MOZ_PLACE, i); // Drop the feed uri for livemark containers - if (node._feedURI) { - addURIData(i, node._feedURI.spec); + let livemarkInfo = this.getCachedLivemarkInfo(node); + if (livemarkInfo) { + addURIData(i, livemarkInfo.feedURI.spec); } else if (node.uri) { addURIData(i); @@ -1113,7 +1116,8 @@ PlacesController.prototype = { if (PlacesUtils.nodeIsFolder(node)) copiedFolders.push(node); - let overrideURI = node._feedURI ? node._feedURI.spec : null; + let livemarkInfo = this.getCachedLivemarkInfo(node); + let overrideURI = livemarkInfo ? livemarkInfo.feedURI.spec : null; let resolveShortcuts = !PlacesControllerDragHelper.canMoveNode(node); contents.forEach(function (content) { @@ -1281,7 +1285,39 @@ PlacesController.prototype = { } if (insertedNodeIds.length > 0) this._view.selectItems(insertedNodeIds, false); - } + }, + + /** + * Cache the livemark info for a node. This allows the controller and the + * views to treat the given node as a livemark. + * @param aNode + * a places result node. + * @param aLivemarkInfo + * a mozILivemarkInfo object. + */ + cacheLivemarkInfo: function PC_cacheLivemarkInfo(aNode, aLivemarkInfo) { + this._cachedLivemarkInfoObjects.set(aNode, aLivemarkInfo); + }, + + /** + * Returns whether or not there's cached mozILivemarkInfo object for a node. + * @param aNode + * a places result node. + * @return true if there's a cached mozILivemarkInfo object for + * aNode, false otherwise. + */ + hasCachedLivemarkInfo: function PC_hasCachedLivemarkInfo(aNode) + this._cachedLivemarkInfoObjects.has(aNode), + + /** + * Returns the cached livemark info for a node, if set by cacheLivemarkInfo, + * null otherwise. + * @param aNode + * a places result node. + * @return the mozILivemarkInfo object for aNode, if set, null otherwise. + */ + getCachedLivemarkInfo: function PC_getCachedLivemarkInfo(aNode) + this._cachedLivemarkInfoObjects.get(aNode, null) }; /** diff --git a/browser/components/places/content/tree.xml b/browser/components/places/content/tree.xml index 1da6a931567..1fb5a827298 100644 --- a/browser/components/places/content/tree.xml +++ b/browser/components/places/content/tree.xml @@ -98,17 +98,18 @@ callback = new Function("aContainer", onOpenFlatContainer); } - let treeView = new PlacesTreeView(this.flatList, callback); + if (!this._controller) { + this._controller = new PlacesController(this); + this.controllers.appendController(this._controller); + } + + let treeView = new PlacesTreeView(this.flatList, callback, this._controller); // Observer removal is done within the view itself. When the tree // goes away, treeboxobject calls view.setTree(null), which then // calls removeObserver. result.addObserver(treeView, false); this.view = treeView; - if (!this._controller) { - this._controller = new PlacesController(this); - this.controllers.appendController(this._controller); - } this._cachedInsertionPoint = undefined; ]]> diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js index f682f7f0439..2616c8f9d7d 100644 --- a/browser/components/places/content/treeView.js +++ b/browser/components/places/content/treeView.js @@ -2,7 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at http://mozilla.org/MPL/2.0/. */ -function PlacesTreeView(aFlatList, aOnOpenFlatContainer) { +function PlacesTreeView(aFlatList, aOnOpenFlatContainer, aController) { this._tree = null; this._result = null; this._selection = null; @@ -10,6 +10,7 @@ function PlacesTreeView(aFlatList, aOnOpenFlatContainer) { this._rows = []; this._flatList = aFlatList; this._openContainerCallback = aOnOpenFlatContainer; + this._controller = aController; } PlacesTreeView.prototype = { @@ -92,7 +93,7 @@ PlacesTreeView.prototype = { */ _isPlainContainer: function PTV__isPlainContainer(aContainer) { // Livemarks are always plain containers. - if (aContainer._feedURI) + if (this._controller.hasCachedLivemarkInfo(aContainer)) return true; // We don't know enough about non-query containers. @@ -299,7 +300,7 @@ PlacesTreeView.prototype = { // Recursively do containers. if (!this._flatList && curChild instanceof Ci.nsINavHistoryContainerResultNode && - !curChild._feedURI) { + !this._controller.hasCachedLivemarkInfo(curChild)) { let resource = this._getResourceForNode(curChild); let isopen = resource != null && PlacesUIUtils.localStore.HasAssertion(resource, @@ -789,7 +790,7 @@ PlacesTreeView.prototype = { _populateLivemarkContainer: function PTV__populateLivemarkContainer(aNode) { PlacesUtils.livemarks.getLivemark({ id: aNode.itemId }, - (function (aStatus, aLivemark) { + function (aStatus, aLivemark) { let placesNode = aNode; // Need to check containerOpen since getLivemark is async. if (!Components.isSuccessCode(aStatus) || !placesNode.containerOpen) @@ -800,7 +801,7 @@ PlacesTreeView.prototype = { let child = children[i]; this.nodeInserted(placesNode, child, i); } - }).bind(this)); + }.bind(this)); }, nodeTitleChanged: function PTV_nodeTitleChanged(aNode, aNewTitle) { @@ -818,7 +819,7 @@ PlacesTreeView.prototype = { nodeHistoryDetailsChanged: function PTV_nodeHistoryDetailsChanged(aNode, aUpdatedVisitDate, aUpdatedVisitCount) { - if (aNode.parent && aNode.parent._feedURI) { + if (aNode.parent && this._controller.hasCachedLivemarkInfo(aNode.parent)) { // Find the node in the parent. let parentRow = this._flatList ? 0 : this._getRowForNode(aNode.parent); for (let i = parentRow; i < this._rows.length; i++) { @@ -851,9 +852,9 @@ PlacesTreeView.prototype = { else if (aAnno == PlacesUtils.LMANNO_FEEDURI) { PlacesUtils.livemarks.getLivemark( { id: aNode.itemId }, - (function (aStatus, aLivemark) { + function (aStatus, aLivemark) { if (Components.isSuccessCode(aStatus)) { - aNode._feedURI = aLivemark.feedURI; + this._controller.cacheLivemarkInfo(aNode, aLivemark); let properties = this._cellProperties.get(aNode, null); if (properties) properties.push(this._getAtomFor("livemark")); @@ -861,7 +862,7 @@ PlacesTreeView.prototype = { // The livemark attribute is set as a cell property on the title cell. this._invalidateCellValue(aNode, this.COLUMN_TYPE_TITLE); } - }).bind(this) + }.bind(this) ); } }, @@ -887,10 +888,11 @@ PlacesTreeView.prototype = { } PlacesUtils.livemarks.getLivemark({ id: aNode.itemId }, - (function (aStatus, aLivemark) { + function (aStatus, aLivemark) { if (Components.isSuccessCode(aStatus)) { - let shouldInvalidate = !aNode._feedURI; - aNode._feedURI = aLivemark.feedURI; + let shouldInvalidate = + !this._controller.hasCachedLivemarkInfo(aNode); + this._controller.cacheLivemarkInfo(aNode, aLivemark); if (aNewState == Components.interfaces.nsINavHistoryContainerResultNode.STATE_OPENED) { aLivemark.registerForUpdates(aNode, this); // Prioritize the current livemark. @@ -903,7 +905,7 @@ PlacesTreeView.prototype = { aLivemark.unregisterForUpdates(aNode); } } - }).bind(this) + }.bind(this) ); } }, @@ -999,7 +1001,7 @@ PlacesTreeView.prototype = { } } - if (aContainer._feedURI) { + if (this._controller.hasCachedLivemarkInfo(aContainer)) { let queryOptions = PlacesUtils.asQuery(this._result.root).queryOptions; if (!queryOptions.excludeItems) { this._populateLivemarkContainer(aContainer); @@ -1169,20 +1171,20 @@ PlacesTreeView.prototype = { } else if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER || nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT) { - if (node._feedURI) { + if (this._controller.hasCachedLivemarkInfo(node)) { properties.push(this._getAtomFor("livemark")); } else { PlacesUtils.livemarks.getLivemark( { id: node.itemId }, - (function (aStatus, aLivemark) { + function (aStatus, aLivemark) { if (Components.isSuccessCode(aStatus)) { - node._feedURI = aLivemark.feedURI; + this._controller.cacheLivemarkInfo(node, aLivemark); properties.push(this._getAtomFor("livemark")); // The livemark attribute is set as a cell property on the title cell. this._invalidateCellValue(node, this.COLUMN_TYPE_TITLE); } - }).bind(this) + }.bind(this) ); } } @@ -1198,7 +1200,7 @@ PlacesTreeView.prototype = { else if (PlacesUtils.nodeIsURI(node)) { properties.push(this._getAtomFor(PlacesUIUtils.guessUrlSchemeForUI(node.uri))); - if (node.parent._feedURI) { + if (this._controller.hasCachedLivemarkInfo(node.parent)) { properties.push(this._getAtomFor("livemarkItem")); if (node.accessCount) { properties.push(this._getAtomFor("visited")); @@ -1253,7 +1255,7 @@ PlacesTreeView.prototype = { return true; let node = this._rows[aRow]; - if (node._feedURI) { + if (this._controller.hasCachedLivemarkInfo(node)) { let queryOptions = PlacesUtils.asQuery(this._result.root).queryOptions; return queryOptions.excludeItems; } @@ -1503,7 +1505,7 @@ PlacesTreeView.prototype = { } // Persist containers open status, but never persist livemarks. - if (!node._feedURI) { + if (!this._controller.hasCachedLivemarkInfo(node)) { let resource = this._getResourceForNode(node); if (resource) { const openLiteral = PlacesUIUtils.RDF.GetResource("http://home.netscape.com/NC-rdf#open");