547815 - Port Bug 520659 (Lazily build places trees when possible) to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Asaf Romano (gone)]

In bug 520659 we've improved treeview performance bt 50%.   Note that since nothing has changed in the back-end (except the new findNodeByDetails method), the old trevview code which SM uses will still work..

Comment 1

[Robert Kaiser]

Bug 520659 got backed out again so we need to wait for the new patch to land before doing this.

Comment 2

[Robert Kaiser]

The other problem here is manually porting every line to our treeView.js as review forced me to change it enough that it almost doesn't resemble the original one.

Comment 3

[Robert Kaiser]

I should have code locally that ports what the current patch has. Lots of useless changes from var to let confuse the original patch somewhat, but I should have sorted out most of what we need. I'll wait on review on the original and do some testing before attaching the patch here, though.

Comment 4

[Marco Bonardo [:mak]]

i think you should just look at changes to the interfaces like findNodeByDetails or GetChildIndex.
As Mano said your treeview should still work, but this new implementation in FX (if you take it as a whole) will be quite faster than the old one.
There is quite some clean-up ongoing on Places lately, sorry.

Comment 5

[Robert Kaiser]

(In reply to comment #4)
> There is quite some clean-up ongoing on Places lately, sorry.

I appreciate cleanup, but using let in the toplevel of functions is considered noise around here (ask Neil about that, if you want, we need to pass his strict review rules).

Comment 6

[Robert Kaiser]

Attachment: sync SeaMonkey places history to Firefox places work

This incorporates patches for both bug 531696 (which I guess should dupe here now) and bug 520659, i.e. for a hopefully complete update to Firefox places work.

I noticed when testing that sorting of date groups is broken, but it's already broken before my patch, so I guess it's better to get this in and fix it afterwards. Should probably file a bug on that - Neil, as you're the expert for grouping code, I hope you can take a look at that.

Comment 7

[[e-mail]]

(In reply to comment #6)
> I noticed when testing that sorting of date groups is broken, but it's already
> broken before my patch, so I guess it's better to get this in and fix it
> afterwards. Should probably file a bug on that - Neil, as you're the expert for
> grouping code, I hope you can take a look at that.
Broken? Expected behaviour is that neither date nor host groups are affected by changes in sort order.

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> (In reply to comment #6)
> > I noticed when testing that sorting of date groups is broken, but it's already
> > broken before my patch, so I guess it's better to get this in and fix it
> > afterwards. Should probably file a bug on that - Neil, as you're the expert for
> > grouping code, I hope you can take a look at that.
> Broken? Expected behaviour is that neither date nor host groups are affected by
> changes in sort order.

Sort order is by date group title instead of time they represent. I filed bug 548920 for that.

Comment 9

[[e-mail]]

Comment on attachment 429174 [details] [diff] [review]
sync SeaMonkey places history to Firefox places work

I guess most of these need to be answered by someone on the Places team.

>+   * @note It's guaranteed that all containers are listed in the rows
>+   * elements array.  It's also guaranteed that separators (if they're not
>+   * filtered, see below) are listed in the visible elements array, because
>+   * bookmark folders are never built lazily, as described above.
Nit: no bookmark folders or separators in history anyway.

>+    if (aContainer._plainContainer !== undefined)
>+      return aContainer._plainContainer;
I'm not convinced we need to cache this, because ...

>+    // We don't know enough about non-query containers.
>+    if (!(aContainer instanceof Components.interfaces.nsINavHistoryQueryResultNode))
... all of our containers are query containers. In fact, we only have four types of container, DATE_QUERY, SITE_QUERY, DATE_SITE_QUERY and URI. Only the URI container is a plain container. So that simplifies things somewhat.

>+   *        node which isn't supposed to be in the tree (e.g. separators in
No separators in history.

>+   *        @see isPlainContainer.
_isPlainContainer

>+    let parent = aNode.parent;
>+    if (!parent || !parent.containerOpen)
What about DATE_SITE_QUERY where the node could have a closed grandparent?

>+    // Non-plain containers are initially built with their contents.
>+    let parentIsPlain = this._isPlainContainer(parent);
>+    if (!parentIsPlain) {
>+      if (parent == this._rootNode)
>+        return this._rows.indexOf(aNode);
>+
>+      return this._rows.indexOf(aNode, aParentRow);
Won't this._rows.indexOf(aNode, aParentRow); work in the rootNode case anyway?

>+    let row = -1;
>+    let useNodeIndex = typeof(aNodeIndex) == "number";
>+    if (parent == this._rootNode)
>+      row = useNodeIndex ? aNodeIndex : this._rootNode.getChildIndex(aNode);
>+    else if (useNodeIndex && typeof(aParentRow) == "number") {
>+      // If we have both the row of the parent node, and the node's index, we
>+      // can avoid searching the rows array if the parent is a plain container.
>+      row = aParentRow + aNodeIndex + 1;
>+    }
>+    else {
>+      // Look for the node in the nodes array.  Start the search at the parent
>+      // row.  If the parent row isn't passed, we'll pass undefined to indexOf,
>+      // which is fine.
>+      row = this._rows.indexOf(aNode, aParentRow);
>+      if (row == -1 && aForceBuild) {
>+        let parentRow = typeof(aParentRow) == "number" ? aParentRow
>+                                                       : this._getRowForNode(parent);
>+        row = parentRow + parent.getChildIndex(aNode) + 1;
>+      }
>+    }
This is giving me a headache. I need to go back and look at it again.

>+  _getParentByChildRow: function PTV__getParentByChildRow(aChildRow) {
I'm not convinced of the utility of this function, so I'll come up with a rewrite. In particular it has two callers; one already knows the parent node, while the other doesn't need to force the node to exist.

>+  _rootNode: null,
Strange place to put this; the constructor looks slightly less unreasonable.

>+    this._rows =
>+      this._rows.slice(0, aFirstChildRow).concat(newElements)
>+          .concat(this._rows.slice(aFirstChildRow, this._rows.length));
There is in fact a very slightly neater way of writing this:
this._rows = this._rows.splice(0, aFirstChildRow)
                       .concat(newElements, this._rows);

>-                     PlacesUIUtils.localStore.HasAssertion(resource, openLiteral,
>+                     PlacesUIUtils.localStore.HasAssertion(resource,
>+                                                           openLiteral,
>                                                            trueLiteral, true);
???

>+          rowsAddedCounter += this._buildVisibleSection(curChild, aToOpen,
>+                                                        row + 1);
rowsInsertedCounter, no? [Although I don't understand the Counter suffix.]

>+    if (node === undefined ||
>+        !(node instanceof Components.interfaces.nsINavHistoryContainerResultNode))
Nit: instanceof duplicates undefined test

>+  _getNewRowForRemovedNode:
>+  function PTV__getNewRowForRemovedNode(aUpdatedContainer, aOldNode) {
>+    var parent = aOldNode.parent;
>+    if (parent) {
>+      // If the node's parent is still set, the node is not obsolete
>+      // and we should just find out its new position.  However, if the node's
>+      // parent is closed, the node is invisible.
[What about grandparents in date site queries?]

>+    if (aNodesInfo.length == 1 && selection.getRangeCount() == 0) {
[Could use selection.count instead.]

>+      let row = Math.min(aNodesInfo[0].oldRow, this._rows.length - 1);
>+      selection.rangedSelect(row, row, true);
>+      if (aNodesInfo[0].wasVisible && scrollToRow == -1)
>+        scrollToRow = aNodesInfo[0].oldRow;
I don't quite understand this. The scrollToRow isn't the selected row?

>-    selection.selectEventsSuppressed = false;
I assume the callers are doing this now?

>+  nodeInserted: function PTV_nodeInserted(aParentNode, aNode, aNewIndex) {
More stuff I need to read later.

>+  // This is _invalidateCellValue in the original implementation.
>+  _invalidateNode: function PTV__invalidateNode(aNode) {
Why was this moved and renamed?

>-  get result() {
>-    return this._result;
>-  },
>-
>+  get result() this._result,
Why this change?

>   nodeForTreeIndex: function PTV_nodeForTreeIndex(aIndex) {
>-    if (aIndex > this._visibleElements.length)
>+    if (aIndex > this._rows.length)
>       throw Components.results.NS_ERROR_INVALID_ARG;
> 
>-    return this._visibleElements[aIndex].node;
>+    return this._getNodeForRow(aIndex);
Why doesn't this need to force the node to exist?

>+  get selection() this._selection,
>+  set selection(val) this._selection = val,
> 
>-  get selection() {
>-    return this._selection;
>-  },
>+  getRowProperties: function() { },
> 
>-  set selection(val) {
>-    return this._selection = val;
>-  },
>-
>-  getRowProperties: function PTV_getRowProperties(aRow, aProperties) { },
Well, I guess for getRowProperties you're just adding the missing name...

>+    if (!node)
>+      return false;
I notice some places use node === undefined. I prefer this way :-)

>-        var parent = node.parent;
>-        if((PlacesUtils.nodeIsQuery(parent) ||
>-            PlacesUtils.nodeIsFolder(parent)) &&
>-           !node.hasChildren)
>+        let parent = node.parent;
>+        if ((PlacesUtils.nodeIsQuery(parent) ||
>+             PlacesUtils.nodeIsFolder(parent)) &&
>+            !node.hasChildren)
I guess the if is a whitespace/indentation fix...

>+      let rowNode = this._getNodeForRow(i);
Bah, this creates all the intervening nodes...

>-    var icon = node.icon;
>-    if (icon)
>-      return icon.spec;
>-    return "";
>+    return this._getNodeForRow(aRow).icon;
Hmm, so there were some other breaking changes?

>+    var node = this._rows[aRow];
Ironically I expected this to use _getNodeForRow.

Comment 10

[Robert Kaiser]

(In reply to comment #9)
> (From update of attachment 429174 [details] [diff] [review])
> I guess most of these need to be answered by someone on the Places team.

Mano/mak:
Can you help there?


> ... all of our containers are query containers. In fact, we only have four
> types of container, DATE_QUERY, SITE_QUERY, DATE_SITE_QUERY and URI. Only the
> URI container is a plain container. So that simplifies things somewhat.

and RESULTS_AS_URI doesn't even have folders, right?


> What about DATE_SITE_QUERY where the node could have a closed grandparent?

> Won't this._rows.indexOf(aNode, aParentRow); work in the rootNode case anyway?

I'll leave those to Mano/mak.


> This is giving me a headache. I need to go back and look at it again.

Heh, I can't react much to that ;-)


> >+  _getParentByChildRow: function PTV__getParentByChildRow(aChildRow) {
> I'm not convinced of the utility of this function, so I'll come up with a
> rewrite. In particular it has two callers; one already knows the parent node,
> while the other doesn't need to force the node to exist.

A general rewrite or one for us only? To land before or after my patch?


> >+  _rootNode: null,
> Strange place to put this; the constructor looks slightly less unreasonable.

True, I just kept it in snyc with Firefox.
Mano/mak: Any special reason for that placement?


> >+    this._rows =
> >+      this._rows.slice(0, aFirstChildRow).concat(newElements)
> >+          .concat(this._rows.slice(aFirstChildRow, this._rows.length));
> There is in fact a very slightly neater way of writing this:
> this._rows = this._rows.splice(0, aFirstChildRow)
>                        .concat(newElements, this._rows);

Will apply it here.
Mano/mak: should this get done in Firefox as well?


> >-                     PlacesUIUtils.localStore.HasAssertion(resource, openLiteral,
> >+                     PlacesUIUtils.localStore.HasAssertion(resource,
> >+                                                           openLiteral,
> >                                                            trueLiteral, true);
> ???

Tried to break at 80 chars here, but I guess it's a bit useless in this case.


> >+          rowsAddedCounter += this._buildVisibleSection(curChild, aToOpen,
> >+                                                        row + 1);
> rowsInsertedCounter, no? [Although I don't understand the Counter suffix.]

Actually, that same bug is in Firefox!
Mano/mak: I guess you should fix that!


> >+    if (node === undefined ||
> >+        !(node instanceof Components.interfaces.nsINavHistoryContainerResultNode))
> Nit: instanceof duplicates undefined test

Another thing Mano/mak also want to fix in Firefox as well, I think. :)


> [What about grandparents in date site queries?]

Mano/mak?


> >+    if (aNodesInfo.length == 1 && selection.getRangeCount() == 0) {
> [Could use selection.count instead.]

I guess you mean instead getRangeCount() - probably could, but does it make any real difference? I'd like to not diverge too far from Firefox for further porting when needed.


> >+      let row = Math.min(aNodesInfo[0].oldRow, this._rows.length - 1);
> >+      selection.rangedSelect(row, row, true);
> >+      if (aNodesInfo[0].wasVisible && scrollToRow == -1)
> >+        scrollToRow = aNodesInfo[0].oldRow;
> I don't quite understand this. The scrollToRow isn't the selected row?

> >-    selection.selectEventsSuppressed = false;
> I assume the callers are doing this now?

Mano/mak?

 
> >+  nodeInserted: function PTV_nodeInserted(aParentNode, aNode, aNewIndex) {
> More stuff I need to read later.

OK...

> >+  // This is _invalidateCellValue in the original implementation.
> >+  _invalidateNode: function PTV__invalidateNode(aNode) {
> Why was this moved and renamed?

Moved to match the location in the file where Firefox does bascially the same thing, for ease of future porting. Renamed because it's internal only, so it deserves an underscore prefix.


> >-  get result() {
> >-    return this._result;
> >-  },
> >-
> >+  get result() this._result,
> Why this change?

Just to be in sync with Firefox. I can undo them if you want.


> Why doesn't this need to force the node to exist?

Mano/mak?


> >+  getRowProperties: function() { },
> >-  getRowProperties: function PTV_getRowProperties(aRow, aProperties) { },
> Well, I guess for getRowProperties you're just adding the missing name...

Adding? I synched with FF not having anything there...


> >+    if (!node)
> >+      return false;
> I notice some places use node === undefined. I prefer this way :-)

Sure, can do that.
Mano/mak: Want to do the same on Firefox?


> >-        var parent = node.parent;
> >-        if((PlacesUtils.nodeIsQuery(parent) ||
> >-            PlacesUtils.nodeIsFolder(parent)) &&
> >-           !node.hasChildren)
> >+        let parent = node.parent;
> >+        if ((PlacesUtils.nodeIsQuery(parent) ||
> >+             PlacesUtils.nodeIsFolder(parent)) &&
> >+            !node.hasChildren)
> I guess the if is a whitespace/indentation fix...

yes, just that. I guess you would usually be the first to call out if( as a review nit. ;-)


> >+      let rowNode = this._getNodeForRow(i);
> Bah, this creates all the intervening nodes...

?


> >-    var icon = node.icon;
> >-    if (icon)
> >-      return icon.spec;
> >-    return "";
> >+    return this._getNodeForRow(aRow).icon;
> Hmm, so there were some other breaking changes?

Not sure what you are talking about here...


> >+    var node = this._rows[aRow];
> Ironically I expected this to use _getNodeForRow.

Mano/mak?

Comment 11

[Robert Kaiser]

Comment on attachment 429174 [details] [diff] [review]
sync SeaMonkey places history to Firefox places work

Now that we have this flag, requesting feedback? from Mano. See the "Mano/mak" questions in comment #10 for what Neil actually wants to know there.

Comment 12

[Asaf Romano (gone)]

(I'll review this today.)

Comment 13

[Marco Bonardo [:mak]]

About history simplifications, as i said to Robert, i think there is no point in removing bookmarks related stuff when your current plan is to move to Places bookmarks and in future merge treeviews implementations. If i should make a choice i'd retain the full treeview functionality without simplifying it for history only.

I think instead makes sense that Mano takes meaningful Neil's comments, and backport them to Places treeview implementation in a new bug, then you take back again our impl that will be fixed for you too.

Comment 14

[Asaf Romano (gone)]

>>+    // Non-plain containers are initially built with their contents.
>>+    let parentIsPlain = this._isPlainContainer(parent);
>>+    if (!parentIsPlain) {
>>+      if (parent == this._rootNode)
>>+        return this._rows.indexOf(aNode);
>>+
>>+      return this._rows.indexOf(aNode, aParentRow);
> Won't this._rows.indexOf(aNode, aParentRow); work in the rootNode case anyway?

Probably.  However,  In Fx, we didn't want to force any value of aParentRow when it's meaningless (i.e. when there's no parent). Therefore, I'll leave it as-is for now.

>> >-    selection.selectEventsSuppressed = false;
>> I assume the callers are doing this now?

Yes.

>>   nodeForTreeIndex: function PTV_nodeForTreeIndex(aIndex) {
>>-    if (aIndex > this._visibleElements.length)
>>+    if (aIndex > this._rows.length)
>>       throw Components.results.NS_ERROR_INVALID_ARG;
>> 
>>-    return this._visibleElements[aIndex].node;
>>+    return this._getNodeForRow(aIndex);
> Why doesn't this need to force the node to exist?

It does... you can only avoid forcing with getRowForNode.

>> >+    var node = this._rows[aRow];
> Ironically I expected this to use _getNodeForRow.

Not sure what instance you're referring here.

Comment 15

[Asaf Romano (gone)]

Attachment: Fix Fx

Thanks for you input!

Comment 16

[Asaf Romano (gone)]

Comment on attachment 435260 [details] [diff] [review]
Fix Fx

Moved to bug 555293.

Comment 17

[Asaf Romano (gone)]

Comment on attachment 429174 [details] [diff] [review]
sync SeaMonkey places history to Firefox places work

Fed+

Comment 18

[Asaf Romano (gone)]

For what it's worth, I second Marco's position in comment 13.

Comment 19

[Robert Kaiser]

Attachment: v2: update to Neil's comments and Mano's Firefox patch

This patch updates the work to Neil's comments and Mano's new patch for the Firefox side.

Neil, is this OK with the answers/comments Marco and Mano gave here?

Comment 20

[Robert Kaiser]

Attachment: v2.1: incorporate async API stub

This patch includes the stub for the async API as introduced in bug 536893.

Comment 21

[Robert Kaiser]

OK, unless something really breaks, I'll not port any more places changes to history until bug 498596 lands and we can make both bookmarks and history use one set of common places files (esp. treeView, utils.js/PlacesUIUtils.jsm, etc.).

Comment 22

[[e-mail]]

Comment on attachment 438798 [details] [diff] [review]
v2.1: incorporate async API stub

>+    let parent = aNode.parent;
>+    if (!parent || !parent.containerOpen)
>+      throw "Invisible node passed to _getRowForNode";
Shouldn't this use nodeAncestors?

>+          wasVisbile: i >= firstVisibleRow && i <= lastVisibleRow
wasVisible

>-  nodeInserted: function PTV_nodeInserted(aParent, aNode, aNewIndex) {
>+    nodeInserted: function PTV_nodeInserted(aParentNode, aNode, aNewIndex) {
Bogus indentation change.

>+      let separatorsAreHidden = PlacesUtils.nodeIsSeparator(aNode) &&
>+        this._result.sortingMode != Components.interfaces.nsINavHistoryQueryOptions.SORT_BY_NONE;
[Never true in history.]

>+    var rowToSelect = Math.min(oldRow,  this._rows.length - 1);
Nit: doubled space after oldRow,

>+  _invalidateNode: function PTV__invalidateNode(aNode) {
Without the _ changes please.

>-  get result() {
>-    return this._result;
>-  },
>-
>+  get result() this._result,
(and similar) More unnecessary changes.

>       // treat non-expandable childless queries as non-containers
[I wonder how I let that slip in there...]

>+    var node = this._rows[aRow];
Ah, so toggleOpenState assumes that the row is a container.

Comment 24

[Robert Kaiser]

(In reply to comment #22)
> (From update of attachment 438798 [details] [diff] [review])
> >+    let parent = aNode.parent;
> >+    if (!parent || !parent.containerOpen)
> >+      throw "Invisible node passed to _getRowForNode";
> Shouldn't this use nodeAncestors?

mak says "I think the point is that we cannot have a closed container containing an open container, thus checking the first parent should be enough".

> >+          wasVisbile: i >= firstVisibleRow && i <= lastVisibleRow
> wasVisible

Good catch - this is wrong in Firefox as well!

> >-  nodeInserted: function PTV_nodeInserted(aParent, aNode, aNewIndex) {
> >+    nodeInserted: function PTV_nodeInserted(aParentNode, aNode, aNewIndex) {
> Bogus indentation change.

oops.

> >+  _invalidateNode: function PTV__invalidateNode(aNode) {
> Without the _ changes please.

Er, had missed to change the callers anyhow...

> >-  get result() {
> >-    return this._result;
> >-  },
> >-
> >+  get result() this._result,
> (and similar) More unnecessary changes.

Eww, I liked how clean the new ones looked. Well, whatever, as long as review is satisfied.

> >       // treat non-expandable childless queries as non-containers
> [I wonder how I let that slip in there...]

I guess the bookmarks work would bring it back anyhow ;-)

> >+    var node = this._rows[aRow];
> Ah, so toggleOpenState assumes that the row is a container.

I guess it's pretty sure it's not called if it's not.

Comment 25

[Robert Kaiser]

Attachment: v2.2: update for latest review comments

> >+      let separatorsAreHidden = PlacesUtils.nodeIsSeparator(aNode) &&
> >+        this._result.sortingMode != Components.interfaces.nsINavHistoryQueryOptions.SORT_BY_NONE;
> [Never true in history.]

I hope I solved that one correctly, as it has a cascade effect on the for loop which doesn't need to loop any more from what I'm seeing.

Anyhow, things like this all will come back again with the places bookmarks work...

Comment 26

[[e-mail]]

mak, then why does getNewRowForRemovedNode need to use nodeAncestors?

Comment 27

[Marco Bonardo [:mak]]

I'm not completely sure of the answer, asking Mano would be much better since I could miss some thought behind that.  To me sounds like getNewRowForRemovedNode acts on a detached node (part of a detached structure) and detacheds node have live-updating problems, thus weird things could happen.

Comment 28

[Robert Kaiser]

Attachment: also use nodeAncestors

OK, I'm just going for nodeAncestors as Mano doesn't seem to be reachable for an answer. I'll let him review the port of this to Firefox, though.

Comment 29

[Robert Kaiser]

Attachment: v2.4: keep parent defined

Sorry, the last version of this patch had a bug that broke a few things. We need to keep parent defined as it's being used in a few other places in that function.

Comment 30

[Robert Kaiser]

Thanks, pushed as http://hg.mozilla.org/comm-central/rev/1b14adf0e98a

\o/