Closed Bug 542221 Opened 10 years ago Closed 10 years ago

move getAccessible methods from nsIAccessibilityService to internals

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: davidb)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file, 5 obsolete files)

nsIAccessibilityService::GetAccessible() and nsIAccessibleRetrieval::GetAccessibleInWeakShell() should be moved to nsAccessibilityService class to be used internally.

Iirc getAccessibleInWindow() isn't used so it can be removed

Internally we should deal with nsAccessibiltiyService class instead of nsIAccessibilityService.

GetAccessible could return not addrefed nsAccessible* pointer since it puts the accessible into cache. So that interface methods could look like

NS_IMETHODIMP
Method(nsIAccessible **aAccessible)
{
  NS_IF_ADDREF(*aAccessible = accesibilityServiceInstance->GetAccessible());
  return NS_OK;
}
Thanks for filing Alex; taking, as per IRC chat.
Assignee: nobody → bolterbugz
I'm unassigning (at least until the dependency has landed)
Assignee: bolterbugz → nobody
(In reply to comment #2)
> I'm unassigning (at least until the dependency has landed)

dependency has landed
(In reply to comment #3)
> (In reply to comment #2)
> > I'm unassigning (at least until the dependency has landed)
> 
> dependency has landed

Not quite, but as per our IRC chat I can hack this without the dependency.
Assignee: nobody → bolterbugz
Attached patch patch 1 (obsolete) — Splinter Review
Attachment #423927 - Flags: review?(surkov.alexander)
Status: NEW → ASSIGNED
Attachment #423927 - Attachment is obsolete: true
Attachment #423939 - Flags: review?(surkov.alexander)
Attachment #423927 - Flags: review?(surkov.alexander)
1) Don't store nsAccessibiltiyService pointers locally, make sure proper methods are inline
2) I think we could have global GetAccService inside of nsAccesibilityService.cpp so that we won't need nsAccessNode::GetAccService().
Attached patch patch 1.3 (obsolete) — Splinter Review
Attachment #423939 - Attachment is obsolete: true
Attachment #424030 - Flags: review?(surkov.alexander)
Attachment #423939 - Flags: review?(surkov.alexander)
note i left two minor cases of local storage of the accservice, where we have iterators.
Attachment #424030 - Flags: review?(surkov.alexander)
Attachment #424030 - Attachment is obsolete: true
Attachment #424070 - Flags: review?(surkov.alexander)
Comment on attachment 424070 [details] [diff] [review]
patch 1.4 (putting the addref back)


> static NS_IMETHODIMP
> NS_ConstructAccessibilityService(nsISupports *aOuter, REFNSIID aIID, void **aResult)
> {

>+    rv = NS_GetAccessibilityService(aIID, aResult);

define nsresult right here

>     if (NS_FAILED(rv)) {
>         NS_ERROR("Unable to construct accessibility service");
>         return rv;
>     }
>-    rv = accessibility->QueryInterface(aIID, aResult);
>-    NS_ASSERTION(NS_SUCCEEDED(rv), "unable to find correct interface");

we would like to save this assertion. So you either check nsresult or move assertion into NS_GetAccessibilityService.

> 
>-extern nsresult
>-NS_GetAccessibilityService(nsIAccessibilityService** aResult);
>+extern nsresult NS_GetAccessibilityService(REFNSIID aIID, void **aResult);

extern nsresult looks fine on own line

>     nsCOMPtr<nsIPresShell> presShell(do_QueryReferent(mWeakShell));
>     if (presShell) {
>       nsCOMPtr<nsIDOMNode> docNode(do_QueryInterface(presShell->GetDocument()));
>       if (docNode) {
>-        nsIAccessibilityService *accService = GetAccService();
>-        if (accService) {
>-          nsCOMPtr<nsIAccessible> accessible;
>-          accService->GetAccessibleInShell(docNode, presShell,
>-                                           getter_AddRefs(accessible));
>-          docAccessible = do_QueryInterface(accessible);
>-        }
>+        nsCOMPtr<nsIAccessible> accessible;
>+        GetAccService()->GetAccessibleInShell(docNode, presShell,
>+                                              getter_AddRefs(accessible));

use GetAccessibleInWeakShell since GetAccessibleInShell is public, also I hope it allows to get rid local presShell

>-nsresult 
>-nsAccessibilityService::GetAccessibilityService(nsIAccessibilityService** aResult)
>+nsresult
>+NS_GetAccessibilityService(REFNSIID aIID, void **aResult)
> {
>   NS_ENSURE_TRUE(aResult, NS_ERROR_NULL_POINTER);
>   *aResult = nsnull;
> 
>-  if (!gAccessibilityService) {
>-    gAccessibilityService = new nsAccessibilityService();
>-    NS_ENSURE_TRUE(gAccessibilityService, NS_ERROR_OUT_OF_MEMORY);
>-
>-    gIsShutdown = PR_FALSE;
>+  if (!nsAccessibilityService::gAccessibilityService) {
>+    nsAccessibilityService::gAccessibilityService = new nsAccessibilityService();
>+    NS_ENSURE_TRUE(nsAccessibilityService::gAccessibilityService, NS_ERROR_OUT_OF_MEMORY);
>+    nsAccessibilityService::gAccessibilityService->QueryInterface(aIID, aResult);
>+    nsAccessibilityService::gIsShutdown = PR_FALSE;
>   }

what's about query if service was requested second time?

>   /**
>+   * Handy global function to get the accessibility service instance.
>+   */

I would prefer to see the comment before declaration actually

>+  friend nsAccessibilityService* GetAccService();
>+
>+  /**
>+   * Return accessibility service; creating one if necessary.
>+   */
>+  friend nsresult  NS_GetAccessibilityService(REFNSIID aIID, void **aResult);

move friends into private section, far from eyes.

>+  /**
>+   * Return an nsIAccessible for a DOM node in the given weak shell.

return an accessible for the given DOM node

probably presshell is more descriptive? I feel weak shell is our term - probably I'm wrong.

>+   * 
>+   * @param aNode       [in] The DOM node to get an accessible for.

the given node

>+   * @param aPresShell  [in] The presentation shell which contains layout info for the DOM node.

the presentation shell of the given node

>+inline nsAccessibilityService*
>+GetAccService()
>+{
>+  return nsAccessibilityService::gAccessibilityService;
>+}

doesn't it produce a link warnings?

>+  GetAccService()->
>+                            GetAccessible(mState.domNode, presShell, mWeakShell,
>+                            mState.frame.GetFrame(), &mState.isHidden,
>+                            getter_AddRefs(mState.accessible));

check the formatting: if you move GetAccesible( to a new line then make 2 spaces indent.

>-  nsresult rv;
>+  nsresult rv = NS_ERROR_FAILURE;

do you need nsresult at this point since you removed it below?

>   PRBool goThroughDOMSubtree = PR_TRUE;
> 
>   if (isVisible) {
>     nsCOMPtr<nsIAccessible> accessible;
>-    rv = nsAccessNode::GetAccService()->
>-      GetAccessibleInShell(DOMNode, shell, getter_AddRefs(accessible));
>-    if (NS_SUCCEEDED(rv) && accessible) {
>+    GetAccService()->GetAccessibleInShell(DOMNode, shell,
>+                                               getter_AddRefs(accessible));
>+    if (accessible) {
>       rv = AppendFromAccessible(accessible, aString);

> 
>-void nsHTMLSelectableAccessible::iterator::AddAccessibleIfSelected(nsIAccessibilityService *aAccService, 
>+void nsHTMLSelectableAccessible::iterator::AddAccessibleIfSelected(nsAccessibilityService *aAccService, 
>                                                                    nsIMutableArray *aSelectedAccessibles, 
>                                                                    nsPresContext *aContext)

while you are here move "void" on own line, and remove the service from arguments

> PRBool nsHTMLSelectableAccessible::iterator::GetAccessibleIfSelected(PRInt32 aIndex, 
>-                                                                     nsIAccessibilityService *aAccService, 
>+                                                                     nsAccessibilityService *aAccService, 
>                                                                      nsPresContext *aContext, 
>                                                                      nsIAccessible **aAccessible)

the same.

>   nsHTMLSelectableAccessible::iterator iter(this, mWeakShell);
>+  nsAccessibilityService *accService = GetAccService();

it would be nice if you'll get rid local pointers

>   nsHTMLSelectableAccessible::iterator iter(this, mWeakShell);
>+  nsAccessibilityService *accService = GetAccService();

the same

>+    nsCOMPtr<nsIAccessible> selAcc;
>+    GetAccService()->GetAccessibleFor(selectNode, getter_AddRefs(selAcc));

use GetAccessibleInWeakShell - if nodes lives in the same document.

>-    void AddAccessibleIfSelected(nsIAccessibilityService *aAccService, 
>+    void AddAccessibleIfSelected(nsAccessibilityService *aAccService, 
>                                  nsIMutableArray *aSelectedAccessibles, 
>                                  nsPresContext *aContext);

would be nice to have a comment while you're here ;)

>-    PRBool GetAccessibleIfSelected(PRInt32 aIndex, nsIAccessibilityService *aAccService, nsPresContext *aContext, nsIAccessible **_retval);
>+    PRBool GetAccessibleIfSelected(PRInt32 aIndex, nsAccessibilityService *aAccService, nsPresContext *aContext, nsIAccessible **_retval);

change argument name while you're here

>   if (selectedItem) {
>-    nsCOMPtr<nsIAccessibilityService> accService = GetAccService();
>-    if (accService) {
>-      accService->GetAccessibleInWeakShell(selectedItem, mWeakShell, aAccessible);
>-      if (*aAccessible) {
>-        NS_ADDREF(*aAccessible);

it's a leak
Attachment #424070 - Flags: review?(surkov.alexander)
Thanks Alex. I fixed a lot of your catches. Unfortunately I didn't run test my patch before asking you to look at it... I'm working on some bugs now.
Attached patch patch 2 (obsolete) — Splinter Review
I think this covers everything? Note I pulled the getAccessibleInWindow removal out for separate bug 543033. I've run this patch locally with no leaks and a full a11y test suite pass.
Attachment #424070 - Attachment is obsolete: true
Attachment #424276 - Flags: review?(surkov.alexander)
Comment on attachment 424276 [details] [diff] [review]
patch 2


> %{ C++
> 
> // for component registration
> // {DE401C37-9A7F-4278-A6F8-3DE2833989EF}
> #define NS_ACCESSIBILITY_SERVICE_CID \
>-{ 0xde401c37, 0x9a7f, 0x4278, { 0xa6, 0xf8, 0x3d, 0xe2, 0x83, 0x39, 0x89, 0xef } }
>+  { 0x84a3ab70, 0x8f7e, 0x4610, \
>+    { 0x9c, 0xd8, 0xbd, 0x69, 0x30, 0x8b, 0x76, 0xc5 } }

Do we really need to change the service cid? Why don't you change the cid of retrieval service? If so then please update the comment with the new uuid.

> nsresult
> NS_GetAccessibilityService(nsIAccessibilityService** aResult)
> {
>-  return nsAccessibilityService::GetAccessibilityService(aResult);
>+   NS_ENSURE_TRUE(aResult, NS_ERROR_NULL_POINTER);
>+   *aResult = nsnull;
>+ 
>+  if (!GetAccService()) {

be consistent, nsAccessibilityService::gAccessibilityService usage is better for code readability.

>+    nsAccessibilityService::gAccessibilityService = new nsAccessibilityService();
>+    NS_ENSURE_TRUE(nsAccessibilityService::gAccessibilityService, NS_ERROR_OUT_OF_MEMORY);
>+ 
>+    nsAccessibilityService::gIsShutdown = PR_FALSE;
>+   }

>+   * Return an accessible for a DOM node in the given pres shell.
>+   * 
>+   * @param aNode       [in] the given node.
>+   * @param aPresShell  [in] The presentation shell of the given node.

nit:  "The" should be in lowercase

>+  /**
>+   * Return the accessibility service instance.
>+   */
>+  friend nsAccessibilityService* GetAccService();
>+
>+  /**
>+   * Return accessibility service; creating one if necessary.
>+   */
>+  friend nsresult  NS_GetAccessibilityService(nsIAccessibilityService** aResult);

again, friend is not method declaration, move comments to proper place please where these methods are really defined.


>+// Handy global function to get the accessibility service instance.
>+inline nsAccessibilityService*

use java like comment style, btw, it's funny to have two different wording for the one method.

>+GetAccService()
>+{
>+  return nsAccessibilityService::gAccessibilityService;
>+}


>-void nsHTMLSelectableAccessible::iterator::AddAccessibleIfSelected(nsIAccessibilityService *aAccService, 
>+void
>+nsHTMLSelectableAccessible::iterator::AddAccessibleIfSelected(nsAccessibilityService *aAccService, 

again, could you please remove the service from arguments?

>   public:
>     iterator(nsHTMLSelectableAccessible *aParent, nsIWeakReference *aWeakShell);
> 
>     void CalcSelectionCount(PRInt32 *aSelectionCount);
>     void Select(PRBool aSelect);
>-    void AddAccessibleIfSelected(nsIAccessibilityService *aAccService, 
>+    void AddAccessibleIfSelected(nsAccessibilityService *aAccService, 
>                                  nsIMutableArray *aSelectedAccessibles, 
>                                  nsPresContext *aContext);
>-    PRBool GetAccessibleIfSelected(PRInt32 aIndex, nsIAccessibilityService *aAccService, nsPresContext *aContext, nsIAccessible **_retval);
>+    PRBool GetAccessibleIfSelected(PRInt32 aIndex, nsAccessibilityService *aAccService, nsPresContext *aContext, nsIAccessible **_retval);

still no comments and wrong styled arguments names


>+    GetAccService()->GetAccessibleInWeakShell(selectedItem, mWeakShell,
>+                                              aAccessible);
>+    if (*aAccessible) {
>+      NS_ADDREF(*aAccessible);
>+      return NS_OK;
>     }

again, it's a leak.

please address all comments.
Attachment #424276 - Flags: review?(surkov.alexander)
Attached patch patch 2.1Splinter Review
(In reply to comment #14)
> (From update of attachment 424276 [details] [diff] [review])
> 
> > %{ C++
> > 
> > // for component registration
> > // {DE401C37-9A7F-4278-A6F8-3DE2833989EF}
> > #define NS_ACCESSIBILITY_SERVICE_CID \
> >-{ 0xde401c37, 0x9a7f, 0x4278, { 0xa6, 0xf8, 0x3d, 0xe2, 0x83, 0x39, 0x89, 0xef } }
> >+  { 0x84a3ab70, 0x8f7e, 0x4610, \
> >+    { 0x9c, 0xd8, 0xbd, 0x69, 0x30, 0x8b, 0x76, 0xc5 } }
> 
> Do we really need to change the service cid?

I think yes.

> Why don't you change the cid of
> retrieval service? If so then please update the comment with the new uuid.

Done.

> 
> > nsresult
> > NS_GetAccessibilityService(nsIAccessibilityService** aResult)
> > {
> >-  return nsAccessibilityService::GetAccessibilityService(aResult);
> >+   NS_ENSURE_TRUE(aResult, NS_ERROR_NULL_POINTER);
> >+   *aResult = nsnull;
> >+ 
> >+  if (!GetAccService()) {
> 
> be consistent, nsAccessibilityService::gAccessibilityService usage is better
> for code readability.

Done.

> 
> >+    nsAccessibilityService::gAccessibilityService = new nsAccessibilityService();
> >+    NS_ENSURE_TRUE(nsAccessibilityService::gAccessibilityService, NS_ERROR_OUT_OF_MEMORY);
> >+ 
> >+    nsAccessibilityService::gIsShutdown = PR_FALSE;
> >+   }
> 
> >+   * Return an accessible for a DOM node in the given pres shell.
> >+   * 
> >+   * @param aNode       [in] the given node.
> >+   * @param aPresShell  [in] The presentation shell of the given node.
> 
> nit:  "The" should be in lowercase

Done.


> again, friend is not method declaration, move comments to proper place please
> where these methods are really defined.
> use java like comment style, btw, it's funny to have two different wording for
> the one method.

Done and done.

> >-void nsHTMLSelectableAccessible::iterator::AddAccessibleIfSelected(nsIAccessibilityService *aAccService, 
> >+void
> >+nsHTMLSelectableAccessible::iterator::AddAccessibleIfSelected(nsAccessibilityService *aAccService, 
> 
> again, could you please remove the service from arguments?

OK done.

> 
> >   public:
> >     iterator(nsHTMLSelectableAccessible *aParent, nsIWeakReference *aWeakShell);
> > 
> >     void CalcSelectionCount(PRInt32 *aSelectionCount);
> >     void Select(PRBool aSelect);
> >-    void AddAccessibleIfSelected(nsIAccessibilityService *aAccService, 
> >+    void AddAccessibleIfSelected(nsAccessibilityService *aAccService, 
> >                                  nsIMutableArray *aSelectedAccessibles, 
> >                                  nsPresContext *aContext);
> >-    PRBool GetAccessibleIfSelected(PRInt32 aIndex, nsIAccessibilityService *aAccService, nsPresContext *aContext, nsIAccessible **_retval);
> >+    PRBool GetAccessibleIfSelected(PRInt32 aIndex, nsAccessibilityService *aAccService, nsPresContext *aContext, nsIAccessible **_retval);
> 
> still no comments and wrong styled arguments names

Fixed, but passing on comment ;)

> 
> 
> >+    GetAccService()->GetAccessibleInWeakShell(selectedItem, mWeakShell,
> >+                                              aAccessible);
> >+    if (*aAccessible) {
> >+      NS_ADDREF(*aAccessible);
> >+      return NS_OK;
> >     }
> 
> again, it's a leak.
> 

OK this is separate - I'll fix on bug 543478
Attachment #424276 - Attachment is obsolete: true
Attachment #424603 - Flags: review?(surkov.alexander)
(In reply to comment #15)
> > > // for component registration
> > > // {DE401C37-9A7F-4278-A6F8-3DE2833989EF}
> > > #define NS_ACCESSIBILITY_SERVICE_CID \
> > >-{ 0xde401c37, 0x9a7f, 0x4278, { 0xa6, 0xf8, 0x3d, 0xe2, 0x83, 0x39, 0x89, 0xef } }
> > >+  { 0x84a3ab70, 0x8f7e, 0x4610, \
> > >+    { 0x9c, 0xd8, 0xbd, 0x69, 0x30, 0x8b, 0x76, 0xc5 } }
> > 
> > Do we really need to change the service cid?
> 
> I think yes.

Why? We never did this iirc :)
Comment on attachment 424603 [details] [diff] [review]
patch 2.1


> // for component registration
>-// {DE401C37-9A7F-4278-A6F8-3DE2833989EF}
>+// {84A3AB70-8f7E-4610-9CD8-BD69308B76C5}

I'm still not sure if we should change the uuid but please make it lowercase.

>@@ -147,24 +147,24 @@ nsTextEquivUtils::AppendTextEquivFromCon
>   }

>-  nsresult rv;
>+  nsresult rv = NS_ERROR_FAILURE;

>+++ b/accessible/src/html/nsHTMLSelectAccessible.h
>-#include "nsIAccessibilityService.h"
>+#include "nsAccessibilityService.h"

you don't need since it's included in nsAccessNode.h

>+    void AddAccessibleIfSelected(nsIMutableArray *aSelectedAccessibles, 
>                                  nsPresContext *aContext);
>-    PRBool GetAccessibleIfSelected(PRInt32 aIndex, nsIAccessibilityService *aAccService, nsPresContext *aContext, nsIAccessible **_retval);
>+    PRBool GetAccessibleIfSelected(PRInt32 aIndex, nsPresContext *aContext,
>+                                   nsIAccessible **_retval);

please rename "_retval" to aAccessible.

>+++ b/accessible/src/msaa/nsAccessNodeWrap.cpp
>@@ -34,17 +34,17 @@
>-#include "nsIAccessibilityService.h"
>+#include "nsAccessibilityService.h"

the same

r=me but please fix argument names.
Attachment #424603 - Flags: review?(surkov.alexander) → review+
(In reply to comment #17)
> (From update of attachment 424603 [details] [diff] [review])
> 
> > // for component registration
> >-// {DE401C37-9A7F-4278-A6F8-3DE2833989EF}
> >+// {84A3AB70-8f7E-4610-9CD8-BD69308B76C5}
> 
> I'm still not sure if we should change the uuid but please make it lowercase.
> 

OK I think you are right and I should update the uuid but not the component id (I confirmed with Enn). Fixed.

(fixed the rest locally)

Thanks for your patience on this one.
landed 1.9.3 http://hg.mozilla.org/mozilla-central/rev/fb6bebbcf7f3
(Wasn't exactly the commit message I meant but close enough)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.