get rid nsIAccessibilityService

NEW
Assigned to

Status

()

Core
Disability Access APIs
6 years ago
3 years ago

People

(Reporter: surkov, Assigned: debkanya, Mentored)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=c++][leave open])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
Assignee: nobody → jigneshhk1992
(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().
Created attachment 610017 [details] [diff] [review]
Part1

Removed all virtual CreateFooAccessible() from nsIAccessibilityService and
marked the impls on nsAccessibilityService as non-virtual.
Attachment #610017 - Flags: feedback?(trev.saunders)
Attachment #610017 - Flags: feedback?(trev.saunders) → review+
(Reporter)

Comment 3

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3923a202ed23
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++] → [good first bug][mentor=trev.saunders@gmail.com][lang=c++][leave open]
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14

Comment 5

6 years ago
Assuming this was meant to stay open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Josh Matthews [:jdm] from comment #5)
> Assuming this was meant to stay open.

yup, still more goblins :)
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++][leave open] → [leave open][good first bug][mentor=trev.saunders@gmail.com][lang=c++][leave open]
(Reporter)

Updated

6 years ago
Depends on: 763150
Keeping open.
Assignee: jigneshhk1992 → nobody
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?
(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.
(Assignee)

Comment 10

4 years ago
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

4 years ago
Hi, Debkanya - are you still interested in this?
Status: REOPENED → NEW
Flags: needinfo?(trev.saunders)
I'm not sure what you want to know from me?
Flags: needinfo?(trev.saunders)

Comment 13

4 years ago
Sorry - Are you still willing to mentor this bug?
(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.
Mentor: trev.saunders@gmail.com
Whiteboard: [leave open][good first bug][mentor=trev.saunders@gmail.com][lang=c++][leave open] → [leave open][good first bug][lang=c++][leave open]
(Assignee)

Comment 15

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

Comment 16

3 years ago
Trevor, can you give us a summary of where we stand on this bug? What are the next actions here?
Flags: needinfo?(trev.saunders)
(Assignee)

Comment 17

3 years ago
Created attachment 8463523 [details] [diff] [review]
Part 2:
Attachment #8463523 - Flags: review?(trev.saunders)
Comment on attachment 8463523 [details] [diff] [review]
Part 2:

seems like a weird way to start to me, whatever this is fine.
Attachment #8463523 - Flags: review?(trev.saunders) → review+
Flags: needinfo?(trev.saunders)
(Reporter)

Comment 19

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

Comment 20

3 years ago
Created attachment 8463581 [details] [diff] [review]
Part 3 - replace with GetAccService()->MethodName()  for those which relies on nsIAccessibilityService methods
Attachment #8463581 - Flags: review?(trev.saunders)
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.
Attachment #8463581 - Flags: review?(trev.saunders) → review+
Keywords: checkin-needed
(Assignee)

Comment 22

3 years ago
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(..)?
(Assignee)

Comment 23

3 years ago
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.
Attachment #8463581 - Attachment is obsolete: true
Attachment #8468770 - Flags: review?(trev.saunders)
Assignee: nobody → rimjhim.mazumder17
Whiteboard: [leave open][good first bug][lang=c++][leave open] → [good first bug][lang=c++][leave open]
Target Milestone: mozilla14 → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7488f306eb4
Keywords: checkin-needed
(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.
https://hg.mozilla.org/mozilla-central/rev/f7488f306eb4
(Assignee)

Comment 27

3 years ago
Can I make use of use GetOrCreateAccessible[1]? 

[1] http://dxr.mozilla.org/mozilla-central/source/accessible/base/nsAccessibilityService.cpp#807
(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
(Assignee)

Comment 29

3 years ago
Created attachment 8473208 [details] [diff] [review]
Bug 738221 - Part3
Attachment #8468770 - Attachment is obsolete: true
Attachment #8468770 - Flags: review?(trev.saunders)
Attachment #8473208 - Flags: review?(trev.saunders)
(Assignee)

Comment 30

3 years ago
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?
(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 on attachment 8473208 [details] [diff] [review]
Bug 738221 - Part3

Please update the patch based on my previous comment.
Attachment #8473208 - Flags: review?(trev.saunders)
You need to log in before you can comment on or make changes to this bug.