Comment 1 alexander :surkov 2011-08-03 09:55:57 PDT

Created attachment 550414 [details] [diff] [review]
patch

Comment 2 Trevor Saunders (:tbsaunde) 2011-08-03 13:07:24 PDT

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

>    */
>-  inline bool HasAccessible(nsINode* aNode)
>+  inline bool HasAccessible(nsINode* aNode) const
>   {
>     return GetAccessible(aNode);
>   }

nit, don't we usually use { return blah; } in cases like this?

> 
>   /**
>+   * Return true if the given accessible is in document.
>+   */
>+  inline bool IsInDocument(nsAccessible* aAccessible) const
>+  {
>+    nsAccessible* acc = aAccessible;

why do you need this local var? why not just mutate aAcc? 

>+    while (acc && !acc->IsPrimaryForNode())
>+      acc = acc->Parent();
>+
>+    return acc ? !!mNodeToAccessibleMap.Get(acc->GetNode()) : false;
>+  }

why do you need !! does some compiler complain?

Comment 3 alexander :surkov 2011-08-03 13:12:10 PDT

(In reply to comment #2)

> nit, don't we usually use { return blah; } in cases like this?

ok, fine with me

> >+  {
> >+    nsAccessible* acc = aAccessible;
> 
> why do you need this local var? why not just mutate aAcc? 

harder to debug, following the rule "keep in arguments unchanged"

> >+    return acc ? !!mNodeToAccessibleMap.Get(acc->GetNode()) : false;
> >+  }
> 
> why do you need !! does some compiler complain?

I didn't try, just this is usual way to convert pointer to boolean

Comment 4 alexander :surkov 2011-08-04 02:58:03 PDT

landed on inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/dd38dafe931b

Comment 5 alexander :surkov 2011-08-05 08:48:15 PDT

landed http://hg.mozilla.org/mozilla-central/rev/dd38dafe931b