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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

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.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1413072: Eliminate pointless cross-process QueryInterface for IAccessibleText in AccessibleHandler.

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.
Therefore, we should just query for and use IAccessibleHypertext for all of these methods, thus saving a cross-process call should a client want both interfaces.

Review commit: https://reviewboard.mozilla.org/r/195660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195660/

Comment 2

[James Teh [:Jamie]]

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.

Comment 3

[Marco Zehe (:MarcoZ)]

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

[Marco Zehe (:MarcoZ)]

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.

Comment 5

[Marco Zehe (:MarcoZ)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1116fde20e3b
Eliminate pointless cross-process QueryInterface for IAccessibleText in AccessibleHandler. r=MarcoZ

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1116fde20e3b

Comment 8

[James Teh [:Jamie]]

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.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.