1415408 - Send 'a11y-consumers-change' notifcation whenever accessibility services consumers change.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

We want to notify interested clients whenever a11y service consumers change. This will help with deciding if accessibility services can be, for example, shutdown.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1415408 v1

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1415408 v2

small fix

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8926232 [details] [diff] [review]
1415408 v1

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

::: accessible/base/nsAccessibilityService.cpp
@@ +1386,5 @@
>  
>    MOZ_ASSERT(gConsumers, "Accessibility was shutdown already");
>  
>    gConsumers = 0;
> +  this->NotifyOfConsumerChange();

so if the main process start child process a11y, then we trigger a11y-consumers-changed with all false?

@@ +1882,5 @@
> +  if (!(nsAccessibilityService::gConsumers & aNewConsumer)) {
> +    nsAccessibilityService::gConsumers |= aNewConsumer;
> +    nsAccessibilityService* accService =
> +      nsAccessibilityService::gAccessibilityService;
> +    accService->NotifyOfConsumerChange();

nit: keep in sync naming: Consumer in method name vs Consumers in notification name

btw, you don't need accService variable

@@ +1908,5 @@
>        nsAccessibilityService::eXPCOM;
> +    // Only notify about consumers change when they actually changed.
> +    if (aFormerConsumer != nsAccessibilityService::eXPCOM) {
> +      accService->NotifyOfConsumerChange();
> +    }

I would consider to wrap gConsumer access by SetConsumer and UnsetConsumer methods, which would take a responsibility to notify observers

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8926232 [details] [diff] [review]
1415408 v1

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

::: accessible/base/nsAccessibilityService.cpp
@@ +1386,5 @@
>  
>    MOZ_ASSERT(gConsumers, "Accessibility was shutdown already");
>  
>    gConsumers = 0;
> +  this->NotifyOfConsumerChange();

No, as you can see from the test they start with:
{
	
      XPCOM: false, MainProcess: true, PlatformAPI: false
	
}

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1415408 v3

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #5)
> Comment on attachment 8926232 [details] [diff] [review]
> 1415408 v1
> 
> Review of attachment 8926232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/base/nsAccessibilityService.cpp
> @@ +1386,5 @@
> >  
> >    MOZ_ASSERT(gConsumers, "Accessibility was shutdown already");
> >  
> >    gConsumers = 0;
> > +  this->NotifyOfConsumerChange();
> 
> No, as you can see from the test they start with:
> {
> 	
>       XPCOM: false, MainProcess: true, PlatformAPI: false
> 	
> }

this is not possible right? If gConsumers is 0, then gConsumers & eMainProcess == false

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 8926504 [details] [diff] [review]
1415408 v3

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

::: accessible/base/nsAccessibilityService.cpp
@@ +1384,5 @@
>    // Don't null accessibility service static member at this point to be safe
>    // if someone will try to operate with it.
>  
>    MOZ_ASSERT(gConsumers, "Accessibility was shutdown already");
> +  UnsetConsumers(eXPCOM | eMainProcess | ePlatformAPI);

gConsumers = 0 is a safer way to uninitialize for sure, in case of changing the list of supported values in the future. Not necessary, but you could have ClearConsumers method.

Btw, we may file a follow up to enclose gConsumers into a class like
class Consumers {
  void Set()
  void Unset();
  void Clear();
  bool Has()
private:
  int mConsumers;
}

@@ +1916,5 @@
>        accService->HasXPCDocuments()) {
>      // Still used by XPCOM
> +    if (aFormerConsumer != nsAccessibilityService::eXPCOM) {
> +      // Only unset non-XPCOM consumers.
> +      accService->UnsetConsumers(aFormerConsumer);

you change the behavior here, we used to set up eXPCOM flag, not sure if I can see reasoning behind this. Just double checking if that is on-purpose change.

btw, you can have a single if here

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to alexander :surkov from comment #8)
> Comment on attachment 8926504 [details] [diff] [review]
> 1415408 v3
> 
> Review of attachment 8926504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/base/nsAccessibilityService.cpp
> @@ +1384,5 @@
> >    // Don't null accessibility service static member at this point to be safe
> >    // if someone will try to operate with it.
> >  
> >    MOZ_ASSERT(gConsumers, "Accessibility was shutdown already");
> > +  UnsetConsumers(eXPCOM | eMainProcess | ePlatformAPI);
> 
> gConsumers = 0 is a safer way to uninitialize for sure, in case of changing
> the list of supported values in the future. Not necessary, but you could
> have ClearConsumers method.
> 
> Btw, we may file a follow up to enclose gConsumers into a class like
> class Consumers {
>   void Set()
>   void Unset();
>   void Clear();
>   bool Has()
> private:
>   int mConsumers;
> }
> 
> @@ +1916,5 @@
> >        accService->HasXPCDocuments()) {
> >      // Still used by XPCOM
> > +    if (aFormerConsumer != nsAccessibilityService::eXPCOM) {
> > +      // Only unset non-XPCOM consumers.
> > +      accService->UnsetConsumers(aFormerConsumer);
> 
> you change the behavior here, we used to set up eXPCOM flag, not sure if I
> can see reasoning behind this. Just double checking if that is on-purpose
> change.

Yeah it was on purpose. Before we pretty much cleared former consumer regardless of what it was and re-set the xpcom one (even if it was cleared). Logic is basically the same - only reset non-xpcom one, leave the xpcom one set.

> 
> btw, you can have a single if here

We want to return always but not always unset the consumer.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b807149a0db
send 'a11y-consumers-changed' notifcation whenever accessibility services consumers change. r=surkov

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e81c3337dec6
followup, placate eslint

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8b807149a0db
https://hg.mozilla.org/mozilla-central/rev/e81c3337dec6