Closed
Bug 1415408
Opened 8 years ago
Closed 8 years ago
Send 'a11y-consumers-change' notifcation whenever accessibility services consumers change.
Categories
(Core :: Disability Access APIs, enhancement)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: yzen, Assigned: yzen)
Details
(Keywords: access)
Attachments
(1 file, 2 obsolete files)
13.76 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•8 years ago
|
Keywords: access
Summary: Send → Send 'a11y-consumers-change' notifcation whenever accessibility services consumers change.
Assignee | ||
Comment 1•8 years ago
|
||
We want to notify interested clients whenever a11y service consumers change. This will help with deciding if accessibility services can be, for example, shutdown.
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8926232 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 3•8 years ago
|
||
small fix
Attachment #8926232 -
Attachment is obsolete: true
Attachment #8926232 -
Flags: review?(surkov.alexander)
Attachment #8926233 -
Flags: review?(surkov.alexander)
Comment 4•8 years ago
|
||
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
Assignee | ||
Comment 5•8 years ago
|
||
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
}
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8926232 -
Attachment is obsolete: true
Attachment #8926233 -
Attachment is obsolete: true
Attachment #8926233 -
Flags: review?(surkov.alexander)
Attachment #8926504 -
Flags: review?(surkov.alexander)
Comment 7•8 years ago
|
||
(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•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
(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•8 years ago
|
||
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
Comment 11•8 years ago
|
||
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e81c3337dec6
followup, placate eslint
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8b807149a0db
https://hg.mozilla.org/mozilla-central/rev/e81c3337dec6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•