Last Comment Bug 672515 - remove nsIAccessible getAccessibleToRight/Left/Above/Below
: remove nsIAccessible getAccessibleToRight/Left/Above/Below
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Marco Castelluccio [:marco]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: cleanxpcoma11y
  Show dependency treegraph
 
Reported: 2011-07-19 08:21 PDT by alexander :surkov
Modified: 2011-09-06 04:03 PDT (History)
4 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.04 KB, patch)
2011-08-28 13:57 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Patch v2 (3.87 KB, patch)
2011-08-28 14:35 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Patch v3 (5.75 KB, patch)
2011-08-28 15:40 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
Patch v4 (6.93 KB, patch)
2011-09-01 17:52 PDT, Marco Castelluccio [:marco]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch v5 (6.94 KB, patch)
2011-09-02 03:22 PDT, Marco Castelluccio [:marco]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch v6 (6.94 KB, patch)
2011-09-05 03:30 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review

Description alexander :surkov 2011-07-19 08:21:34 PDT
They weren't ever implemented. MSAA part (NAVDIR_RIHGT and etc) relies on them but I didn't ever hear anybody needs 'em.

Jamie, do you know anything?
Comment 1 Marco Castelluccio [:marco] 2011-08-28 13:57:09 PDT
Created attachment 556422 [details] [diff] [review]
Patch

I think there isn't need to change the uuid, as it's already changed for bug 675870. Am I right?
Comment 2 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-08-28 14:12:45 PDT
You should change the uuid whenever the vtable of an IDL interface implementer would change (such as adding/removing/modifiying functions in the interface).
Comment 3 Marco Castelluccio [:marco] 2011-08-28 14:35:34 PDT
Created attachment 556428 [details] [diff] [review]
Patch v2

Changed uuid.
Comment 4 Trevor Saunders (:tbsaunde) 2011-08-28 15:32:13 PDT
Comment on attachment 556428 [details] [diff] [review]
Patch v2

>-    case NAVDIR_DOWN:
>-      xpAccessibleStart->GetAccessibleBelow(getter_AddRefs(xpAccessibleResult));
>-      break;

you still need to handle these cases by returning E_NOTIMPl,  I'd also like to see this switch statement have a default case which should probably return E_INVALIDARG.

also, you didn't remove the implementations of the xpcom methods.
Comment 5 Marco Castelluccio [:marco] 2011-08-28 15:40:49 PDT
Created attachment 556434 [details] [diff] [review]
Patch v3
Comment 6 Trevor Saunders (:tbsaunde) 2011-08-28 17:33:17 PDT
Comment on attachment 556434 [details] [diff] [review]
Patch v3


Thanks for working on this, this looks correct, but fixing this bug allows some more cleanup.

>   nsCOMPtr<nsIAccessible> xpAccessibleResult;

I don't think we need to use that anymore, you should be able to have nsAccessible* instead.  (please rename xpAccessibleResult while your here).  Doing this will also let you use nsAccessible::Next/Prev/First/Last Child() instead of the xpcom method.

>     case NAVDIR_DOWN:
>-      xpAccessibleStart->GetAccessibleBelow(getter_AddRefs(xpAccessibleResult));
>-      break;
>+      return E_NOTIMPL;

I'd suggest collecting these four cases so you can use falling through cases like this

case DOWN:
case LEFT:
  return E_NOTIMPL;

