208093 - Tree body frame could cache row count

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[[e-mail]]

The tree body frame recently acquired a new member variable mCountBeforeUpdate.
This variable (suitably renamed) could be used to cache the row count. It would
only be updated by RowCountChanged and EndUpdateBatch. This would save 13 calls
to GetRowCount.

Comment 1

[Jan Varga [:janv]]

Yeah, I agree
Neil, do you want to take it ?

Comment 2

[[e-mail]]

Taking.

Comment 3

[[e-mail]]

Attachment: Proposed patch

Tested with a content view, an rdfliner and a custom view.

Comment 4

[Jan Varga [:janv]]

Comment on attachment 124911 [details] [diff] [review]
Proposed patch

Comments on the patch:
1. Feel free to change mUpdateBatchNest from PRInt16 to PRInt32. Originally, I
wanted to couple it with other booleans, but it seems it's not worth trying.

2. I've noticed that you included some other changes in your patch, in
EnsureView() and in some of the scrolling methods. Could you extract them and
include them in a new bug e.g. "Tree enhancements" ?
I'll review those later

Other than that looks good.

Comment 5

[[e-mail]]

Attachment: Bits chopped out

I chopped out my patches to two other bugs (not enhancements) as requested.
I put in an extra assert to catch some other abusers of trees.

Comment 6

[Jan Varga [:janv]]

Comment on attachment 126029 [details] [diff] [review]
Bits chopped out

>@@ -738,6 +733,7 @@
>     }
> 
>     // The scrollbar will need to be updated.
>+    mView->GetRowCount(&mRowCount);
>     InvalidateScrollbar();

Could this be moved above the comment with an extra blank line?

>@@ -892,6 +888,11 @@
>   if (!mRect.IsEmpty()) {
>     nsLeafBoxFrame::Invalidate(mPresContext, mRect, PR_FALSE);
>   }
>+  if (mView) {
>+    PRInt32 rowCount = mRowCount;
>+    mView->GetRowCount(&mRowCount);
>+    NS_ASSERTION(mRowCount == rowCount, "row count changed unexpectedly");
>+  }

I think this can be ifdef'ed:
+
+#ifdef DEBUG
+  if (mView) {
+    PRInt32 rowCount = mRowCount;
+    mView->GetRowCount(&mRowCount);
+    NS_ASSERTION(mRowCount == rowCount, "row count changed unexpectedly");
+  }
+#endif
+

Comment 7

[[e-mail]]

Comment on attachment 126029 [details] [diff] [review]
Bits chopped out

I did think about #ifdef but I was worried that this would result in different
behaviour in debug and release builds.

Comment 8

[Jan Varga [:janv]]

ah, it's in Invalidate()
Actually, why do we need to refresh mRowCount there ?
If you want to just catch bad callers, add |#ifdef DEBUG| and compare it without
refreshing mRowCount. Otherwise I think some code could depend on this in future.

+
+#ifdef DEBUG
+  if (mView) {
+    PRInt32 rowCount;
+    mView->GetRowCount(&rowCount);
+    NS_ASSERTION(mRowCount == rowCount, "row count changed unexpectedly");
+  }
+#endif
+

Comment 9

[[e-mail]]

Jan, nsXULTreeBuilder.cpp abuses the row count :-(
I'm not sure if I'll be able to figure out why...

Comment 10

[[e-mail]]

My bad, EndUpdateBatch calls Invalidate() :-[

Comment 11

[Jan Varga [:janv]]

If this is the case then we better move it to Paint() method.

Comment 12

[[e-mail]]

Jan: here's a good one: when you remove a <tree>'s immediate <treechildren> node
then the tree content view simply calls Invalidate() - it does NOT call
RowCountChanged()! I'm surprised it works at all.

Comment 13

[Jan Varga [:janv]]

As I said, just move it to the Paint() method for now (maybe w/o the assestion).
I'm ok with that.

Comment 14

[[e-mail]]

I'm not quite sure what you want:
1) Leave some or all of the GetRowCount()s in for now
2) As 1) but in debug builds only; release builds always use the cached count
3) As 1) but assert if the count changed
4) Both 2) and 3)

Comment 15

[[e-mail]]

OK, so it doesn't assert in Paint(), so I guess I have no issues with the
content or builder views. I'll leave the #ifdef'd assert in Paint() though, just
in case something else crops up.

Comment 16

[Jan Varga [:janv]]

sounds good

Comment 17

[[e-mail]]

Attachment: Moved and #ifdef'd the asserts

Comment 18

[jag (Peter Annema)]

Comment on attachment 126459 [details] [diff] [review]
Moved and #ifdef'd the asserts

+#ifdef DEBUG
+  PRInt32 rowCount = mRowCount;
+  mView->GetRowCount(&mRowCount);
+  NS_ASSERTION(rowCount == mRowCount, "row count did not change by the amount
suggested, check caller");
+#endif

So if anything's wrong, DEBUG will behave differently than non-DEBUG because it
just updated mRowCount. Of course, it will assert about it, so it may be okay.
I think I'd still prefer mView->GetRowCount(&rowCount) though (both cases).

sr=jag with that changed

Comment 19

[[e-mail]]

Now I remember why I update the row count after asserting - if you don't then
every time you paint you'll assert! This isn't a problem in RowCountChanged
though, that can go either way as you prefer.

Comment 20

[[e-mail]]

I updated the RowCountChanged assert for bitrot and jag's comment.

Comment 21

[[e-mail]]

Didn't quite fix the bitrot properly :-( New patch coming up.

Comment 22

[[e-mail]]

Attachment: Corrected merge

Comment 23

[[e-mail]]

Turns out that adam moved some code as part of bustage so I moved it back :-P

Comment 24

[jag (Peter Annema)]

Comment on attachment 126818 [details] [diff] [review]
Corrected merge

Erh...

+  PRInt32 rowCount = mRowCount;
+  mView->GetRowCount(&rowCount);

Didn't you want to |GetRowCount(&mRowCount)| there?

Nit: in the other check you assert on mRowCount == rowCount, here it's the
other way around.

Comment 25

[[e-mail]]

If you do change the row count when you assert on change, then if someone
precomputes the final row count but calls RowCountChanged multiple times then
the last change would fire an assertion, despite the row count being correct.
If you don't change the row count when you assert on paint, then you'll keep
asserting on paint. Actually now that DOM Inspector is fixed I should be able to
get away with changing this as it shouldn't annoy anybody.