624977 - Optimize nsXulTreeAccessible selectedItems()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b9pre) Gecko/20110111 Firefox/4.0b9pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b9pre) Gecko/20110111 Firefox/4.0b9pre

selectedITems() in nsXulTreeAccessible was iterating over all the rows of a xul tree when finding the list of selected items instead we use getRangeCount() and getRangeAt() from xul tree selection

Reproducible: Couldn't Reproduce

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: a patch implementing this

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 503082 [details] [diff] [review]
a patch implementing this

nsITreeSelection::GetRangeAt is not optimal for us - http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeSelection.cpp#584, but that is much better than we have now - traverse through all tree items and look each item through all ranges. I'd suggest to file followup bug to change nsITreeSelection what can be done after Firefox 4.

>+  PRInt32 selectionGroups, curGroup;
>+  selection->GetRangeCount(&selectionGroups);
>+  for (curGroup = 0; curGroup < selectionGroups; curGroup++) {
>+    PRInt32 curRow, firstRow, lastRow;
>+	  selection->GetRangeAt(curGroup, &firstRow, &lastRow);
>+    for (curRow = firstRow; curRow <= lastRow; curRow++) {
>+      nsIAccessible* item = GetTreeItemAccessible(curRow);
>       if (item)
>         selectedItems->AppendElement(item, PR_FALSE);
>     }
>   }

I'd define variables where they are used, initialize them and change their names, also please use spaces (not tabs) like

>+  PRInt32 rangeCount = 0;
>+  selection->GetRangeCount(&rangeCount);
>+  for (PRint32 rangeIdx = 0; rangeIdx < rangeCount; rangeIdx++) {
>+    PRInt32 firstRowIdx = 0, lastRowIdx = -1;
>     selection->GetRangeAt(rangeIdx, &firstRowIdx, &lastRowIdx);
>+    for (PRInt32 rowIdx = firstRowIdx; rowIdx <= lastRowIdx; rowIdx++) {
>+      nsIAccessible* item = GetTreeItemAccessible(rowIdx);
>       if (item)
>         selectedItems->AppendElement(item, PR_FALSE);
>     }
>   }

please update the patch and rerequest the review

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: updated patch basedon Surkov's comments

updated patch

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 503101 [details] [diff] [review]
updated patch basedon Surkov's comments

r=me, requesting an approval, that should be good perf improvement

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 503101 [details] [diff] [review]
updated patch basedon Surkov's comments

a=me as long as we get a clean try server run, and please confirm that we have sufficient test coverage. (Nice one Trevor!)

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Trevor to confirm test coverage you can purposely introduce various bugs and run our test suite to check for failures. If nothing fails we'll need to write tests.

Comment 7

[Andrew Drake [:adrake]]

Nice and green on tryserver.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Nice and green on tryserver.

Unfortunately it's not covered by mochitest. 

I'll add some mochitests since it sounds like mochitest suite fails when Orca is running and as consequence Trevor can't do that.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: mochitest

Comment 10

[alexander :surkov (:asurkov)]

try server build containing mochitest and patch - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-fe2d80aed386

Comment 11

[Marco Zehe (:MarcoZ)]

Comment on attachment 504370 [details] [diff] [review]
mochitest

r=me, nicely done, thanks!

Comment 12

[alexander :surkov (:asurkov)]

landed on 2.0
http://hg.mozilla.org/mozilla-central/rev/e227ac817df4 (mochitest)
http://hg.mozilla.org/mozilla-central/rev/66addc5c30ca (patch)