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

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: aaronlev, Assigned: surkov)

Tracking

({access})

Trunk
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Similar to bug 337656.
Assignee: aaronleventhal → surkov.alexander
(Assignee)

Comment 2

12 years ago
Created attachment 253611 [details] [diff] [review]
patch
Attachment #253611 - Flags: review?(aaronleventhal)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 3

12 years ago
Comment on attachment 253611 [details] [diff] [review]
patch

David, enjoy your first code review :)
Attachment #253611 - Flags: review?(aaronleventhal) → review?(david.bolter)
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.
Attachment #253611 - Flags: review?(david.bolter) → review+
(Assignee)

Comment 5

12 years ago
(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?
(Reporter)

Comment 6

12 years ago
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.
(Reporter)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 7

12 years ago
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   }
(Assignee)

Comment 8

12 years ago
(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
You need to log in before you can comment on or make changes to this bug.