498130 - Reduce places-views overhead.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Asaf Romano (gone)]

Today, I set onlyBookmarked on the history sidebar query with an explicit search term, then i've set an alert in invalidateContainer, and got between 4 to 6 tree invalidation for every page visit or bookmark addition. Digging a little, I found out that the situation is even worse than it seems. For every QUERYUPDATE_COMPLEX_WITH_BOOKMARKS, we do the following:
 1. On every itemAdded notification, get the itemType from the DB, and don't use it. We could get the item type without going back to the db.
 2. Because the *founded* item-type isn't used, we invalidate queries (and, the view) on the addition of separators and folders.
 3. We don't differ between history queries on which onlyBookmarked is set to "real" bookmark queries. For that reason, we refresh forcefully in OnItemChanged for onlyBookmarked-queries. Note that OnItemChanged is dispatched for annotation-changes as well.
 4. ItemAdded/Removed also ignore the query's uri property.

Comment 1

[Asaf Romano (gone)]

We should fix this first, but we should also have a longer term goal for not using tree invalidation as the only manner to add or remove an item.

Comment 2

[Asaf Romano (gone)]

Attachment: checkpoint

Comment 3

[Asaf Romano (gone)]

Morphing summary to reflect actual work

Comment 4

[Asaf Romano (gone)]

Attachment: another checkpoint

Comment 5

[Asaf Romano (gone)]

Attachment: more...

Comment 6

[Asaf Romano (gone)]

Attachment: more...

Comment 7

[Asaf Romano (gone)]

Attachment: more...

Comment 8

[Asaf Romano (gone)]

Attachment: almost ready

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 398708 [details] [diff] [review]
almost ready

i don't like the viewDOMElement name, sounds like a verb/action, do we really need to specify view? i'd really prefer just node.DOMElement

