613034 - Port browser parts of |Bug 472343 - Managing multiple bookmarks in the Library is very slow| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

The Sync back-end uses batching, so we need to port the browser/ parts of this:
http://hg.mozilla.org/mozilla-central/rev/4503884294a7

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

AFAICS we have treeView.js under both places/ and history/ and both need to be patched.

Comment 2

[[e-mail]]

Comment on attachment 499708 [details] [diff] [review]
patch

Bah, the code doesn't obey the (badly named) interface :s

[It should be something like void setBatching(aIsBatching); ]

Comment 3

[[e-mail]]

Comment on attachment 499708 [details] [diff] [review]
patch

>+  batching: function PTV__batching(aToggleMode) {
[Why the __? I notice sortingChanged also gets this wrong.]

>+      if (this.selection) {
>+        this.selection.selectEventsSuppressed = true;
>+      }
>+      this._tree.beginUpdateBatch();
[I don't think you can have a tree yet not have a selection...]

>+    else if (this._inBatchMode) {
[Sigh. Why is this testing the existing mode only half of the time?]

Comment 4

[[e-mail]]

[See also my rants in bug 472343.]

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> >+  batching: function PTV__batching(aToggleMode) {
> [Why the __? I notice sortingChanged also gets this wrong.]

I guess the original author didn't get that the double underscore is just for private methods that already have an underscore in their function name. I don't care much either way, so if you want me to change it, I will.

> [I don't think you can have a tree yet not have a selection...]

It happened to me more than once (but hardly reproducibly) with the MailNews folder pane that the selection was gone, so that deleting messages didn't work anymore even though there was a selection in the thread pane. I'm not sure whether there was actually no selection in the back-end, though.

> >+    else if (this._inBatchMode) {
> [Sigh. Why is this testing the existing mode only half of the time?]

Well, in the other case they probably thought it doesn't matter because there it's set to true in any case. Anyway, I'm only speculating about things I've been trying to port 1:1. No what? Wait for an answer on the base bug? That might never come...

Comment 6

[Robert Kaiser]

(In reply to comment #1)
> AFAICS we have treeView.js under both places/ and history/ and both need to be
> patched.

If we need it in both right now, yes. Else the plans are to switch history to use the places/ code some time (probably post-2.1, bug is filed) - though someone else than me will probably need to pick that one up.

Comment 7

[Robert Kaiser]

(In reply to comment #5)
> (In reply to comment #3)
> > [I don't think you can have a tree yet not have a selection...]
> 
> It happened to me more than once (but hardly reproducibly) with the MailNews
> folder pane that the selection was gone, so that deleting messages didn't work
> anymore even though there was a selection in the thread pane. I'm not sure
> whether there was actually no selection in the back-end, though.

Forgot to comment on that - from all my work with trees in Data Manager and others it looks like they changed tree code in Mozilla 2.0 in a way that the whole selection object is being destroyed if the tree is empty or something. I had errors of undefined objects cropping up in code that worked fine in earlier versions.

Comment 8

[[e-mail]]

(In reply to comment #5)
> (In reply to comment #3)
> > >+  batching: function PTV__batching(aToggleMode) {
> > [Why the __? I notice sortingChanged also gets this wrong.]
> I guess the original author didn't get that the double underscore is just for
> private methods that already have an underscore in their function name. I don't
> care much either way, so if you want me to change it, I will.
Actually I don't like the double underscores for private methods either.

> > [I don't think you can have a tree yet not have a selection...]
> It happened to me more than once (but hardly reproducibly) with the MailNews
> folder pane that the selection was gone, so that deleting messages didn't work
> anymore even though there was a selection in the thread pane. I'm not sure
> whether there was actually no selection in the back-end, though.
What I meant was that setTree and setSelection are always called together. See
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp?mark=525,530#516

> > >+    else if (this._inBatchMode) {
> > [Sigh. Why is this testing the existing mode only half of the time?]
> Wait for an answer on the base bug? That might never come...
Well, there are a number of options:
1. Leave the code as is and assume the caller gets things right
2. Remove _inBatchMode and assume the caller gets things right
3. Test _inBatchMode before setting it in case the caller gets things wrong
4. Use selectEventsSuppressed otherwise as 3.

(In reply to comment #7)
> Forgot to comment on that - from all my work with trees in Data Manager and
> others it looks like they changed tree code in Mozilla 2.0 in a way that the
> whole selection object is being destroyed if the tree is empty or something.
That's strange, because the link above is the only call to SetSelection...

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2 [Checkin: comment 11]

I'd rather not like to change the API wrt. FF, so I kept the private member variable and method names, but I hope you like the implementation better this way.

Comment 10

[[e-mail]]

Comment on attachment 499769 [details] [diff] [review]
patch v2 [Checkin: comment 11]

>+    this._inBatchMode = this.selection.selectEventsSuppressed = aBatching;
[I wonder whether the assignment order makes any difference. Probably not.]

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 499769 [details] [diff] [review]
patch v2 [Checkin: comment 11]

http://hg.mozilla.org/comm-central/rev/518a897f2fc9

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Looks like we need to port bug 620198, too. File bug 627408 for that.