Closed
Bug 672515
Opened 13 years ago
Closed 13 years ago
remove nsIAccessible getAccessibleToRight/Left/Above/Below
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: surkov, Assigned: marco)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file, 5 obsolete files)
6.94 KB,
patch
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•13 years ago
|
||
I think there isn't need to change the uuid, as it's already changed for bug 675870. Am I right?
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #556422 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 2•13 years ago
|
||
You should change the uuid whenever the vtable of an IDL interface implementer would change (such as adding/removing/modifiying functions in the interface).
Assignee | ||
Comment 3•13 years ago
|
||
Changed uuid.
Attachment #556422 -
Attachment is obsolete: true
Attachment #556428 -
Flags: review?(surkov.alexander)
Attachment #556422 -
Flags: review?(surkov.alexander)
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #556428 -
Attachment is obsolete: true
Attachment #556434 -
Flags: review?(trev.saunders)
Attachment #556428 -
Flags: review?(surkov.alexander)
Comment 6•13 years ago
|
||
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)
Attachment #556434 -
Flags: review?(trev.saunders) → review?(surkov.alexander)
Reporter | ||
Comment 7•13 years ago
|
||
(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.
Reporter | ||
Updated•13 years ago
|
Attachment #556434 -
Flags: review?(surkov.alexander) → review?(trev.saunders)
Assignee | ||
Comment 8•13 years ago
|
||
Made the needed changes, I couldn't try to compile because of a problem, but it should work.
Attachment #556434 -
Attachment is obsolete: true
Attachment #557724 -
Flags: review?(trev.saunders)
Attachment #556434 -
Flags: review?(trev.saunders)
Comment 9•13 years ago
|
||
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
Attachment #557724 -
Flags: review?(trev.saunders) → review+
Reporter | ||
Comment 10•13 years ago
|
||
(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.
Assignee | ||
Comment 11•13 years ago
|
||
Fixed the IsDefunct() call.
Attachment #557724 -
Attachment is obsolete: true
Attachment #557792 -
Flags: review?(trev.saunders)
Comment 12•13 years ago
|
||
Comment on attachment 557792 [details] [diff] [review] Patch v5 tHANKS FOR CLEANING ALL THIS UP!
Attachment #557792 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 13•13 years ago
|
||
In my queue :-) https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=feaf94ae787f
Comment 14•13 years ago
|
||
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' }
Reporter | ||
Comment 15•13 years ago
|
||
(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()
Assignee | ||
Comment 16•13 years ago
|
||
Corrected PreviousSibling in PrevSibling.
Attachment #557792 -
Attachment is obsolete: true
Comment 17•13 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=b696a6f86a9b :-)
Comment 18•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/b38795f5d13b
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/b38795f5d13b
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•