738221 - get rid nsIAccessibilityService

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

See http://mxr.mozilla.org/mozilla-central/ident?i=nsIAccessibilityService&filter=

1) remove nsIAccessibilityService.h and fix nsAccessibilityService class including removal 'virtual' for every nsAccessibileService method coming from nsIAccessibilityService

2) replace
nsCOMPtr<nsIAccessibilityService> accService =
  do_GetService("@mozilla.org/accessibilityService;1");
by
nsCOMPtr<nsIAccessibleRetrieval> accService =
  do_GetService("@mozilla.org/accessibleRetrieval;1");

3) for those who relies on nsIAccessibilityService methods do
GetAccService()->MethodName() declared in nsAccessibilityService.h

Comment 1

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #0)
> See
> http://mxr.mozilla.org/mozilla-central/
> ident?i=nsIAccessibilityService&filter=
> 
> 1) remove nsIAccessibilityService.h and fix nsAccessibilityService class
> including removal 'virtual' for every nsAccessibileService method coming
> from nsIAccessibilityService

it might be good to do this in parts

first remove all virtual CreateFooAccessible() from nsIAccessibilityService and mark the impls on nsAccessibilityService as non-virtual.

next remove the other random virtual helper things and rework users to be nice if needed.

next remove the xpcom goo for component registration and update xpcom/base/ServiceList.h

finally remove the header and stop inheriting nsIAccessibilityService.

> 2) replace
> nsCOMPtr<nsIAccessibilityService> accService =
>   do_GetService("@mozilla.org/accessibilityService;1");
> by
> nsCOMPtr<nsIAccessibleRetrieval> accService =
>   do_GetService("@mozilla.org/accessibleRetrieval;1");

actually, while you do this use mozilla::services::GetAccessibilityService().

Comment 2

[Jignesh Kakadiya [:jhk]]

Attachment: Part1

Removed all virtual CreateFooAccessible() from nsIAccessibilityService and
marked the impls on nsAccessibilityService as non-virtual.

Comment 3

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3923a202ed23

Comment 4

[Matt Brubeck (:mbrubeck)]

This is checked in to mozilla-central for Firefox 14, and will be included in tomorrow's Nightly build.  Thanks!
https://hg.mozilla.org/mozilla-central/rev/3923a202ed23

Comment 5

[Josh Matthews [:jdm]]

Assuming this was meant to stay open.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Josh Matthews [:jdm] from comment #5)
> Assuming this was meant to stay open.

yup, still more goblins :)

Comment 7

[Jignesh Kakadiya [:jhk]]

Keeping open.

Comment 8

[Mark Capella [:capella]]

So what are the plans for / how do we handle http://mxr.mozilla.org/mozilla-central/ident?i=NS_GetAccessibilityService and nsAccessibilityFactory? They're going away?

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #8)
> So what are the plans for / how do we handle
> http://mxr.mozilla.org/mozilla-central/ident?i=NS_GetAccessibilityService
> and nsAccessibilityFactory? They're going away?


NS_ConstructAccessibilityService() atleast needs to stay although maybe we could implemente it with the macros in mozilla/Module.h I think it is.  Mostly they'll just be changed to return a nsIAccessibleRetrieval* instead of a nsIAccessibilityService* and cleaned up. 

Before you worry aboutthat though there are still a few methods to get rid of.

Comment 10

[debkanya]

Hi, This is Debkanya Mazumder. I'm completely new to Mozilla development and I am looking for my first bug to work on. I would like to work on this bug. Could someone guide me about how to go about it? Thank you.

Comment 11

[Mike Hoye [:mhoye]]

Hi, Debkanya - are you still interested in this?

Comment 12

[Trevor Saunders (:tbsaunde)]

I'm not sure what you want to know from me?

Comment 13

[Mike Hoye [:mhoye]]

Sorry - Are you still willing to mentor this bug?

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hoye [:mhoye] from comment #13)
> Sorry - Are you still willing to mentor this bug?

I'm willing to answer questions people have while trying to work on it yes.

Comment 15

[debkanya]

I would like to work on this. Can someone guide me through it?

Comment 16

[Mike Hoye [:mhoye]]

Trevor, can you give us a summary of where we stand on this bug? What are the next actions here?

Comment 17

[debkanya]

Attachment: Part 2:

Comment 18

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8463523 [details] [diff] [review]
Part 2:

seems like a weird way to start to me, whatever this is fine.

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 8463523 [details] [diff] [review]
Part 2:

