Last Comment Bug 760878 - decomtaminate Get Row / Column Description() on accessible tables
: decomtaminate Get Row / Column Description() on accessible tables
Status: RESOLVED FIXED
[good first bug][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]
:
: alexander :surkov
Mentors:
Depends on: 760880
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-06-02 18:51 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-06-16 06:53 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (11.97 KB, patch)
2012-06-13 16:48 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-06-02 18:51:18 PDT
follow same approach as bug 757504
Comment 1 Mark Capella [:capella] 2012-06-13 16:48:07 PDT
Created attachment 632961 [details] [diff] [review]
Patch (v1)
Comment 2 Trevor Saunders (:tbsaunde) 2012-06-14 08:45:34 PDT
Comment on attachment 632961 [details] [diff] [review]
Patch (v1)

>+xpcAccessibleTable::GetColumnDescription(PRInt32 aColIdx, nsAString& aDescription)

nit, over 80 chars

>+XULTreeGridAccessible::ColDescription(PRUint32 aColIdx, nsString& aDescription)
> {
>   aDescription.Truncate();
> 
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>   nsCOMPtr<nsIAccessible> treeColumns;
>   Accessible::GetFirstChild(getter_AddRefs(treeColumns));
>   if (treeColumns) {
>     nsCOMPtr<nsIAccessible> treeColumnItem;
>-    treeColumns->GetChildAt(aColumnIndex, getter_AddRefs(treeColumnItem));
>+    treeColumns->GetChildAt(aColIdx, getter_AddRefs(treeColumnItem));

mind filing a bug to clean up this silly com stuff?

>   var columnDescription;
>   works = true;
>   try{
>     columnDescription = accTable.getColumnDescription(1);
>   }
>   catch (e) {
>     works = false;
>   }
>-  todo(works, "columnDescription should not throw");
>+  is(works, true, "columnDescription should not throw");

well, instead of  checking works == true you could get rid of the try / catch.  Which would just leave you with the silly test var rowDescription = accTable.getRowDescription(1);
a todo for actually implementing this stuff doesn't seem really useful.

> 
>   var rowDescription;
>   works = true;
>   try {
>     rowDescription = accTable.getRowDescription(1);
>   }
>   catch (e) {
>     works = false;
>   }
>-  todo(works, "rowDescription should not throw");
>+  is(works, true, "rowDescription should not throw");

same

note this will change what we return for IAccessibleTable from e_NOTIMPL to S_FALSe, but that seems fine.
Comment 3 Mark Capella [:capella] 2012-06-14 19:19:54 PDT
https://tbpl.mozilla.org/?tree=Try&rev=77f704ba95f8
Comment 4 Mark Capella [:capella] 2012-06-15 01:42:27 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb67e4c9f7df
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-06-16 06:53:27 PDT
https://hg.mozilla.org/mozilla-central/rev/eb67e4c9f7df

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