(I'm not a peer so passing review request to Alex)
Comment 7 alexander :surkov 2011-08-28 20:27:10 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> >   nsCOMPtr<nsIAccessible> xpAccessibleResult;
> 
> I don't think we need to use that anymore, you should be able to have
> nsAccessible* instead.  (please rename xpAccessibleResult while your here). 
> Doing this will also let you use nsAccessible::Next/Prev/First/Last Child()
> instead of the xpcom method.

true, code snippet:

nsAccessible* navAccessible = nsnull;
switch (navDir) {
  case NAVDIR_FIRSTCHILD:
    navAccessible = xpAccessibleStart->FirstChild();
    break;
}

if you rename 'xpAccessibleResult' to 'navAccessible' then it makes sense to rename 'xpAccessibleStart' to 'accessible' as well.

> I'd suggest collecting these four cases so you can use falling through cases
> like this
> 
> case DOWN:
> case LEFT:
>   return E_NOTIMPL;

correct, oleacc.h defines these variables the following way:

#define NAVDIR_DOWN 2
#define NAVDIR_FIRSTCHILD 7
#define NAVDIR_LASTCHILD 8
#define NAVDIR_LEFT 3
#define NAVDIR_NEXT 5
#define NAVDIR_PREVIOUS 6
#define NAVDIR_RIGHT 4
#define NAVDIR_UP 1

so you should keep them together to make sure switch is optimized.

> (I'm not a peer so passing review request to Alex)

It's totally ok if you do review for this patch. Thank you for doing this, Trevor.
Comment 8 Marco Castelluccio [:marco] 2011-09-01 17:52:09 PDT
Created attachment 557724 [details] [diff] [review]
Patch v4

Made the needed changes, I couldn't try to compile because of a problem, but it should work.
Comment 9 Trevor Saunders (:tbsaunde) 2011-09-01 20:51:56 PDT
Comment on attachment 557724 [details] [diff] [review]
Patch v4

nit, 
>+  nsAccessible *accessible = GetXPAccessibleFor(varStart);

nit, I'd just assume you didn't change that, but I suspect Alex will kill me or something if I don't tell you to do nsAccessible *accessible -> nsAccessible* accessible ;)

>+  if (!accessible || IsDefunct())

oops, looks like I didn't fix bug 678189 quiet right, that should be accessible->IsDefunct() not  just IsDefunct()

>       break;
>+
>+    default:

nit, I'm not sure I'd put a blank line there, but whatever.

>   if (xpRelation) {
>     Relation rel = RelationByType(xpRelation);
>-    xpAccessibleResult = rel.Next();
>+    navAccessible = rel.Next();

we don't really need that local var for anything do we Alex?   I think we could just do RelationByType(type).Next() here.

r=me with the IsDefunct() thing fixed
Comment 10 alexander :surkov 2011-09-01 21:20:29 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> Comment on attachment 557724 [details] [diff] [review]
> Patch v4
> 
> nit, 
> >+  nsAccessible *accessible = GetXPAccessibleFor(varStart);
> 
> nit, I'd just assume you didn't change that, but I suspect Alex will kill me
> or something if I don't tell you to do nsAccessible *accessible ->
> nsAccessible* accessible ;)

after you promised that they will be fixed automatically during architecture reorgs I'm resistant to keep nits like this :)

> >   if (xpRelation) {
> >     Relation rel = RelationByType(xpRelation);
> >-    xpAccessibleResult = rel.Next();
> >+    navAccessible = rel.Next();
> 
> we don't really need that local var for anything do we Alex?   I think we
> could just do RelationByType(type).Next() here.

maybe we need it because I assume casting magic wouldn't happen and non const Next() is called on const object.
Comment 11 Marco Castelluccio [:marco] 2011-09-02 03:22:41 PDT
Created attachment 557792 [details] [diff] [review]
Patch v5

Fixed the IsDefunct() call.
Comment 12 Trevor Saunders (:tbsaunde) 2011-09-03 16:49:24 PDT
Comment on attachment 557792 [details] [diff] [review]
Patch v5

tHANKS FOR CLEANING ALL THIS UP!
Comment 13 Ed Morley [:emorley] 2011-09-04 17:37:00 PDT
In my queue :-)
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=feaf94ae787f
Comment 14 Ed Morley [:emorley] 2011-09-04 18:53:54 PDT
Failed try:
https://tbpl.mozilla.org/php/getParsedLog.php?id=6275374

{
e:/builds/moz2_slave/try-w32/build/accessible/src/msaa/nsAccessibleWrap.cpp(835) : error C2039: 'PreviousSibling' : is not a member of 'nsAccessible'
        e:\builds\moz2_slave\try-w32\build\accessible\src\base\nsAccessible.h(98) : see declaration of 'nsAccessible'
}
Comment 15 alexander :surkov 2011-09-04 21:26:47 PDT
(In reply to Ed Morley [:edmorley] from comment #14)

> e:/builds/moz2_slave/try-w32/build/accessible/src/msaa/nsAccessibleWrap.
> cpp(835) : error C2039: 'PreviousSibling' : is not a member of 'nsAccessible'

should be PrevSibling()
Comment 16 Marco Castelluccio [:marco] 2011-09-05 03:30:22 PDT
Created attachment 558260 [details] [diff] [review]
Patch v6

Corrected PreviousSibling in PrevSibling.
Comment 17 Ed Morley [:emorley] 2011-09-05 03:49:46 PDT
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=b696a6f86a9b

:-)
Comment 19 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-06 04:03:17 PDT
http://hg.mozilla.org/mozilla-central/rev/b38795f5d13b

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