1417327 - [e10s a11y] More handler caching

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

There are methods that will very probably be called if the interface exists. For example, if IAccessibleAction is available, a client will very probably ask for the number of actions. If IAccessibleTableCell is available, a client will very probably ask for row and column coordinates. Thus, it makes sense to cache these.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1417327 part 1: Accessible handler: Cache IAccessibleAction::nActions.

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

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 8928423 [details]
Bug 1417327 part 1: Accessible handler: Cache IAccessibleAction::nActions.

https://reviewboard.mozilla.org/r/199662/#review204750

Comment 3

[James Teh [:Jamie]]

Comment on attachment 8928423 [details]
Bug 1417327 part 1: Accessible handler: Cache IAccessibleAction::nActions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199662/diff/1-2/

Comment 4

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #3)
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/199662/diff/1-2/

Marco, this just added a couple of comments to the idl stating which interfaces these properties come from, since we'll be adding more of them.

Comment 5

[James Teh [:Jamie]]

Comment on attachment 8928423 [details]
Bug 1417327 part 1: Accessible handler: Cache IAccessibleAction::nActions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199662/diff/2-3/

Comment 6

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #5)
> Comment on attachment 8928423 [details]
> Bug 1417327 part 2: Accessible handler: Cache IAccessibleTableCell
> row/column indexes/extents.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/199662/diff/2-3/

Urgh. That should have been a separate patch, not an update to the first one. I don't understand why mozreview did that; it has a different mozreview commit id.

Comment 7

[James Teh [:Jamie]]

Comment on attachment 8928423 [details]
Bug 1417327 part 1: Accessible handler: Cache IAccessibleAction::nActions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199662/diff/3-4/

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1417327 part 2: Accessible handler: Cache IAccessibleTableCell row/column indexes/extents.

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

Comment 9

[James Teh [:Jamie]]

Sorry about the noise, Marco. All fixed now. The practical upshot is that there is now a part 2 for you to review. Part 1 is the same as yesterday except for some comments I added to the idl.

Note to self: Don't explicitly push subsequent parts to mozreview by themselves; you must also include the previous parts, even though you've already pushed those.

Comment 10

[Marco Zehe (:MarcoZ)]

Comment on attachment 8928839 [details]
Bug 1417327 part 2: Accessible handler: Cache IAccessibleTableCell row/column indexes/extents.

https://reviewboard.mozilla.org/r/200122/#review205266

r=me, thanks! And the TableCell interface did not make my head in this time. ;)

Comment 11

[James Teh [:Jamie]]

Attachment: Bug 1417327 part 3: Accessible handler: Fix cache for IAccessible::accDefaultAction and use it for IAccessibleAction::name(0).

1. Bug 1363595 added support for retrieving accDefaultAction from the cache, but the value was never cached in the first place.
This would have meant that accDefaultAction was returning nothing to clients.

2. Since accDefaultAction is the name of the first action, we can also use this cached value for IAccessibleAction::name for index 0.

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

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 8928870 [details]
Bug 1417327 part 3: Accessible handler: Fix cache for IAccessible::accDefaultAction and use it for IAccessibleAction::name(0).

https://reviewboard.mozilla.org/r/200156/#review205308

r=me with question answered/fixed.

::: accessible/ipc/win/handler/AccessibleHandler.cpp:1298
(Diff revision 1)
> +      GET_BSTR(mDefaultAction, *name);
> +      return S_OK;
> +    }
> +  }
> +
> +  // At this point, there's either no payload or actionIndex is > 1.

Nit: Shouldn't this say > 0, since the index is 0-based? Just to avoid confusion.

Comment 13

[James Teh [:Jamie]]

Comment on attachment 8928870 [details]
Bug 1417327 part 3: Accessible handler: Fix cache for IAccessible::accDefaultAction and use it for IAccessibleAction::name(0).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200156/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7bbf8608707
part 1: Accessible handler: Cache IAccessibleAction::nActions. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/d660faaf09a2
part 2: Accessible handler: Cache IAccessibleTableCell row/column indexes/extents. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/f9111cd04f8c
part 3: Accessible handler: Fix cache for IAccessible::accDefaultAction and use it for IAccessibleAction::name(0). r=MarcoZ

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/d7bbf8608707
https://hg.mozilla.org/mozilla-central/rev/d660faaf09a2
https://hg.mozilla.org/mozilla-central/rev/f9111cd04f8c

Comment 16

[Marco Zehe (:MarcoZ)]

Verified in build 59.0a1 (20171122103138).

Comment 17

[James Teh [:Jamie]]

Comment on attachment 8928423 [details]
Bug 1417327 part 1: Accessible handler: Cache IAccessibleAction::nActions.

Approval Request Comment
[Feature/Bug causing the regression]: Windows e10s accessibility.
[User impact if declined]: Continued poor performance of Windows accessibility.
[Is this code covered by automated tests?]: No, because there is no mechanism for platform accessibility testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1413287, bug 1416986.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Straightforward patch. Only affects accessibility. Accessibility performance is unusable for many use cases in 57, so we can't make things much worse.
[String changes made/needed]: None.

Comment 18

[James Teh [:Jamie]]

Comment on attachment 8928839 [details]
Bug 1417327 part 2: Accessible handler: Cache IAccessibleTableCell row/column indexes/extents.

Approval Request Comment
[Feature/Bug causing the regression]: Windows e10s accessibility.
[User impact if declined]: Continued poor performance of Windows accessibility.
[Is this code covered by automated tests?]: No, because there is no mechanism for platform accessibility testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1413287, bug 1416986, other patches for this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Straightforward patch. Only affects accessibility. Accessibility performance is unusable for many use cases in 57, so we can't make things much worse.
[String changes made/needed]: None.

Comment 19

[James Teh [:Jamie]]

Comment on attachment 8928870 [details]
Bug 1417327 part 3: Accessible handler: Fix cache for IAccessible::accDefaultAction and use it for IAccessibleAction::name(0).

Approval Request Comment
[Feature/Bug causing the regression]: Windows e10s accessibility.
[User impact if declined]: Continued poor performance of Windows accessibility.
[Is this code covered by automated tests?]: No, because there is no mechanism for platform accessibility testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1413287, bug 1416986, other patches from this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Straightforward patch. Only affects accessibility. Accessibility performance is unusable for many use cases in 57, so we can't make things much worse.
[String changes made/needed]: None.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8928423 [details]
Bug 1417327 part 1: Accessible handler: Cache IAccessibleAction::nActions.

To improve the performance of Windows accessibility. Beta58+.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/403ec09d449e
https://hg.mozilla.org/releases/mozilla-beta/rev/5b467b5636ef
https://hg.mozilla.org/releases/mozilla-beta/rev/ad61044a4ba8