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

RESOLVED FIXED in Firefox 58

Status

()

defect
RESOLVED FIXED
2 years ago
2 years 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

2 years 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

2 years ago
Assignee: nobody → jteh
Comment hidden (mozreview-request)
Assignee

Comment 2

2 years 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 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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1116fde20e3b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee

Comment 8

2 years 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?
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-
You need to log in before you can comment on or make changes to this bug.