416872 - Simplify GetState() impl's for defunct objects

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Right now each GetState() impl that calls up to the parent class must check mDOMNode and return early.

It would be simpler if we could just return early if GetState() != NS_OK.

That would be accomplished by returning NS_OK_DEFUNCT_OBJECT or something like that.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[[e-mail]]

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

>+    nsresult __rv = res; /* Don't evaluate |res| more than once */        \
>+    if (NS_FAILED(__rv)) {                                                \
>+      NS_ENSURE_SUCCESS_BODY(res, ret)                                    \
Eww, NS_ENSURE_SUCCESS_BODY has an external dependency on __rv :-(

Comment 3

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

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

Hi Surkov, Neil has already sr+'ed so these questions/comments are really just for my own education:

>--- a/accessible/src/base/nsAccessNode.h
>+++ b/accessible/src/base/nsAccessNode.h
>@@ -68,16 +68,30 @@ class nsApplicationAccessibleWrap;
> class nsApplicationAccessibleWrap;
> class nsIDocShellTreeItem;
> 
> #define ACCESSIBLE_BUNDLE_URL "chrome://global-platform/locale/accessible.properties"
> #define PLATFORM_KEYS_BUNDLE_URL "chrome://global-platform/locale/platformKeys.properties"
> 
> typedef nsInterfaceHashtable<nsVoidPtrHashKey, nsIAccessNode>
>         nsAccessNodeHashtable;
>+
>+#define NS_OK_DEFUNCT_OBJECT \
>+NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_GENERAL, 0x22)

Where did 0x22 come from?

>+
>+#define NS_ENSURE_A11Y_SUCCESS(res, ret)    

I wondered if other modules might have a similar requirement for dealing with defunct objects, and if this should be a more general shared macro... but I guess leaving it in nsAccessNode.h allows us to easily add additional future checks that may be more a11y specific.


>+  PR_BEGIN_MACRO                                                          \
>+    nsresult __rv = res; /* Don't evaluate |res| more than once */        \
>+    if (NS_FAILED(__rv)) {                                                \
>+      NS_ENSURE_SUCCESS_BODY(res, ret)                                    \
>+      return ret;                                                         \
>+    }                                                                     \
>+    if (__rv == NS_OK_DEFUNCT_OBJECT)                                     \
>+      return ret;                                                         \
>+  PR_END_MACRO

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Where did 0x22 come from?

0x21 is used in nsAccessible.h. I don't remember why it's 0x21. I guess free numbers are started from this point.

> 
> >+
> >+#define NS_ENSURE_A11Y_SUCCESS(res, ret)    
> 
> I wondered if other modules might have a similar requirement for dealing with
> defunct objects, and if this should be a more general shared macro... but I
> guess leaving it in nsAccessNode.h allows us to easily add additional future
> checks that may be more a11y specific.

Sorry I'm not sure I got you. This macros can be used everywhere in a11y module (because nsAccessNode.h is included almost everywhere). I think it may be resonable to use it not for getState() only. But here it's important because this method doesn't fail if the object is defunct.

Comment 5

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

(In reply to comment #4)
> (In reply to comment #3)
> > >+
> > >+#define NS_ENSURE_A11Y_SUCCESS(res, ret)    
> > 
> > I wondered if other modules might have a similar requirement for dealing with
> > defunct objects, and if this should be a more general shared macro... but I
> > guess leaving it in nsAccessNode.h allows us to easily add additional future
> > checks that may be more a11y specific.
> 
> Sorry I'm not sure I got you. This macros can be used everywhere in a11y module
> (because nsAccessNode.h is included almost everywhere). I think it may be
> resonable to use it not for getState() only. But here it's important because
> this method doesn't fail if the object is defunct.

I was thinking about outside the a11y module actually, but it was really just "thinking out loud". I'm not positive there is a need for a general macro for protecting against defunt-ness.

In other words, don't worry about it :)

Comment 6

[alexander :surkov (:asurkov)]

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

just code clean up, no risk

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

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

We're closing down for beta 2, we'll get this after we branch.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 9

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/8dc5392b0c1d