Closed Bug 1415408 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Disability Access APIs, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: yzen, Assigned: yzen)

Details

(Keywords: access)

Attachments

(1 file, 2 obsolete files)

No description provided.
Keywords: access
Summary: Send → Send 'a11y-consumers-change' notifcation whenever accessibility services consumers change.
We want to notify interested clients whenever a11y service consumers change. This will help with deciding if accessibility services can be, for example, shutdown.
Attached patch 1415408 v1 (obsolete) — Splinter Review
Attachment #8926232 - Flags: review?(surkov.alexander)
Attached patch 1415408 v2 (obsolete) — Splinter Review
small fix
Attachment #8926232 - Attachment is obsolete: true
Attachment #8926232 - Flags: review?(surkov.alexander)
Attachment #8926233 - Flags: review?(surkov.alexander)
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
Attachment #8926232 - Attachment is obsolete: false
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
	
}
Attached patch 1415408 v3Splinter Review
Attachment #8926232 - Attachment is obsolete: true
Attachment #8926233 - Attachment is obsolete: true
Attachment #8926233 - Flags: review?(surkov.alexander)
Attachment #8926504 - Flags: review?(surkov.alexander)
(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 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
Attachment #8926504 - Flags: review?(surkov.alexander) → review+
(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.
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b807149a0db
send 'a11y-consumers-changed' notifcation whenever accessibility services consumers change. r=surkov
https://hg.mozilla.org/mozilla-central/rev/8b807149a0db
https://hg.mozilla.org/mozilla-central/rev/e81c3337dec6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.