211695 - Bugs in nsCertTree.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[[e-mail]]

1. Cert tree doesn't use the right update notifications when changing the list
2. Cert tree doesn't locate the parent index correctly
3. Cert tree doesn't draw lines correctly (HasNextSibling unimplemented)
4. Cert tree shouldn't compare bools to bool constants

Comment 1

[[e-mail]]

Attachment: Proposed patch

Comment 2

[Kai Engert [:KaiE:]]

Before I give review+ or review-:

1)
I can not comment whether changing RowCountChanged+Invalidate to
BeginUpdateBatch+EndUpdateBatch is a safe change.
I am fine with the change if you are sure it is correct.

2)
You say "Cert tree doesn't locate the parent index correctly".
Which bug do you see that will be fixed by your change?
In general, I agree your new code is reasonable.

3)
/** 
 * HasNextSibling is used to determine if the row at rowIndex has a nextSibling
 * that occurs *after* the index specified by afterIndex.  Code that is forced
 * to march down the view looking at levels can optimize the march by starting
 * at afterIndex+1.
 */

Are you using afterIndex correctly?
I think the comment says, "you may use afterIndex to optimize your code".
I think your code will never look at rows greater than afterIndex,
it will always decide there is "no sibling afterIndex"?

Comment 3

[[e-mail]]

Bah, moz crashed while typing in a long explaination. Trying again...

The old code is wrong, because it never deletes rows from the tree.
So, I've come up with some options:
[1]
if (mTree) {
  PRInt32 oldRowCount = mRowCount;
  mRowCount = 0;
  mTree->RowCountChanged(0, -oldRowCount);
}
mRowCount = count + mNumOrgs;
if (mTree)
  mTree->RowCountChanged(0, mRowCount);
[2]
if (mTree) {
  mTree->BeginUpdateBatch();
  mTree->RowCountChanged(0, -mRowCount);
}
mRowCount = count + mNumOrgs;
if (mTree)
  //mTree->RowCountChanged(0, mRowCount); has no effect here
  mTree->EndUpdateBatch();
[3]
PRInt32 oldRowCount = mRowCount;
mRowCount = count + mNumOrgs;
if (mTree) {
  mTree->RowCountChanged(PR_MIN(mRowCount, oldRowCount), mRowCount - oldRowCount);
  mTree->Invalidate();
}
[4]
if (mTree)
  mTree->BeginUpdateBatch();
mRowCount = count + mNumOrgs;
if (mTree)
  mTree->EndUpdateBatch();
[1] is same as [2] except [1] always scrolls the list to the top
[2] .. [4] are very similar, except [4] never clears the selection, [3] clears
it sometimes and [2] always clears it.

To reproduce the parent index bug, go to the authorities tree, note the position
of the headings, note how they move when you collapse the first heading, put the
cursor on a child and press left and see how it moves to the where the heading
used to be (of course it works when all the headings are expanded).

HasNextSibling is actually used to determine whether to paint a reverse L or a
sideways T. rowIndex should be the index of the appropriate ancestor (in this
case always the item itself, thus the confusion). afterIndex should be the index
of the child being painted. If I swap the two variables around, that should fix
things, right?

Comment 4

[Kai Engert [:KaiE:]]

To summarize from our IRC discussions, and as a follow up to my three points
mentioned in comment 2:

1) I don't understand the tree code well enough to judge whether your
beginupdate+endupdate change is correct. If you are uncertain, please get
another review.

2) same as 1)

3) You said, you can assume that afterIndex is always > rowIndex. From reading
the comments this is not clear to me. If you are certain this is always true,
please add a comment and an assertion to HasNextSibling, this will make future
reading of the code easier.

r=kaie on the changes in general, assuming you get a tree code expert to review,
too.

Comment 5

[[e-mail]]

Attachment: Revised patch

Comment 6

[[e-mail]]

Comment on attachment 127354 [details] [diff] [review]
Revised patch

Looking for some tree love.

Comment 7

[Alec Flett]

Comment on attachment 127354 [details] [diff] [review]
Revised patch

sr=alecf - seems ok to me but obviously jan's r= is necessary for the actual
tree logic.

Comment 8

[Jan Varga [:janv]]

Comment on attachment 127354 [details] [diff] [review]
Revised patch

looks good

Comment 9

[[e-mail]]

Comment on attachment 127354 [details] [diff] [review]
Revised patch

Fixes some cosmetic issues with the certificates tree. I can't actually find
someone who uses it to see what sort of reward it might be, though :-/

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 127354 [details] [diff] [review]
Revised patch

a=asa (on behalf of drivers) for checkin to Mozilla 1.5beta.

Comment 11

[[e-mail]]

Fix checked in.