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.

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

Taking.

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

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.

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 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 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.

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 +

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

My bad, EndUpdateBatch calls Invalidate() :-[

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

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.

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

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)

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.

sounds good

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

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.

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

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

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

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.

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.