Closed Bug 1413072 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
All
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: Jamie, Assigned: Jamie)

Details

Attachments

(1 file)

Bug 1413072: Eliminate pointless cross-process QueryInterface for IAccessibleText in AccessibleHandler.
59 bytes, text/x-review-board-request
MarcoZ
: review+
ritu
: approval-mozilla-beta-
Details 
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: nobody → jteh
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.
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1116fde20e3b
Eliminate pointless cross-process QueryInterface for IAccessibleText in AccessibleHandler. r=MarcoZ
https://hg.mozilla.org/mozilla-central/rev/1116fde20e3b
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.