>diff --git a/browser/components/places/content/menu.xml b/browser/components/places/content/menu.xml
>--- a/browser/components/places/content/menu.xml
>+++ b/browser/components/places/content/menu.xml
>@@ -531,19 +531,23 @@
>     </handlers>
>   </binding>
> 
> 
>   <binding id="places-menupopup"
>            extends="chrome://browser/content/places/menu.xml#places-popup-base">
>     <implementation>
>       <destructor><![CDATA[
>-        this._resultNode = null;
>+        if (this._resultNode) {

While here please add missing _resultNode field near _result field.

>@@ -594,44 +598,39 @@
>+
>+          // break the cycle before GC does

Please, while moving on, ucfirst and end with . all comments you add.

>+          if (child.node)
>+            delete child.node.viewDOMElement;

what's the purpose here? cycle collector has been created exactly for the sake of avoiding having to solve cycles manually
do you fear this cycle could delay the collection too much? It's fine for me but i'd like to see reasons in the above comment at least.

>         itemRemoved: function PMV_itemRemoved(aParentNode, aNode, aIndex) {
>-          var popup = this._getPopupForContainer(aParentNode);
>+          let parentElt = aParentNode.viewDOMElement;
>+          let nodeElt = aNode.viewDOMElement;
>+
>+          NS_ASSERT(parentElt, "parent-node passed must have a viewDOMElement");
>+          NS_ASSERT(nodeElt, "node passed must have a viewDOMElement");

please move nodeElt after the popup check below, before its first use. I think will make things easier to read (e.g. you get the parentElt because its needed to get the popup, then...)

>         itemMoved:
>         function PMV_itemMoved(aItem, aOldParent, aOldIndex, aNewParent,
>                                aNewIndex) {
>+          let element = aItem.viewDOMElement;

please try to be naming consistent, in the previous handler you called the temp var NodeElt, here it is element. the same name all around will help code clarity.

>+          NS_ASSERT(element, "node must have a dom element");

DOM uppercase anywhere i think

>+
>+          if (element == this._self)
>+            return;

why this check here and not, for example, in itemRemoved?
is it really possible we notify a change to the root node? if not, should not this check be an actual error (ASSERT) everywhere instead?

> 
>-          var popup = this._getPopupForContainer(parentNode);
>-          if (!popup._built)
>+        itemURIChanged: function PMV_itemURIChanged(aNode, aURIString) {
>+          let element = aNode.viewDOMElement;
>+          if (!element)
>             return;

ditto: is the element == this needed everywhere since it is not here? I'll stop commenting on this from now on.

>         itemReplaced:
>         function PMV_itemReplaced(aParentNode, aOldNode, aNewNode, aIndex) {
>-          var popup = this._getPopupForContainer(aParentNode);
>+          let parentElt = aParentNode.viewDOMElement;
>+          let nodeElt = aOldNode.viewDOMElement;
>+
>+          NS_ASSERT(parentElt, "parent-node passed must have a viewDOMElement");
>+          NS_ASSERT(nodeElt, "node passed must have a viewDOMElement");

same as above, i'd prefer parentElt just before popup, nodeElt before its first use, and temp var name consistency.

>  
>         invalidateContainer: function PMV_invalidateContainer(aContainer) {
>           if (!this._self._built)
>             return;
> 
>-          function isChildOf(node, container) {
>-            var parent = node.parent;
>-            while (parent) {
>-              if (parent == container)
>-                return true;
>-              parent = parent.parent;
>-            }
>-            return false;
>+          var containerDOMElement = aContainer.viewDOMElement;
>+          NS_ASSERT(containerDOMElement, "node must have a dom element");
>+          if (containerDOMElement == this._self) {
>+            this._self._built = false;
>+
>+            // if the menupopup is open we should live-update it
>+            if (this._self.open)
>+              this._rebuild();

is it fine to call rebuild() (that will set built = true on the popup), and then go on and set built = false on the popup just below?
maybe should early return?

>           }
> 
>-          var popupToRebuild = null;
>-          for (var i=0; i < this._self._containerNodesMap.length; i++) {
>-            var node = this._self._containerNodesMap[i].resultNode;
>+          // child-popup case
>+          var popupToRebuild = containerDOMElement.firstChild;
>+          if (popupToRebuild)
>+            popupToRebuild._built = false;
> 
>-            if (node == aContainer)
>-              popupToRebuild = this._self._containerNodesMap[i].domNode;
>-            if (isChildOf(node, aContainer)) {
>-              this._self._containerNodesMap.splice(i,1);
>-              i--;
>-            }
>-          }
>-
>-          if (!popupToRebuild)
>-            popupToRebuild = this._self;
>           popupToRebuild._built = false;

this is repeated two times if the diff is not fooling me.

>diff --git a/browser/components/places/content/toolbar.xml b/browser/components/places/content/toolbar.xml

some of the above comments are valid in all views, as well, i'm not going to repeat them

>--- a/browser/components/places/content/toolbar.xml
>+++ b/browser/components/places/content/toolbar.xml
>@@ -93,16 +93,20 @@
>       ]]></constructor>
> 
>       <destructor><![CDATA[
>         this._scrollbox.removeEventListener("overflow", this, false);
>         this._scrollbox.removeEventListener("underflow", this, false);
>         window.removeEventListener("resize", this, false);
> 
>         if (this._result) {
>+          let rootNode = this._result.root;
>+          if (rootNode)
>+            rootNode.viewDOMElement = null;

could you please close the rootNode while you're here?

>+
>+          let rootNode = this._result.root;

_resultNode?
Actually we have a _resultNode field, but looks like it's not correctly set and used?
Please, fix it and use it here and below.

>+          while (chevronPopup.hasChildNodes()) {

you used firstChild above, why lastChild here?
btw, maybe just Array.forEach(chevronPopup.childNodes, function (aChild) { whatever? });

>       <method name="removeItem">
>         <parameter name="child"/>
>         <body><![CDATA[
>-          if (PlacesUtils.nodeIsContainer(child.node)) {
>-            for (let i = 0; i < this._containerNodesMap.length; i++) {
>-              if (this._containerNodesMap[i].resultNode == child.node) {
>-                this._containerNodesMap.splice(i, 1);
>-                break;
>-              }
>-            }
>-          }
>-
>           // if document.popupNode pointed to this child, null it out,
>           // otherwise controller's command-updating may rely on the removed
>           // item still being "selected".
>           if (document.popupNode == child)
>             document.popupNode = null;
>+
>+          // paranoid: break the cycle before GC does

yay, paranoid! please fix these comments :)


>           catch(ex) {
>             // Invalid query, or had no results.
>-            // This is valid, eg: user deletes their bookmarks toolbar folder. 
>+            // This is valid, eg: user deletes their bookmarks toolbar folder.

While here, please rephrase this comment to be readable.

>+          let element = aItem.viewDOMElement;
>+          NS_ASSERT(element, "node must have a dom element");
>+            
>+          let onToolbar = element.localName != "menuitem";

wait, we also have "menu"s right? would be better == "toolbarbutton" i guess
here and below. won't repeat the comment.
If this check is largely used, i'd suggest a local method _isOnToolbar(aElement)

>diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js
>@@ -250,44 +198,41 @@
>   _refreshVisibleSection: function PTV__refreshVisibleSection(aContainer) {
>     NS_ASSERT(this._result, "Need to have a result to update");
>     if (!this._tree)
>       return;
> 
>-    // The root node is invisible if showRoot is not set. Otherwise aContainer
>-    // must be visible
>-    if (this._showRoot || aContainer != this._result.root) {
>+    // The root node is invisible

i prefer "The root node is always invisible, but any other container must be visible."

>     // We don't replace the container item itself so we decrease the
>     // replaceCount by 1. We don't do so though if there is no visible item
>-    // for the container. This happens when aContainer is the root node and
>-    // showRoot is not set.
>+    // for the container. This happens when aContainer is the root node.

double spacing after dots please

>@@ -520,17 +465,17 @@
>         this._result.sortingMode != Ci.nsINavHistoryQueryOptions.SORT_BY_NONE) {
>       aItem.viewIndex = -1;
>       return;
>     }
> 
>     // update parent when inserting the first item because twisty may
>     // have changed

please while here fix comment uc and . end.

>+  _invalidateValue: function PTV__invalidateValue(aItem, aColumnType) {

I'm not sure i like the name of this method, it's just a bit too generic, which "value"?
maybe _InvalidateItemByColumn?

>     NS_ASSERT(this._result, "Got a notification but have no result!");
>     var viewIndex = aItem.viewIndex;
>-    if (this._tree && viewIndex >= 0)
>-      this._tree.invalidateRow(viewIndex);
>+    if (this._tree && viewIndex >= 0) {
>+      let valueColumn = this._findColumnByType(aColumnType);
>+      if (valueColumn && !valueColumn.element.hidden)
>+        this._tree.invalidateCell(viewIndex, valueColumn);
>+
>+      // last modified is altered for almost all item changes.

ucfirst plz and "Last modified column is altered for almost any item change."

> 
>+  _columns: [],
>+  _findColumnByType: function(aColumnType) {
>+    if (this._columns[aColumnType])
>+      return this._columns[aColumnType];
>+
>+    var columns = this._tree.columns;
>+    var colCount = columns.count;
>+    for (var i = 0; i < colCount; i ++) {
>+      var column = columns.getColumnAt(i);
>+      if (this._getColumnType(column) == aColumnType)
>+        return this._columns[aColumnType] = column;
>+    }
>+
>+    return null;
>+  },

would not be simpler just make columns a getter so on first access it will be filled up and ready for next ones?
maybe renaming it to _columnsByType , then you could just use all around this._columnsByType[TYPE]

>@@ -936,70 +916,66 @@
>   },
> 
>   getColumnProperties: function(aColumn, aProperties) { },
> 
>   isContainer: function PTV_isContainer(aRow) {
>     this._ensureValidRow(aRow);
> 
>     var node = this._visibleElements[aRow].node;
>-    if (PlacesUtils.nodeIsContainer(node)) {
>+    var nodeType = node.type;
>+    if (PlacesUtils.containerTypes.indexOf(nodeType) != -1) {

hm, why? unless there are clear perf measures that demonstrate this is actually giving back something, i would not really move away from common nodeIsXXX utils, since this is just going to duplicate code, make it more error-prone and less readable.
Gaining some us paying code clarity is not worth the cost

> 
>       // treat non-expandable childless queries as non-containers
>-      if (PlacesUtils.nodeIsQuery(node)) {
>+      if (nodeType == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY) {

ditto, won't repeat below. Please U-turn

>   isContainerOpen: function PTV_isContainerOpen(aRow) {
>     if (this._flatList)
>       return false;
> 
>     this._ensureValidRow(aRow);
>-    if (!PlacesUtils.nodeIsContainer(this._visibleElements[aRow].node))
>-      throw Cr.NS_ERROR_INVALID_ARG;
>-

i suppose this is called by the tree code, and won't call it for non containers, right?

>diff --git a/toolkit/components/places/public/nsINavBookmarksService.idl b/toolkit/components/places/public/nsINavBookmarksService.idl

>@@ -79,19 +79,22 @@
>    * no additional notifications will be sent.
>    *
>    * @param aItemId
>    *        The id of the bookmark that was added.
>    * @param aFolder
>    *        The folder that the item was added to.
>    * @param aIndex
>    *        The item's index in the folder.
>+   * @param aItemType
>+   *        The type of the item that was added (one of the TYPE_* constants
>+   *        defined above.)

period after added and new line for the type_* comment
passing also aDateAdded could be useful.

>    */
>   void onItemAdded(in long long aItemId, in long long aFolder,
>-                   in long aIndex);
>+                   in long aIndex, in unsigned short aItemType);

s/aFolder/aFolderId/

onItemRemoved could also gain aItemType param please?
and also onBeforeItemRemoved please.

>@@ -117,31 +120,34 @@
>    * will be called whenever any attributes like "title" are changed.
>    * 
>    * @param aItemId
>    *        The id of the item that was changed.
>    * @param aProperty
>    *        The property which changed.
>    * @param aIsAnnotationProperty
>    *        Is aProperty the name of an item annotation
>-   *
>+   * @param aNewLastModified
>+   *        If the item's lastModified field was changed, this parameter is
>+   *        set to the new value, otherwise it's set to 0.
>+   *        
>    * property = "cleartime": (history was deleted, there is no last visit date):
>    *                         value = empty string.
>    * property = "title": value = new title.
>    * property = "favicon": value = new "moz-anno" URL of favicon image
>    * property = "uri": value = new uri spec.
>    * property = "tags: (tags set for the bookmarked uri have changed)
>    *             value = empty string.
>    * property = "dateAdded": value = PRTime when the item was first added
>    * property = "lastModified": value = PRTime when the item was last modified
>    * aIsAnnotationProperty = true: value = empty string.
>    */
>   void onItemChanged(in long long aBookmarkId, in ACString aProperty,
>                      in boolean aIsAnnotationProperty,
>-                     in AUTF8String aValue);
>+                     in AUTF8String aValue, in PRTime aNewLastModified);

s/aNewLastModified/aLastModified/
s/aValue/aNewValue/
s/aBookmarkId/aItemId/

please, i'd say to add aItemType

>@@ -159,20 +165,24 @@
>    *  @param aOldParent
>    *         The id of the old parent.
>    *  @param aOldIndex
>    *         The old index inside aOldParent.
>    *  @param aNewParent
>    *         The id of the new parent.
>    *  @param aNewIndex
>    *         The foindex inside aNewParent.
>+   * @param  aItemType
>+   *         The type of the item that was moved (one of the TYPE_* constants
>+   *         defined above.)
>    */
>   void onItemMoved(in long long aItemId,
>                    in long long aOldParent, in long aOldIndex,
>-                   in long long aNewParent, in long aNewIndex);
>+                   in long long aNewParent, in long aNewIndex,
>+                   in unsigned short aItemType);

s/aOldParent/aOldParentId/
s/aNewParent/aNewParentId/

>diff --git a/toolkit/components/places/public/nsINavHistoryService.idl b/toolkit/components/places/public/nsINavHistoryService.idl

>@@ -140,25 +141,28 @@
>    * hierarchy. The members of result.children have indentLevel = 0, their
>    * children have indentLevel = 1, etc. The indent level of the root node is
>    * set to -1.
>    */
>   readonly attribute long indentLevel;
> 
>   /**
>    * Value with undefined meaning for use by the view. Its initial value will
>-   * be -1. The result implementation treats nodes with this property set to
>-   * -1 as invisible!
>+   * be -1. If viewDOMElement is null, the result implementation treats nodes
>+   * with this property set to -1 as invisible!

reading this comment looks like these 2 properties are duping the same functionality,
i think should be rephrased better.

>    *
>    * View-implementations may use this value to track the node index in the
>    * view, e.g. the tree view uses this value to indicate the row in the
>    * tree that this node is at. Other views may choose not to use this, but
>    * should inititalize this value to anything but -1 for visible nodes.
>    */
>   attribute long viewIndex;
>+
>+  attribute nsIDOMElement viewDOMElement;
>+
> 

too many newlines?
Please add a better comment explaining what is the new property, why we have it, what are its advantages.
The idl should be self-explicative, since we lazy update the docs pitifully.

>+   * Called right after aNode's title property is changed.

right after sounds too much implementation wide. I'd just say "Called when aNode's title property has changed."

>+   * 
>+   * @param aNode
>+   *        a result node

ucfirst and end with . everywhere please

>+  /**
>+   * Called right after aNode's time property or accessCount property, or both
>+   * are changed.
>+   *
>+   * @param aNode
>+   *        a uri result node
>+   * @param aUpdateVisitDate
>+   *        the updated visit date
>+   * @param aUpdatedAccessCount
>+   *        the updated access-count
>+   */
>+  void itemTimeOrAccessCountChanged(in nsINavHistoryResultNode item,
>+                                    in PRTime aUpdateVisitDate,
>+                                    in unsigned long aUpdatedAccessCount);

reasons to have these together rather than splitted?

>diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp

>@@ -2131,17 +2131,18 @@
> {
>   NS_ENSURE_ARG_MIN(aItemId, 1);
> 
>   nsresult rv = SetItemDateInternal(mDBSetItemDateAdded, aItemId, aDateAdded);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   ENUMERATE_OBSERVERS(mCanNotify, mCacheObservers, mObservers, nsINavBookmarkObserver,
>                       OnItemChanged(aItemId, NS_LITERAL_CSTRING("dateAdded"),
>-                                    PR_FALSE, nsPrintfCString(16, "%lld", aDateAdded)));
>+                                    PR_FALSE, nsPrintfCString(16, "%lld", aDateAdded),
>+                                    0));

iirc when dateAdded changes lastModified is set to the same value, please check, in such a case return dateAdded instead of 0

>diff --git a/toolkit/components/places/src/nsNavHistoryQuery.cpp b/toolkit/components/places/src/nsNavHistoryQuery.cpp

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp

>   // easy case, the node itself is visible
>-  if (mViewIndex >= 0)
>+  if (mViewIndex >= 0 || mViewDOMElement)

in which case we do have to check both?
Are there cases where viewIndex is < 0 but the node has a viewDOMElement and is visible?

> void
> nsNavHistoryContainerResultNode::ReverseUpdateStats(PRInt32 aAccessCountChange)
> {
>   if (mParent) {
>+    nsNavHistoryResult* result = GetResult();
>+    PRBool shouldUpdateView = result && result->GetView() &&
>+                              mParent->mParent &&
>+                              mParent->mParent->AreChildrenVisible();
>+
>     mParent->mAccessCount += aAccessCountChange;
>     PRBool timeChanged = PR_FALSE;
>     if (mTime > mParent->mTime) {
>       timeChanged = PR_TRUE;
>       mParent->mTime = mTime;
>+
>+    }

bogus newline before the closing parens


> NS_IMETHODIMP
> nsNavHistoryQueryResultNode::OnItemChanged(PRInt64 aItemId,
>                                            const nsACString& aProperty,
>                                            PRBool aIsAnnotationProperty,
>-                                           const nsACString& aValue)
>+                                           const nsACString& aValue,
>+                                           PRTime aNewLastModified)
> {
>   // History observers should not get OnItemChanged
>   // but should get the corresponding history notifications instead.
>   // For bookmark queries, "all bookmark" observers should get OnItemChanged.
>   // For example, when a title of a bookmark changes, we want that to refresh.
>-  if (mLiveUpdate == QUERYUPDATE_COMPLEX_WITH_BOOKMARKS)
>+  if (mLiveUpdate == QUERYUPDATE_COMPLEX_WITH_BOOKMARKS) {
>+    // Make sure it's not a folder or a separator or a folder
>+    nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>+    NS_ENSURE_TRUE(bookmarks, NS_ERROR_UNEXPECTED);
>+    PRUint16 itemType;
>+    nsresult rv = bookmarks->GetItemType(aItemId, &itemType);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if (itemType != nsINavBookmarksService::TYPE_BOOKMARK)
>+      return NS_OK;

why just not adding itemType to onItemChanged, this is exactly the reason why we are adding it in other notifications, so that observers don't have to catch it by themselves.

>+  else if (aProperty.EqualsLiteral("dateAdded")) {
>+    nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>+    NS_ENSURE_TRUE(bookmarks, NS_ERROR_UNEXPECTED);
>+
>+    PRTime dateAdded;
>+    nsresult rv = bookmarks->GetItemDateAdded(aItemId, &dateAdded);

hum, wait, if property is dateAdded, why don't we get the value in aValue?

This is HUGE, but nice. Just please let's try to finalize this and don't add any other change, any new change can be done in followups, i think there is enough work here, just to fix all tests.

Comment 10

[Asaf Romano (gone)]

I'll reply and fix the rest later, but for now I'd like to reply on a couple of issues:
1. The nodeIs* functions are a design mistake of the original places team. If you'll check carefully (even in treeview.xml), you'll see that I've tried to use it as less as possible.

For that reason, I'm not going to revert the current changes but rather make them more readable - maybe start using instanceof (which also QIs! withour throwing!). I'm willing to get rid of the nodeIs* functions in the upcoming followups.

2. with this patch, viewIndex is kept -1 when (view)DOMElement is used. It was never a good a design to use it as a "boolean? (0=true,-1=false...). I still haven't updated the IDL to reflect that change.

3. I'll just remove the "paranoid" cycle breaking. Old Habits.

Comment 11

[Asaf Romano (gone)]

...and just see how it then calls nodeIsDay/Host - each one checks the type again.

Comment 12

[Marco Bonardo [:mak]]

(In reply to comment #10)
> 1. The nodeIs* functions are a design mistake of the original places team. If
> you'll check carefully (even in treeview.xml), you'll see that I've tried to
> use it as less as possible.

I still don't see reasoning, i mean, could even be those methods are not performant, but having a centralized way to recognize nodes is fairly better than having to recall how each node is defined in code.
If you want to change the helpers to make them more performant, i'm fine with it, but i don't agree in making the code harder to follow, and more dependant on implementation details, just to gain some microsecond here and there.
Also notice some of the nodeIs* utils are effectively doing something useful, see nodeIsLivemarkContainer recalling itemIsLivemark, that will avoid to initialize the service if it has not yet started.
If the problem is the fact we cross XPCOM too much to check node.type (so we end up calling node->GetItemType() every time we do if(folder) else if(livemark) else if (whatever)...), then we should just have a single PlacesUtils.getNodeType, that will return some PlacesUtils constants like PlacesUtils.nodeTypes.folder... that would even be better so you would call nodeType = PU.getNodeType(node); and then if (nodeType = PU.nodeTypes.livemark) do_something. But then i'd say to file this as a followup and don't touch nodeIs* in this iteration please. Also because that would involve some more code change i don't want to review here. So please, for now followup the full thing to a new bug and revert changes. Eventually evaluate my suggested approach.

> 2. with this patch, viewIndex is kept -1 when (view)DOMElement is used. It was
> never a good a design to use it as a "boolean? (0=true,-1=false...). I still
> haven't updated the IDL to reflect that change.

Could you please briefly explain what is the final target for these 2 properties? Somehow they are oviously overlapping, i'd like to understand in your final design idea, which part of their usage will overlap, which is unique. Something like: we need DOMElement to... but we still need viewIndex when. That should also go in the idl comment for sake of clarity.

The only cpp point where i see DOMElement used is in AreChildrenVisible(), i'm trying to understand if this double check is really useful and when, because if we can live without it, then there is no need to add a fontend property to each node, we could just have an associative array in the view like view.DOMElements[node], when we set a new result or we close the resultNode, we clear its contents. Would that perform really worst in your opinion?
I understand that just setting it to the node is the easiest and cleanest thing, just guessing if we can avoid polluting the node's idl without interesting perf loss.

Thank you for this work, looks great so far.

Comment 13

[Asaf Romano (gone)]

NodeType: ok ok, getNodeType(s) sounds nice. will do (in a follow up, yes).

viewIndex: That property was added for trees, back in the fx2-places days, when menu/toolbar didn't use views. The places tree doesn't represent its nodes with DOM elements - It has flat list of rows (indentLevel is handled separately. The *essential* purpose of viewIndex was/is to allow the view to store its child-postion, *as a flat list*. That indeed made a lot of sense for trees. The "hacky" purpose of node's viewIndex was to tell the reslt weather or not it's visible.

Now, we couldn't use viewIndex in the essential manner for the rest of our views, simply because they do no have flat lists concept - you'd have to create it, forcing code complexity and perf isses. Without this patch, viewIndex is set to 0 for each node just to tell the view "that node is visible". That's clearly wrong from both a pratical aspect and a design aspect:
1. design: index is supposed to unique, not a boolean on which 0=true (i think VB6 did that though!).
2. practical: Once index means index, we can automate viewIndex changes within a result (e.g. when a node is removed). As things stand, the treeView does it manually in a very inefficitent way. By automating that, we'll be able to finally fix our most critical perf issue w/ trees.

Thus, the double-check in AreChildrenVisibile checks the two different ways in which a node is represnted. Of course, I need to document that well, but I don't find it wrong/hacky at all.

About the array proposal: even if you disagree on the broken viewIndex usage, we had enogh experience with that (and in trees, we still do) for not doing it ever again.

Comment 14

[Marco Bonardo [:mak]]

i don't disagree in broken viewIndex usage, if i got this correctly, viewIndex is used for tree-like views, for any other view it is hacky used as a boolean visible/invisible. the new DOMElement replaces the hacky usage of viewIndex for those views. (yeah better comments would really help getting this concept).
And all of this complexity is mostly used for AreChildrenVisible(), this is just crazy.

I'd expect the view itself to provide a method to tell if a node is visible and the view should be updated, and the result to call that method if a view is attached. Actually instead view properties are interleaved in result properties, and the result has to guess if the view has to be updated or not by itself.

About array implementation, it would not be like visibleList (Sequential access), but being associative would just be a random access array[node] = DOMElement, or array[node] = viewIndex, but hold from the view rather than from the result. btw, i don't know how fast is the underlying js representation for that, that sounds like a sparse array, so *could* be memory inefficient rather then perf inefficient.

Would be really cool to solve this complexity without having to add further view properties to a result (and maybe also removing the existing ones).

Comment 15

[Marco Bonardo [:mak]]

PS: could be another followup unless we have an idea to solve that here in an elegant way.
Would be crazy to instead add a method to the view: needsUpdate(aChangingNode, &aNeesUpdate) so that is the view to tell the result if it should notify? But again now i'd expect the result to always notify and the view to get by itself if it should ignore the notification or not.

Comment 16

[Asaf Romano (gone)]

Attachment: close close...

Thanks to a lot of help from Boris, this actually works.

Remaining work:
1. few more comments from marco to address
2. make "icon" a string
3. tests

And that's it.

Comment 17

[Asaf Romano (gone)]

> why just not adding itemType to onItemChanged, this is exactly the reason why
> we are adding it in other notifications, so that observers don't have to catch
> it by themselves.

No, we're adding it so we don't have to find out the itemtype _twice_. Unlike other notifications, when onItemChanged is notified, the bookmarks service often doesn't have the item type handy. Also, the complex-query case is the only one in which the type is necessary, so I decided not to add itemType for now. Otherwise we'll call getItemType pretty often for no reason, which is exactly what i'm trying to avoid here.

I know this might be considered "ugly", but then again, I'm planning to privatize the bookmarks and history observers for 3.7.

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #17)
> No, we're adding it so we don't have to find out the itemtype _twice_.

Exactly to not force all observers to catch it again, and again, and again, for the same notification.

Unlike
> other notifications, when onItemChanged is notified, the bookmarks service
> often doesn't have the item type handy.

i don't mind if it does not have the type offhand, it's getting it once against getting it in any observer. And extensions can use it, could even be useful for Weave. Looking at code the points where i don't see an itemType offhand are: SetItemDateAdded, SetItemLastModified, SetItemTitle, onItemAnnotationSet, onItemAnnotationRemoved. Other code paths looks like having the type (or at least it is possible to guess it, since if an uri changes it won't be a folder or a separator!).

SetItemDateAdded and SetItemLastModified are practically only used by migration or restore, that are usually tasks for which saving a bunch of ms won't make a difference, setItemTitle is not called often, so i think it's not a problem to catch the type there. 
about onItemAnnotationSet and onItemAnnotationRemoved, why are they notifying, if someone is interested in looking at annotations changes, they should listen to annotations service.

Btw, all cases looks to me not often called methods, while often called methods have the itemType offhand, imo won't really hurt adding it to itemChanged.

You also say we need it for complex queries, that today are present in our UI, in many crucial points.

Comment 19

[Asaf Romano (gone)]

Attachment: useful checkpoint

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Mano: why did you nominate this for blocking? I think it's a nice-to-have, but I wouldn't hold Firefox 3.6 for this; perhaps you have a more compelling reason (such as evidence of what the Ts/Tp gains are expected to be?)

Comment 21

[Marco Bonardo [:mak]]

i'm pass-reviewing the patch in comment 19, will take some time since it's 180KB, btw i should be there by end of the PDT day.

Comment 22

[Marco Bonardo [:mak]]

Attachment: review for last iteration

I'm attaching the review to avoid polluting the bug.

Comment 23

[Asaf Romano (gone)]

> +  itemTitleChanged: function PTV_itemTitleChanged(aItem, aNewTitle) {
> +    this._invalidateValue(aItem, this.COLUMN_TYPE_TITLE);
> +  },
> do we notify the root node? in such a case we should ignore it since it's invisible

_invalidate(Cell)Value check for invisible nodes.

>     this._ensureValidRow(aRow);
> -    if (!PlacesUtils.nodeIsContainer(this._visibleElements[aRow].node))
> -      throw Cr.NS_ERROR_INVALID_ARG;
> -
> was this a useless check?

Yes. The tree implementation isn't that bad...

> not finding a column looks like an error, should we throw or ASSERT?

I've added the following comment
"// That's completely valid - most of our trees actually include just
 // the title column."

>> this is the only view that is not doing anything for livemarks on itemAnnotationChanged,
>> i guess if this is correct, should not we update >> the TITLE column for the livemark icon?

Maybe. I don't know. Please follow up on that.

>> should we clear _DOMElement here?

We shouldn't clear it anywhere now that we use expandos.

Comment 24

[Asaf Romano (gone)]

Attachment: update

This addresses marco's comments.

TODO:
 1. Add aItemType to few more methods.
 2. Fix the tests, again.

Comment 25

[Asaf Romano (gone)]

Attachment: ready for review, marco ;)

Comment 26

[Marco Bonardo [:mak]]

pushed to tryserver with changeset abb70fb988a5.

Comment 27

[Marco Bonardo [:mak]]

Debug build, 90MBs real places.sqlite.
Time to fill the history sidebar: before patch 47s, with patch something 42s (10% improvement).
buildVisibleSection takes about 7.5s
openContainer (from opening resultNode to containerOpened notification) takes 34s

So definately this helped removing overhead in the building part of the tree. still buildVisibleSection is slow because it's getting all nodes through XPCOM (getChild) to build the visible elements list.
Vast majority of time is still spent to query the db, get all results, create nodes and add them to the result. Here bug 490714 would help a lot.

Comment 28

[Marco Bonardo [:mak]]

my last wip patch in Bug 500391 was reducing the buildVisibleSection time moving the list build in cpp, that was saving about 2 seconds out of those 7.5, not a perfect solution since required to walk the list 2 times, but demonstrated that a lot of time is spent in crossing XPCOM due to getChild().
Btw, Mano should have plans to get rid of the visibleList thing in a followup, so that patch will most likely be useful only for search case, and should be unbitrotted after this one lands.

Comment 29

[Marco Bonardo [:mak]]

iirc the patch in bug 500391 was also speeding up some code path in filtering results, so we should accelerate on this big patch, to allow writing smaller improvements patches for the other paths, that atm are blocked to avoid bitrotting this one.

Comment 30

[Marco Bonardo [:mak]]

Sorry for bugspam, just putting down notes.
Talos tests have not showed any interesting improvement or regression, but that's expected since they don't measure Tsnappiness.
Most of the gains are in adding/removing/managing nodes in the views, something you can just measure visually while using the UI. Plus a lot of gain in code clarity, separation and cleanup.

Comment 31

[Marco Bonardo [:mak]]

Attachment: review for last iteration

Comment 32

[Marco Bonardo [:mak]]

Comment on attachment 401079 [details] [diff] [review]
ready for review, marco ;)

Review is attached above.

Please pass through my comments one-by-one, i see you forgot to fix some of the comments from my last review and i had to repeat them again.
But for next reviews i'd like to just look at the interdiff, so even if it will be tedious, be picky!

We are near a final patch, just 2 or 3 errors and some more small minor things.

Maybe next week i'll have less time, so please let's try to finish this for tomorrow.

Comment 33

[Asaf Romano (gone)]

> could you add names to these methods even if they are empty?

No, there's no value in maintaining arguments list for a no-op function. That's a great JS feature, not a bug.

Comment 34

[Marco Bonardo [:mak]]

yeah, i was talking about function labels, but nevermind.

Comment 35

[Asaf Romano (gone)]

> +namespace mozilla {
> +  namespace places {
> +    // Class-info and the scriptable helper are implemented in order to
> +    // allow the JS front-end code to set expando properties on result nodes.

s/helper/helpers/

Actually, we implement just one helper...

> +    class ResultNodeClassInfo : public nsIClassInfo,
> +                                public nsIXPCScriptable {

> comma on new line

We don't do it elsewhere and I don't think we should...

Comment 36

[Asaf Romano (gone)]

Attachment: address comments

Comment 37

[Marco Bonardo [:mak]]

(In reply to comment #35)
> s/helper/helpers/
> 
> Actually, we implement just one helper...

i misread the comment

> > +    class ResultNodeClassInfo : public nsIClassInfo,
> > +                                public nsIXPCScriptable {
> 
> > comma on new line
> 
> We don't do it elsewhere and I don't think we should...

oh we do, and we should, since makes blame better.

class ResultNodeClassInfo : public nsIClassInfo
                          , public nsIXPCScriptable
{

a lot of code uses this style today

Comment 38

[Marco Bonardo [:mak]]

Comment on attachment 401665 [details] [diff] [review]
address comments

r+ code wise, just a few nits.

Now i'll retest functionality and push again to the tryserver.

diff --git a/browser/components/places/content/menu.xml b/browser/components/places/content/menu.xml

+        nodeRemoved: function PMV_nodeRemoved(aParentNode, aNode, aIndex) {
+
+          // Figure out if we need to show the "<Empty>" menu-item.
+          // XXX Bug 517701: This doesn't seem to handle the case of an empty
+          // root (parentElt == this._self.parentNode).

s/XXX/TODO/

+        nodeMoved:
+        function PMV_nodeMoved(aNode,
+                               aOldParent, aOldIndex,
+                               aNewParent, aNewIndex) {
+          // Note: the current implementation of moveItem does not actually
+          // use this notification when the item in question is moved from one
+          // folder to another.  Instead, it calls nodeRemoved and nodeInserted
+          // for the two folders.  Thus, we can assume aOldParent==aNewParent.

spacing aOldParent == aNewParent

diff --git a/browser/components/places/content/toolbar.xml b/browser/components/places/content/toolbar.xml

+        nodeMoved:
+        function TV_V_nodeMoved(aNode,
+                                aOldParent, aOldIndex,
+                                aNewParent, aNewIndex) {
+          // Note: the current implementation of moveItem does not actually
+          // use this notification when the item in question is moved from one
+          // folder to another.  Instead, it calls nodeRemoved and nodeInserted
+          // for the two folders.  Thus, we can assume aOldParent==aNewParent.

ditto

diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js

+  // This is used in nodeRemoved and nodeMoved to fix viewIndex values.
+  // Throw if the node has an invalid viewIndex.
+  _fixViewIndexOnRemove: function PTV_fixViewIndexOnRemove(aNode,
+                                                           aParentNode) {

s/Throw/Throws/

+  _columns: [],
+  _findColumnByType: function(aColumnType) {

could you label this method?

diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp

+namespace mozilla {
+  namespace places {
+    // Class-info and the scriptable helper are implemented in order to
+    // allow the JS frontend code to set expando properties on result nodes.
+    class ResultNodeClassInfo : public nsIClassInfo,
+                                public nsIXPCScriptable
+    {

as said in the comment above for code style

 NS_IMETHODIMP
 nsNavHistoryFolderResultNode::OnEndUpdateBatch()
 {
   return NS_OK;
 }
 
 
-NS_IMETHODIMP
-nsNavHistoryFolderResultNode::OnItemAdded(PRInt64 aItemId,
-                                         PRInt64 aFolder,
-                                         PRInt32 aIndex)
-{
-  nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
-  NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY);
-  PRUint16 itemType;
-  nsresult rv = bookmarks->GetItemType(aItemId, &itemType);
-  NS_ENSURE_SUCCESS(rv, rv);
-  return OnItemAdded(aItemId, aFolder, aIndex, itemType);
-}
-
-
 // nsNavHistoryFolderResultNode::OnItemAdded (nsINavBookmarkObserver)
 
looks like double new line above

diff --git a/toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js b/toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js

-  // nsINavHistoryResultViewer.itemChanged
-  // adding a visit causes itemChanged for the folder
-  do_check_eq(root.uri, viewer.changedItem.uri);
+  // nsINavHistoryResultViewer.nodeHisotryDetailsChanged
+  // adding a visit causes nodeHisotryDetailsChanged for the folder

double typo: nodeHisotryDetailsChanged

Comment 39

[Marco Bonardo [:mak]]

Looks like something is wrong with the contextual menus on the toolbar after i open the customize dialog.

All commands are disabled, looks like the old controller is in the middle :(

Comment 40

[Marco Bonardo [:mak]]

still a compile problem on Linux, namespace definitions don't want semicolons
namespace name {
  xxx
}

NEXT ERROR /builds/slave/sendchange-linux-unittest/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:192: error: extra ;
/builds/slave/sendchange-linux-unittest/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:193: error: extra ;

and a warning

/builds/slave/sendchange-linux-unittest/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp: In member function PRBool nsNavHistoryContainerResultNode::AreChildrenVisible():
/builds/slave/sendchange-linux-unittest/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:519: warning: unused variable ancestorIsClosed

Comment 41

[Asaf Romano (gone)]

I couldn't reproduce the toolbar+cust issue, STR would help.

Comment 42

[Asaf Romano (gone)]

OK, I was able to reproduce it and also find out what's causing that.

The old controller actually _isn't_ in the middle. It's removed by cust (likely when the toolbar element is wrapped) and isn't added back. This worked before due to the extra bindings. I'll need to re-add the controller in the ctor.

Fix coming...

Comment 43

[Marco Bonardo [:mak]]

(In reply to comment #42)
yeah my strs were quite easy, just right click on a  bookmark in the toolbar check that delete is enabled. open customize. Repeat the check, delete is now disabled.
Notice tomorrow i won't be around in the afternoon, but should be after midnight GMT+1, and tomorrow, for review.

Comment 44

[Asaf Romano (gone)]

It's a minimal change so I don't need one, waiting for mconnor's sr.

Comment 45

[Asaf Romano (gone)]

Attachment: all fixed.

Comment 46

[Marco Bonardo [:mak]]

Comment on attachment 401920 [details] [diff] [review]
all fixed.

>diff --git a/browser/components/places/content/toolbar.xml b/browser/components/places/content/toolbar.xml
> 
>-        if (this.hasAttribute("place")) {
>-          // Do the initial build.
>-          this.place = this.place;
>-        }
>+          // Attach the places controller

Places uppercase, and with period.

>+          if (!this._controller) 
>+            this._controller = new PlacesController(this);
>+
>+          this.controllers.appendController(this._controller);
>+
>+          this._scrollbox.addEventListener("overflow", this, false);
>+          this._scrollbox.addEventListener("underflow", this, false);
>+          window.addEventListener("resize", this, false);

i prefer the listeners being set before the place is, they should be always in place when we rebuild the toolbar

>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp

>+namespace mozilla {
>+  namespace places {
>+    // Class-info and the scriptable helper are implemented in order to
>+    // allow the JS frontend code to set expando properties on result nodes.
>+    class ResultNodeClassInfo : public nsIClassInfo
>+                                , public nsIXPCScriptable

there's one more spacing than needed before the comma, it should be aligned with :

Comment 47

[Mike Connor [:mconnor]]

Comment on attachment 401920 [details] [diff] [review]
all fixed.


>+   *        Wether or not aProperty the name of an item annotation.

whether

overall, I'm a little leery of the method renames for compat, but we're changing the API enough that the names aren't the biggest worry.

Make sure MDC gets the changes!

Comment 48

[Asaf Romano (gone)]

Attachment: as checked in

Comment 49

[Asaf Romano (gone)]

changest 149c3820e8a8 on mozilla-central

Comment 50

[Dietrich Ayala (:dietrich)]

please detail the regression risk, any possible compat issues, and how well tested this is (flag not set?!).

Comment 51

[Asaf Romano (gone)]

Well, Marco and I tested it over the last few weeks and caught most/all regression. So far no regressions were reported on trunk.

I'm marking this in-testsuite+, because I've updated the tests to use the new methods (plus, the tests use treeView which relies on these changes). Having said that, there's a lot of room for improvement in views-testing. I'll file a bug.

As for compatibility: We've changed the viewer interface much, but as far as I can tell it's not used widely. I'm going to update MDC tonight on these changes. We've also removed some obsolete methods (removeFolder, removeItemAt) from the bookmarks service, we can bring them back for the brenach version

Comment 52

[Asaf Romano (gone)]

mfinkle: can you check how many addons use result.viewer (nsINavHistoryResultViewer)?

Comment 53

[Asaf Romano (gone)]

well, except bug 519186...

Comment 54

[Dietrich Ayala (:dietrich)]

(In reply to comment #51)
> I'm marking this in-testsuite+, because I've updated the tests to use the new
> methods (plus, the tests use treeView which relies on these changes). Having
> said that, there's a lot of room for improvement in views-testing. I'll file a
> bug.

ok please make the followup bug block this. given the scope of these changes, it'd be great to get better testing here asap.

> As for compatibility: We've changed the viewer interface much, but as far as I
> can tell it's not used widely. I'm going to update MDC tonight on these
> changes. We've also removed some obsolete methods (removeFolder, removeItemAt)
> from the bookmarks service, we can bring them back for the brenach version

ok. please make the branch patch and request approval on that.

Comment 55

[Marco Bonardo [:mak]]

removeFolder was already marked as deprecated in the idl for 3.5, i think we can go on with getting rid of it in 3.6.

Comment 56

[Shawn Wilsher :sdwilsh]

(In reply to comment #55)
> removeFolder was already marked as deprecated in the idl for 3.5, i think we
> can go on with getting rid of it in 3.6.
Well, we are considering giving users 3.6 as a minor update to 3.5, so maybe that's not such a good idea.

Comment 57

[(mostly gone) XtC4UaLL [:xtc4uall]]

this checkin broke the extension "Sage++ (Higmmer's Edition)" http://blog-imgs-30-origin.fc2.com/h/i/m/himag/sagepp-higmmersedition-install.html?lang=en wrt checking for updated feeds and enabling/disabling the "Show only updated Feeds" option. it used to be the only Sage derivative that worked on trunk so far.

since i'm not a dev i don't know what particular change broke it, if it's an expected, indented ext compat issue and if it's worth filing a separate bug report =).

Comment 58

[Marco Bonardo [:mak]]

extension incompatibilities do not need special bug reports, new versions of the browser are usually known to break compatibility and is up to ext dev to update their extensions. Sadly if we want to have a better and faster interface we have to change things.

Comment 59

[Brad Lassey [:blassey] (use needinfo?)]

this landing seems to have cause a startup crash on windows mobile.  See bug 519854.

Comment 60

[Dietrich Ayala (:dietrich)]

backed out due to the crash. blassey, please update bug 519854 w/ stacks and as much information as possible to help people w/o winmo figure out the problem here.

http://hg.mozilla.org/mozilla-central/rev/e12943b9ce6a

Comment 61

[Dietrich Ayala (:dietrich)]

relanded: http://hg.mozilla.org/mozilla-central/rev/513e9fc142e5

Comment 62

[Marco Bonardo [:mak]]

Comment on attachment 403017 [details] [diff] [review]
as checked in

wrongly cleared approval request.

Comment 63

[Asaf Romano (gone)]

Comment on attachment 403017 [details] [diff] [review]
as checked in

dietrich cleared it becuase he wants to see the branch version.

Comment 64

[Asaf Romano (gone)]

Attachment: 1.9.2 patch

* undo the removal of propertyBags and deprecated methods support
 * includes the fix for bug 519186
 * add the depracted methods back, but mark them as such
 * I did not revert URI-based node.uri, since it does hurt perf.
 * I did not add back node.viewIndex because it's now magically supported through expandos.

Comment 65

[Asaf Romano (gone)]

revert->restore in the last comment.

I've also added back the showSessions property.

Comment 66

[Dietrich Ayala (:dietrich)]

Comment on attachment 404426 [details] [diff] [review]
1.9.2 patch

had bake time, regressions fixed and included, has test coverage, big snappiness win -> a+.

Comment 67

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/54ae7d98c34e

Comment 68

[Eric Shepherd [:sheppy]]

All these interface changes have now been documented.

Comment 69

[Gervase Markham [:gerv]]

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