New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gecko_ia2 vbuf backend: Support IAccessibleHypertext2 to improve performance for Firefox multi-process. #7719
Conversation
…ormance for Firefox multi-process. IAccessibleHypertext2::hyperlinks allows all embedded objects to be retrieved at once, rather than retrieving them one at a time. This improves performance for cross-process renders, which is the case for Firefox multi-process. IAccessibleHypertext is still supported and will be used if the newer interface is not present. In addition: 1. Don't bother calling hyperlinkIndex, since in Gecko (and Chrome), hyperlinks correspond with embedded object characters. 2. Use a constant for the embedded object character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please move the hyperlink getter classes into common/ia2utils.cpp/h, as we'll want to use them in ia2liveRegions.cpp as well eventually. Other than that this all looks fine to me.
nvdaHelper/common/ia2utils.h
Outdated
}; | ||
|
||
/** | ||
* Create an appropriate HyperlinkGetterto retrieve hyperlinks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HyperlinkGetterto > HyperlinkGetter to
The gecko vbuf (and probably any future callers) only call HyperlinkGetter::next() when they encounter an embedded object character. That is, if there are no embedded object characters, there will be no calls. Thus: 1. For IAccessibleHypertext, we don't need to call nHyperlinks. hyperlink will fail or return null if we pass an invalid index anyway. 2. For IAccessibleHypertext2, we can lazily fetch the hyperlinks on the first call to get(), thus saving a potential cross-process call when there are none.
I ran with both try builds for a day or two now, have been using the second one since it became available, and haven't found anything bad, no missing text or links or such. |
Confirmed with FF Nightly and the try builds.
|
Link to issue number:
None.
Summary of the issue:
With Firefox multi-process, rendering a virtual buffer for browse mode now makes cross-process calls, since the content document accessibility tree is now in another process. This has a substantial impact on performance. The more cross-process calls we can reduce, the better. This PR introduces a change which fetches all embedded children at once, rather than fetching each in a separate call.
Description of how this pull request fixes the issue:
IAccessibleHypertext2::hyperlinks allows all embedded objects to be retrieved at once, rather than retrieving them one at a time.
This improves performance for cross-process renders, which is the case for Firefox multi-process.
IAccessibleHypertext is still supported and will be used if the newer interface is not present.
In addition:
Testing performed:
confirmed that the World War I Wikipedia article renders as expected in:
Using IAHypertext2 seems to shave up to a second off the render time for the World War I article, though it's difficult to provide concrete numbers because the time seems to be a little variable.
Known issues with pull request:
None know.
Change log entry:
In Bug Fixes: