Skip to content

Add a helper to getAccessibleChildren#13126

Merged
feerrenrut merged 3 commits into
masterfrom
addHelperForGetAccessibleChildren
Jan 17, 2022
Merged

Add a helper to getAccessibleChildren#13126
feerrenrut merged 3 commits into
masterfrom
addHelperForGetAccessibleChildren

Conversation

@feerrenrut

@feerrenrut feerrenrut commented Dec 3, 2021

Copy link
Copy Markdown
Contributor

Link to issue number:

None
Related to #13106

Summary of the issue:

While working on #13106 I noticed several different approaches to get accessible children.

Description of how this pull request fixes the issue:

Make all usages of AccessibleChildren conform to a consitant approach (with the exception of getIAccessibleText, which will be addressed separately, via #13127)
Resources are now managed with smart pointers.

Testing strategy:

  • Builds and runs locally.
  • Testing from alpha users in the specific applications (webkit, lotus notes, firefox)

Known issues with pull request:

None

Change log entries:

None

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@feerrenrut feerrenrut mentioned this pull request Dec 3, 2021
5 tasks
Make all usages of AccessibleChildren conform to a consitant approach.
Management of resources is automatic.
@feerrenrut feerrenrut force-pushed the addHelperForGetAccessibleChildren branch from 1b675b8 to 2a7e8c1 Compare January 12, 2022 04:13
@feerrenrut feerrenrut marked this pull request as ready for review January 13, 2022 02:00
@feerrenrut feerrenrut requested a review from a team as a code owner January 13, 2022 02:00
@feerrenrut feerrenrut requested a review from seanbudd January 13, 2022 02:00
Comment thread nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp
Comment thread nvdaHelper/common/ia2utils.cpp Outdated
@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

LOG_DEBUG(L"Error in calling fillVBuf");
childPacc->Release();
VariantClear(&(varChildren[i]));
child.Clear();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the calls to child.clear are not necessary I think, as CComVariant's destructor does this itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, explicitly calling clear isn't necessary. I left them in to maintain the existing timing of the release. As much as possible I was trying to avoid the output binary from changing. I can look at this again if you would like?

@michaelDCurran michaelDCurran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A part from possibly removing the calls to CComVariant.clear (as the destructor already does this), This all looks good to me.

@feerrenrut

Copy link
Copy Markdown
Contributor Author

Since the open question doesn't seem to be a matter of correctness, and this is blocking the subsequent change (#13127), I'd like to merge this now.

I'm happy to follow up to remove the calls to child.Clear(); in a subsequent PR if we decide to go that way.

@feerrenrut feerrenrut merged commit c6a6df8 into master Jan 17, 2022
@feerrenrut feerrenrut deleted the addHelperForGetAccessibleChildren branch January 17, 2022 08:15
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants