Closed Bug 1417327 Opened 7 years ago Closed 7 years ago

[e10s a11y] More handler caching

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:
mozilla59
Tracking Flags:
Tracking Status
firefox58 --- fixed
firefox59 --- verified

People

(Reporter: Jamie, Assigned: Jamie)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1417327 part 1: Accessible handler: Cache IAccessibleAction::nActions.
59 bytes, text/x-review-board-request
MarcoZ
: review+
gchang
: approval-mozilla-beta+
Details
Bug 1417327 part 2: Accessible handler: Cache IAccessibleTableCell row/column indexes/extents.
59 bytes, text/x-review-board-request
MarcoZ
: review+
gchang
: approval-mozilla-beta+
Details
Bug 1417327 part 3: Accessible handler: Fix cache for IAccessible::accDefaultAction and use it for IAccessibleAction::name(0).
59 bytes, text/x-review-board-request
MarcoZ
: review+
gchang
: approval-mozilla-beta+
Details 
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.
Depends on: 1416986
Comment on attachment 8928423 [details]
Bug 1417327 part 1: Accessible handler: Cache IAccessibleAction::nActions.

https://reviewboard.mozilla.org/r/199662/#review204750
Attachment #8928423 - Flags: review?(mzehe) → review+
(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.
(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.
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 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. ;)
Attachment #8928839 - Flags: review?(mzehe) → review+
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.
Attachment #8928870 - Flags: review?(mzehe) → review+
Blocks: 1419362
Pushed by mzehe@mozilla.com:
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
Verified in build 59.0a1 (20171122103138).
status-firefox58: --- → affected
status-firefox59: fixed → verified
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.
Attachment #8928423 - Flags: approval-mozilla-beta?
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.
Attachment #8928839 - Flags: approval-mozilla-beta?
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.
Attachment #8928870 - Flags: approval-mozilla-beta?
Comment on attachment 8928423 [details]
Bug 1417327 part 1: Accessible handler: Cache IAccessibleAction::nActions.

To improve the performance of Windows accessibility. Beta58+.
Attachment #8928423 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8928839 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8928870 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: