Closed Bug 1310833 Opened 8 years ago Closed 8 years ago

Send content process MSAA ID to child in ActivateA11y

Categories

(Core :: Disability Access APIs, defect)

Unspecified
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: bugzilla, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

This is nicer and gets rid of some problems that could happen with the current implementation if it were to lazily request the content process MSAA Id while the MessageChannel is already waiting on a sync reply.
Attached patch Part 1 - PContent changes (obsolete) — Splinter Review
Attachment #8801893 - Flags: review?(jmathies)
Attached patch Part 2 - MsaaIdGenerator changes (obsolete) — Splinter Review
Attachment #8801897 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8801897 [details] [diff] [review]
Part 2 - MsaaIdGenerator changes

it would really be better to merge this with the other patch since they are interdependent, or restructure them so later patches only depend on earlier ones so you don't break bisection.

otherwise this seems fine.
Attachment #8801897 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8801893 [details] [diff] [review]
Part 1 - PContent changes

since Jim hasn't delt with this yet I will (I generally take the position that I can review a11y stuff in other directories when reasonable) at least until somebody yells at me for it ;)

>+ContentChild::RecvActivateA11y(const uint32_t& aMsaaID)
> {
> #ifdef ACCESSIBILITY
>+#ifdef XP_WIN

you could assert here its non zero

>+#if defined(XP_WIN) && defined(ACCESSIBILITY)
>+  uint32_t mMsaaID;

it would be good to have some comments what this and the accessor are about.  I know, but I doubt random people looking at this file do.

>     /**
>      * Start accessibility engine in content process.
>+     * (aMsaaID only used on Windows)

be nice to comment what it is maybe.
Attachment #8801893 - Flags: review?(jmathies) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> Comment on attachment 8801893 [details] [diff] [review]
> Part 1 - PContent changes
> 
> since Jim hasn't delt with this yet I will (I generally take the position
> that I can review a11y stuff in other directories when reasonable) at least
> until somebody yells at me for it ;)

No problem here! :)
Attached patch Merged patchSplinter Review
Merged patches and addressed review comments. Carrying forward r+.
Attachment #8801893 - Attachment is obsolete: true
Attachment #8801897 - Attachment is obsolete: true
Attachment #8803038 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/069958e96c3de4800b9d00cb7370bd0c6482f538
Bug 1310833: Modify PContentParent::SendActivateA11y to accept the content process's MSAA ID as a parameter; r=tbsaunde
Because just one set of opaque failures in a notoriously opaque suite is never enough, you also had failures in talos-other-e10s on all three versions of Windows like https://treeherder.mozilla.org/logviewer.html#?job_id=38027192&repo=mozilla-inbound
(In reply to Wes Kocher (:KWierso) from comment #8)
> I had to back this out for Windows VM Marionette bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=38005118&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b6556299d0

This is busted due to the fact that marionette tries to instantiate a11y from a content script, without a11y ever having been instantiated in chrome.

I need to file a bug to address this, but what exactly gets filed depends on this question: Is instantiating from content a use case that we want to support? My guess is, "no," but over to Trevor to confirm.
Flags: needinfo?(aklotz) → needinfo?(tbsaunde+mozbugs)
Discussed on IRC.
Flags: needinfo?(tbsaunde+mozbugs)
Depends on: 1312816
This restores the synchronous GetA11yContentId() function, but it is only used when instantiated from a content-based consumer. If content a11y is requested by chrome, it still uses the ID passed down by ActivateA11y().

I attached this as a separate patch so that you wouldn't need to interdiff, but I can also merge it into the first one if you prefer.
Attachment #8805226 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8805226 [details] [diff] [review]
Part 2 - Fix the "Instantiated from content XPCOM" case

ick at half supporting starting a11y in the content process, I worry about cases where that happens by accident and it actually matters, but I guess we can live for now.
Attachment #8805226 - Flags: review?(tbsaunde+mozbugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d74a682eeaebecb0cf31625add0a92821fbb1c
Bug 1310833: Modify PContentParent::SendActivateA11y to accept the content process's MSAA ID as a parameter; r=tbsaunde

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f15ff10cb267d4289655b2e3cf0a0dee48e7c0b
Bug 1310833: Make nsAccessibilityService synchronously query for its MSAA content process ID if the ID is not yet present; r=tbsaunde
https://hg.mozilla.org/mozilla-central/rev/e8d74a682eea
https://hg.mozilla.org/mozilla-central/rev/3f15ff10cb26
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1314310
You need to log in before you can comment on or make changes to this bug.