[e10s a11y] Eliminate pointless cross-process QueryInterface for IAccessibleText

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Jamie, Assigned: Jamie)

Tracking

Trunk
mozilla58
All
Windows
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
AccessibleHandler's AccessibleTextTearoff currently makes separate queries for the IAccessibleText and IAccessibleHypertext interfaces. However, IAccessibleHypertext inherits from IAccessibleText, and wherever one of these interfaces are present, Gecko always implements both. NVDA (and possibly other clients) always queries for both interfaces when rendering. Therefore, we should just query for IAccessibleHypertext for all of these methods, thus saving a cross-process call should a client want both interfaces. There is no penalty if a client only wants one of them, so we only have something to gain and nothing to lose here.
(Assignee)

Updated

a year ago
Assignee: nobody → jteh
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
Win64 build: https://public-artifacts.taskcluster.net/e1QK3zpOQambcSf4bFgYig/0/public/build/target.zip
Marco, if you have a chance, it'd be good if you could compare this against nightly to see if it yields a noticeable perf improvement. If it does, we should request uplift, as it is a fairly simple patch.
This try build behaves as if bug 1363595 and friends never landed, e. g. World War I is back up from 8 to 12-13 seconds.

Comment 4

a year ago
mozreview-review
Comment on attachment 8924380 [details]
Bug 1413072: Eliminate pointless cross-process QueryInterface for IAccessibleText in AccessibleHandler.

https://reviewboard.mozilla.org/r/195660/#review200858

This looks good to me.
Attachment #8924380 - Flags: review?(mzehe) → review+
Note: The reason I was seeing bad perf on the try ZIP was because it wasn't using the handler. I manually registered it and found that it is snappier than current Nightly especially on small pages, not so much a perf difference on big ones. I believe this is a good and safe optimization also for 57.

Comment 6

a year ago
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1116fde20e3b
Eliminate pointless cross-process QueryInterface for IAccessibleText in AccessibleHandler. r=MarcoZ

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1116fde20e3b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 8

a year ago
Comment on attachment 8924380 [details]
Bug 1413072: Eliminate pointless cross-process QueryInterface for IAccessibleText in AccessibleHandler.

Approval Request Comment
[Feature/Bug causing the regression]: E10s Windows accessibility.
[User impact if declined]: Decreased performance.
[Is this code covered by automated tests?]: No, because we don't have a mechanism for platform accessibility testing.
[Has the fix been verified in Nightly?]: I confirmed that pages render as expected with equivalent or better performance.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Simple patch containing a safe optimisation. Easy to verify that breakage has not occurred.
[String changes made/needed]: None.
Attachment #8924380 - Flags: approval-mozilla-beta?
status-firefox57: --- → affected
Comment on attachment 8924380 [details]
Bug 1413072: Eliminate pointless cross-process QueryInterface for IAccessibleText in AccessibleHandler.

Jim weighed in on this uplift and we don't believe this is a must fix for 57. I'd prefer to let this one ride the 58 train.
Attachment #8924380 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox57: affected → wontfix
You need to log in before you can comment on or make changes to this bug.