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)
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)
12.21 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
6.34 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8801893 -
Flags: review?(jmathies)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8801897 -
Flags: review?(tbsaunde+mozbugs)
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Comment 5•8 years ago
|
||
(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! :)
Assignee | ||
Comment 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
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
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
Flags: needinfo?(aklotz)
Comment 9•8 years ago
|
||
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
Assignee | ||
Comment 10•8 years ago
|
||
(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)
Assignee | ||
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
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+
Assignee | ||
Comment 14•8 years ago
|
||
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
Comment 15•8 years ago
|
||
bugherder |
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
You need to log in
before you can comment on or make changes to this bug.
Description
•