Last Comment Bug 760881 - decomtaminate Select Row / Column() on accessible tables
: decomtaminate Select Row / Column() on accessible tables
Status: RESOLVED FIXED
[good first bu][mentor=trev.saunders@...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-06-02 18:57 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-06-12 03:07 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (13.56 KB, patch)
2012-06-07 19:40 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-06-02 18:57:16 PDT
similar approach to bug 757504
Comment 1 Mark Capella [:capella] 2012-06-07 19:40:57 PDT
Created attachment 631252 [details] [diff] [review]
Patch (v1)

Asking feedback from mentor ...
Comment 2 alexander :surkov 2012-06-07 19:44:50 PDT
Comment on attachment 631252 [details] [diff] [review]
Patch (v1)

one review is enough for this bug
Comment 3 Trevor Saunders (:tbsaunde) 2012-06-09 16:04:01 PDT
Comment on attachment 631252 [details] [diff] [review]
Patch (v1)

>+  for (PRInt32 rowIdx = 0; (row = rowIter.Next()); rowIdx++)
>+    SetARIASelected(row, rowIdx == aRowIdx);

I don't think that should fail under normal cases, so I'd rather not get rid of the warning all together.  I'm not sure what the best replacement is though, maybe NS_ASSERTION(NS_SUCCeeDED(rv), "blah shouldn't fail!"); ?


>-    nsresult rv = SetARIASelected(row, false);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    SetARIASelected(row, false);

same

>+  RemoveRowsOrColumnsFromSelection(aRowIdx,
>+                                   nsISelectionPrivate::TABLESELECTION_ROW,
>+                                   true);
> 
>-  nsresult rv =
>-    RemoveRowsOrColumnsFromSelection(aRow,
>-                                     nsISelectionPrivate::TABLESELECTION_ROW,
>-                                     true);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  return AddRowOrColumnToSelection(aRow,
>-                                   nsISelectionPrivate::TABLESELECTION_ROW);
>+  AddRowOrColumnToSelection(aRowIdx, nsISelectionPrivate::TABLESELECTION_ROW);

same for these functions.

>   nsCOMPtr<nsITreeSelection> selection;
>   mTreeView->GetSelection(getter_AddRefs(selection));
>-  NS_ENSURE_STATE(selection);

same here

>+  if (control) {

I don't think you need to add this check, it wasn't here before, and we really shouldn't create this type of accessible when the content is of the different type.

>+    nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
>+    control->GetItemAtIndex(aRowIdx, getter_AddRefs(item));
>+    control->SelectItem(item);

on the other hand we used to check item wasn't null but I'm not sure that's really necesary now since we assume the row index is valid.
Comment 4 Mark Capella [:capella] 2012-06-09 17:04:48 PDT
re: "I don't think that should fail under normal cases" ...

I patterned these SelectRow() / SelectCol() methods on the existing UnselectRow () / UnselectCol() methods, which don't have warnings. If you lean towards my keeping warnings in Select methods, should I retro-fit them back into the Unselect methods?

re: if (control) { ... I don't think you need to add this check ...

Again, this is how the Unselect() function currently is coded, so I sought uniformity.

If you are flexible with your thoughts here, I'd like to leave the patch as I now have it. If you strongly disagree, then I'd feel better at least making consistent changes for both the Select() and Unselect() methods.
Comment 5 Mark Capella [:capella] 2012-06-10 12:41:50 PDT
TRY push
https://tbpl.mozilla.org/?tree=Try&rev=049e05386514
Comment 6 Trevor Saunders (:tbsaunde) 2012-06-10 14:16:44 PDT
(In reply to Mark Capella [:capella] from comment #4)
> re: "I don't think that should fail under normal cases" ...
> 
> I patterned these SelectRow() / SelectCol() methods on the existing
> UnselectRow () / UnselectCol() methods, which don't have warnings. If you
> lean towards my keeping warnings in Select methods, should I retro-fit them
> back into the Unselect methods?

I'd probably prefer that happen some day, but fixing them doesn't seem terribly urgent.

> re: if (control) { ... I don't think you need to add this check ...
> 
> Again, this is how the Unselect() function currently is coded, so I sought
> uniformity.

consistantly wrong is worse than sometimes right and inconsnistant.

> If you are flexible with your thoughts here, I'd like to leave the patch as
> I now have it. If you strongly disagree, then I'd feel better at least
> making consistent changes for both the Select() and Unselect() methods.

why? what's wrong with just fixing the current issue the best way possiblew?  THere are certainly inconsistant things, but if you try and fix them all at once you'll never get it done.  If you really can't stand the thought of a little bit of inconsistancy I guess I don't mind fixing them now since its small, and sort of maybe a little related, but that seems like enough change I'd probably get it reviewed again, were as if you just change these methods I wouldn't bother.
Comment 7 Mark Capella [:capella] 2012-06-10 15:07:50 PDT
Yah ok, stuck in the mud again. Let me focus on something else for a bit.
Comment 8 Mark Capella [:capella] 2012-06-10 19:24:19 PDT
Ok, got the build problems w/ bug 763212 fixed, so I'm able to build / test again ... changed / tested just those methods, and pushing to TRY again to be safe

https://tbpl.mozilla.org/?tree=Try&rev=4f92a8fead85
Comment 9 Mark Capella [:capella] 2012-06-10 22:45:57 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4c34e1a4cbb5
Comment 10 Ed Morley [:emorley] 2012-06-11 01:47:21 PDT
Unfortunately this appears to have caused:

Regression :( SVG increase 26.9% on Linux x64 Mozilla-Inbound-Non-PGO
---------------------------------------------------------------------
    Previous: avg 3573.097 stddev 6.190 of 30 runs up to revision 61fd66629c4f
    New     : avg 4535.128 stddev 12.532 of 5 runs since revision 4c34e1a4cbb5
    Change  : +962.031 (26.9% / z=155.408)
    Graph   : http://mzl.la/N2bejZ 

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/QuabFfbyLAo
Comment 11 Ed Morley [:emorley] 2012-06-11 01:52:17 PDT
Sorry, believe that was actually caused by bug 539356 looking at the graphs for other platforms.
Comment 12 Graeme McCutcheon [:graememcc] 2012-06-12 03:07:31 PDT
https://hg.mozilla.org/mozilla-central/rev/4c34e1a4cbb5

Note You need to log in before you can comment on or make changes to this bug.