Review of attachment 8463523 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentChild.cpp
@@ +1621,5 @@
>  #ifdef ACCESSIBILITY
>      // Start accessibility in content process if it's running in chrome
>      // process.
> +	nsCOMPtr<nsIAccessibilityService> accService =
> +        services::GetAccessibilityService();

nit: wrong indentation, here and below

Comment 20

[debkanya]

Attachment: Part 3 - replace with GetAccService()->MethodName() for those which relies on nsIAccessibilityService methods

Comment 21

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8463581 [details] [diff] [review]
Part 3 - replace with GetAccService()->MethodName()  for those which relies on nsIAccessibilityService methods

>   nsCOMPtr<nsIAccessibilityService> accService =
>     services::GetAccessibilityService();
>   if (accService) {
>-    return accService->GetRootDocumentAccessible(presShell, nsContentUtils::IsSafeToRunScript());
>+    return GetAccService()->GetRootDocumentAccessible(presShell, nsContentUtils::IsSafeToRunScript());

its kind of weird to do call services::GetAccessibilityService() and then throw away the result and get it again, and you'll need to update this again, but I guess this is fine.

Comment 22

[debkanya]

Hi, I'm not clear with what I have to update here. Do I remove the accService statement and the if condition and only return the GetAccService()->GetRootDocumentAccessible(..)?

Comment 23

[debkanya]

Attachment: Bug 738221 - Part3

Sorry for taking so much time. I'm not sure about what to do so I've removed accService statement, if statement and the return nullptr statement.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7488f306eb4

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to Debkanya Mazumder from comment #23)
> Created attachment 8468770 [details] [diff] [review]
> Bug 738221 - Part3
> 
> Sorry for taking so much time. I'm not sure about what to do so I've removed
> accService statement, if statement and the return nullptr statement.

take your time, but that doesn't work.  The difference between GetAccService() and services::GetAccessibilityService() is that the first assumes the accessibility service already exists, and the second creates it if it doesn't exist. So you can't just replace services::GetAccessibilityService() with GetAccService() I think you first need to add a GetOrCreateAccService that creates the service if it doesn't exist yet and then use that. you can write GetOrCreateAccService from NS_GetAccessibilityService, turning NS_GetAccessibilityService into a wrapper for GetOrCreateAccessibilityService.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7488f306eb4

Comment 27

[debkanya]

Can I make use of use GetOrCreateAccessible[1]? 

[1] http://dxr.mozilla.org/mozilla-central/source/accessible/base/nsAccessibilityService.cpp#807

Comment 28

[Trevor Saunders (:tbsaunde)]

(In reply to Debkanya Mazumder from comment #27)
> Can I make use of use GetOrCreateAccessible[1]? 

no, that's about accessiblee objects which are different from the accessibility service.  You want to use http://mxr.mozilla.org/mozilla-central/ident?i=NS_GetAccessibilityService

Comment 29

[debkanya]

Attachment: Bug 738221 - Part3

Comment 30

[debkanya]

I'm having a little trouble with using NS_GetAccessibilityService as the wrapper. Can someone tell me how to go about it?

To use NS_GetAccessibilityService(nsIAccessibilityService** aResult) as a wrapper for GetOrCreateAccessibilityService, should the latter have a nsIAccessibilityService return type or nsAccessibilityService return type?

Comment 31

[Trevor Saunders (:tbsaunde)]

(In reply to Debkanya Mazumder from comment #30)
> I'm having a little trouble with using NS_GetAccessibilityService as the
> wrapper. Can someone tell me how to go about it?

rename NS_GetAccessibilityService to GetOrCreateAccessibilityService and change the out argument to a return value.  THen write a new NS_GetAccessibilityService function like this

nsresult
NS_GetAccessibilityService(nsIAccessibilityService** aResult)
{
  NS_ENSURE_ARG_POINTER(aResult);
  NS_IF_ADDREF(*aResult = GetOrCreateAccessibilityService());
  return *aResult ? NS_OK : NS_ERROR_FAILURE;
}

> To use NS_GetAccessibilityService(nsIAccessibilityService** aResult) as a
> wrapper for GetOrCreateAccessibilityService, should the latter have a
> nsIAccessibilityService return type or nsAccessibilityService return type?

nsAccessibilityService*

Comment 32

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8473208 [details] [diff] [review]
Bug 738221 - Part3

Please update the patch based on my previous comment.

Comment 33

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 34

[Sneha k]

Hey! Can I take up this bug? Can you please tell me How to proceed to fix it?