347019 - Don't throw exception when no accessible for node in nsIAccessibleRetrieval::getAccessibleFor() and friends

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

It is very normal for a DOM node not to have an accessible associated with it.

Exceptions should be reserved for unexpected occurances or errors of some kind.

Therefore we should simply return null for the accessible, but NS_OK since there is no exception. We'll need to check all the callers to make sure they check the returned accessible and not the nsresult.

Comment 1

[Aaron Leventhal]

Similar to bug 337656.

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[Aaron Leventhal]

Comment on attachment 253611 [details] [diff] [review]
patch

David, enjoy your first code review :)

Comment 4

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

Comment on attachment 253611 [details] [diff] [review]
patch

Looks good. One minor nit is that I see you are following the no {}'s for single line if's. I think for the accessible module we are including them.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (From update of attachment 253611 [details] [diff] [review])
> Looks good. One minor nit is that I see you are following the no {}'s for
> single line if's. I think for the accessible module we are including them.
> 

Though IIRC mozilla code styling guide doesn't define unuque way how to style single lines 'if'/'else' but for example, in toolkit, mano requires to remove them if they are. I don't like to have two or more code styling in mozilla code since it's awful to remember where should I use one style where should I use another one. Aaron, what's your point about code styling?

Comment 6

[Aaron Leventhal]

I won't require it either way at this point. We have both in the code and I think this is taking more time to talk about than it's worth. There are reasons for {} on single lines, but honestly I'm not interested in spending more time on the issue. Let's just check in the code. Try to be similar code in the file/method you are changing and we'll be fine.

Comment 7

[Aaron Leventhal]

Surkov, didn't we need to return NS_OK here?
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessibilityService.cpp#1131

1130   if (!aAccessibleIn) {
1131     return NS_ERROR_FAILURE; // No accessible to init
1132   }

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Surkov, didn't we need to return NS_OK here?
> http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessibilityService.cpp#1131
> 
> 1130   if (!aAccessibleIn) {
> 1131     return NS_ERROR_FAILURE; // No accessible to init
> 1132   }
> 

let's move discussion to bug 385731