Closed Bug 1119979 Opened 7 years ago Closed 7 years ago

hide IPC a11y architecture under pref

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: surkov, Assigned: surkov)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
No description provided.
Attachment #8546878 - Flags: review?(dbolter)
Comment on attachment 8546878 [details] [diff] [review]
patch

>   return false;
> #else
>-  return XRE_GetProcessType() == GeckoProcessType_Content;
>+  return XRE_GetProcessType() == GeckoProcessType_Content &&
>+    mozilla::Preferences::GetBool("accessibility.ipc_architecture.enabled", false);
> #endif

I don't see why this is a useful pref, since its basically "please don't work", so even if its useful for something defaulting to false seems wrong.
(In reply to Trevor Saunders (:tbsaunde) from comment #1)

> I don't see why this is a useful pref, since its basically "please don't
> work", so even if its useful for something defaulting to false seems wrong.

false is ok until we have something ready for testing
Is my understanding below correct?

If e10s is enabled, accessibility will be completely broken if this is set to false and partly broken if this is set to true.

If e10s is disabled, this setting has no effect.

Normally I think we like new features to default to off but if my understanding is correct then this might be a special case. Alex does it make a big difference to you?
E.g. is false avoiding frequent crashes?
(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> 
> > I don't see why this is a useful pref, since its basically "please don't
> > work", so even if its useful for something defaulting to false seems wrong.
> 
> false is ok until we have something ready for testing

merely contradicting isn't really convincing.


(In reply to David Bolter [:davidb] from comment #3)
> Is my understanding below correct?
> 
> If e10s is enabled, accessibility will be completely broken if this is set
> to false and partly broken if this is set to true.

It probably depends on the platform, but setting it to false is asking for a configuration I don't care about.

> If e10s is disabled, this setting has no effect.

yes

> Normally I think we like new features to default to off but if my
> understanding is correct then this might be a special case. Alex does it
> make a big difference to you?

I think that only applies to web facing things.
(In reply to David Bolter [:davidb] from comment #3)
> Is my understanding below correct?
> 
> If e10s is enabled, accessibility will be completely broken if this is set
> to false and partly broken if this is set to true.

No. On Windows the browser is still accessible (with bunch of problems of course).

> Normally I think we like new features to default to off but if my
> understanding is correct then this might be a special case. Alex does it
> make a big difference to you?

I don't feel it special, I see it causes problems from time to time (like periodical top crashers), but if you insist that it has to be running by default then it's fine with me.
Comment on attachment 8546878 [details] [diff] [review]
patch

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

r=me.

I think the crashers provide value so my preference is to default to true. I reserve the right to change my mind later :)
Attachment #8546878 - Flags: review?(dbolter) → review+
https://hg.mozilla.org/mozilla-central/rev/66c9bbebe52c
Assignee: nobody → surkov.alexander
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.