Skip to content

macOS: Compare accessibility strings as NSStrings in tests - #191853

Merged
cbracken merged 1 commit into
flutter:masterfrom
cbracken:eliminate-pointless-str-conversions
Aug 28, 2026
Merged

cbracken merged 1 commit into
flutter:masterfrom
cbracken:eliminate-pointless-str-conversions

Conversation

@cbracken

Copy link
Copy Markdown
Member

We were converting NSString instances to std::strings purely to compare them against C string literals. Comparing the NSString directly is shorter, matches the neighbouring role assertions, and drops five UTF8String conversions.

Round-tripping these through std::string results in any embedded NULs triggering truncation which could, in theory, hide bugs.

Spotted while working on: #191616

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

We were converting `NSString` instances to `std::string`s purely to
compare them against C string literals. Comparing the `NSString`
directly is shorter, matches the neighbouring role assertions, and drops
five `UTF8String` conversions.

Round-tripping these through `std::string` results in any embedded NULs
triggering truncation which could, in theory, hide bugs.

Spotted while working on: flutter#191616
@cbracken
cbracken requested a review from a team as a code owner August 27, 2026 03:47
@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label Aug 27, 2026
@github-actions github-actions Bot added engine flutter/engine related. See also e: labels. platform-macos Building on or for macOS specifically a: desktop Running on desktop team-macos Owned by the macOS platform team labels Aug 27, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors unit tests in FlutterEngineTest.mm and FlutterPlatformNodeDelegateMacTest.mm by replacing C++ std::string conversions and comparisons of Objective-C string properties with direct isEqualToString: comparisons. There are no review comments, and I have no feedback to provide.

pull Bot pushed a commit to Mu-L/flutter that referenced this pull request Aug 28, 2026
…1855)

`FlutterPlatformViewsController` was holding its view factories and
their gesture blocking policies in `std::unordered_map`s keyed by
`std::string`, so both the registration path and the create path
converted an `NSString` to a `std::string` first. But all the mapped
values are Obj-C types so we can migrate to na `NSMutableDictionary`.
This matches how we do it in the macOS embedder.

This removes the last of the `UTF8String` conversions in this file. As
noted in flutter#191853 and flutter#191854 `std::string`'s `const char*` constructor
determines the length using `strlen`, so it truncates at an embedded NUL
and dereferences null if `UTF8String` ever returns NULL, which it does
for a nil receiver.

Spotted while working on:
flutter#191616

No new tests since this just changes the implementation without changing
any semantics.

<!--
Thanks for filing a pull request!
Reviewers are typically assigned within a week of filing a request.
To learn more about code review, see our documentation on Tree Hygiene:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
-->


## Pre-launch Checklist

- [X] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [X] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [X] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [X] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [X] I signed the [CLA].
- [X] I listed at least one issue that this PR fixes in the description
above.
- [X] I updated/added relevant documentation (doc comments with `///`).
- [X] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [X] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [X] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

If this change needs to override an active code freeze, provide a
comment explaining why. The code freeze workflow can be overridden by
code reviewers. See pinned issues for any active code freezes with
guidance.

**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
@cbracken
cbracken added this pull request to the merge queue Aug 28, 2026
pull Bot pushed a commit to TheRakeshPurohit/flutter that referenced this pull request Aug 28, 2026
…er#191854)

`AccessibilityBridgeMacSpy` was recording the notifications it saw in a
`std::unordered_map<std::string, gfx::NativeViewAccessible>`, converting
the `NSAccessibilityNotificationName` key to a `std::string` on the way
in and back again on the way out.

Both the key and value are Obj-C objects, so we can use an
`NSMutableDictionary` and avoid all the conversions. Similar to flutter#191853,
`std::string` conversions use `strlen` to determine the length, which
truncates at any embedded NUL chars, potentially hiding bugs.

Spotted while working on:
flutter#191616

<!--
Thanks for filing a pull request!
Reviewers are typically assigned within a week of filing a request.
To learn more about code review, see our documentation on Tree Hygiene:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
-->


## Pre-launch Checklist

- [X] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [X] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [X] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [X] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [X] I signed the [CLA].
- [X] I listed at least one issue that this PR fixes in the description
above.
- [X] I updated/added relevant documentation (doc comments with `///`).
- [X] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [X] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [X] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

If this change needs to override an active code freeze, provide a
comment explaining why. The code freeze workflow can be overridden by
code reviewers. See pinned issues for any active code freezes with
guidance.

**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Merged via the queue into flutter:master with commit c7d2345 Aug 28, 2026
23 checks passed
@cbracken
cbracken deleted the eliminate-pointless-str-conversions branch August 28, 2026 03:01
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Aug 28, 2026
flutter/flutter@e8dca90...8d8c1aa

2026-08-28 [email protected] Roll Packages from bd3cbc1 to cd4cdd0 (15 revisions) (flutter/flutter#191965)
2026-08-28 [email protected] Bump customer testing version to pick up forui tests (flutter/flutter#191931)
2026-08-28 [email protected] Use leading scroll offset instead of infinite offset to reveal pinned slivers (flutter/flutter#191329)
2026-08-28 [email protected] Roll Skia from 21f71051e749 to fe0b9a5261f9 (1 revision) (flutter/flutter#191960)
2026-08-28 [email protected] Share the common Linux window controller code in mixins (flutter/flutter#191930)
2026-08-28 [email protected] Roll Dart SDK from 2cf2f69fd9f0 to ae63a6eca3f1 (1 revision) (flutter/flutter#191953)
2026-08-28 [email protected] Roll Skia from 11e3a7cb0fd0 to 21f71051e749 (3 revisions) (flutter/flutter#191942)
2026-08-28 [email protected] Roll Dart SDK from e030e77d5413 to 2cf2f69fd9f0 (2 revisions) (flutter/flutter#191934)
2026-08-28 [email protected] Remove no-shuffle in flutter tools overall experience test (flutter/flutter#191861)
2026-08-28 [email protected] [flutter_tools] Gracefully handle Windows Application Control and security policy execution blocks (flutter/flutter#191912)
2026-08-28 [email protected] [tool] Migrate ConfigCommand to modular dependency injection (flutter/flutter#190743)
2026-08-28 [email protected] Roll Skia from 37459da0a755 to 11e3a7cb0fd0 (9 revisions) (flutter/flutter#191929)
2026-08-28 [email protected] Connect to moved-to-rect after the window is realized (flutter/flutter#191773)
2026-08-28 [email protected] macOS: Compare accessibility strings as NSStrings in tests (flutter/flutter#191853)
2026-08-28 [email protected] macOS: Record test accessibility notifications in a dictionary (flutter/flutter#191854)
2026-08-28 [email protected] Roll Fuchsia Test Scripts from 6SXRz6wsA2Zm_rAqP... to u_fSMsPU22VvUsvSo... (flutter/flutter#191927)
2026-08-28 [email protected] Roll Dart SDK from cb4c39d067f3 to e030e77d5413 (3 revisions) (flutter/flutter#191923)
2026-08-27 [email protected] Migrate ios driver tests batch 2 to simulators (flutter/flutter#190930)
2026-08-27 [email protected] [flutter_test] Ensure LiveTestWidgetsFlutterBinding executes frame and pointer callbacks in the test zone (flutter/flutter#191595)
2026-08-27 [email protected] Deprecate legacy gen_defaults tool (flutter/flutter#191911)
2026-08-27 [email protected] Fix black window in the subsurface view renderer (flutter/flutter#191850)
2026-08-27 [email protected] Roll Skia from 82e972ea84b1 to 37459da0a755 (10 revisions) (flutter/flutter#191906)
2026-08-27 [email protected] iOS: Key the platform view factory registries by NSString (flutter/flutter#191855)
2026-08-27 [email protected] Sync CHANGELOG.md from stable (flutter/flutter#191895)
2026-08-27 [email protected] [flutter_tools] Honor configured build-dir across flutter project and build targets (flutter/flutter#191592)
2026-08-27 [email protected] Revert: Handle linear and radial gradients in UberSDF (flutter/flutter#191913)
2026-08-27 [email protected] Explicitly disable HCPP in hybrid_android_views integration test (flutter/flutter#191894)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
victorsanni pushed a commit to victorsanni/packages that referenced this pull request Sep 9, 2026
…r#12679)

flutter/flutter@e8dca90...8d8c1aa

2026-08-28 [email protected] Roll Packages from bd3cbc1 to cd4cdd0 (15 revisions) (flutter/flutter#191965)
2026-08-28 [email protected] Bump customer testing version to pick up forui tests (flutter/flutter#191931)
2026-08-28 [email protected] Use leading scroll offset instead of infinite offset to reveal pinned slivers (flutter/flutter#191329)
2026-08-28 [email protected] Roll Skia from 21f71051e749 to fe0b9a5261f9 (1 revision) (flutter/flutter#191960)
2026-08-28 [email protected] Share the common Linux window controller code in mixins (flutter/flutter#191930)
2026-08-28 [email protected] Roll Dart SDK from 2cf2f69fd9f0 to ae63a6eca3f1 (1 revision) (flutter/flutter#191953)
2026-08-28 [email protected] Roll Skia from 11e3a7cb0fd0 to 21f71051e749 (3 revisions) (flutter/flutter#191942)
2026-08-28 [email protected] Roll Dart SDK from e030e77d5413 to 2cf2f69fd9f0 (2 revisions) (flutter/flutter#191934)
2026-08-28 [email protected] Remove no-shuffle in flutter tools overall experience test (flutter/flutter#191861)
2026-08-28 [email protected] [flutter_tools] Gracefully handle Windows Application Control and security policy execution blocks (flutter/flutter#191912)
2026-08-28 [email protected] [tool] Migrate ConfigCommand to modular dependency injection (flutter/flutter#190743)
2026-08-28 [email protected] Roll Skia from 37459da0a755 to 11e3a7cb0fd0 (9 revisions) (flutter/flutter#191929)
2026-08-28 [email protected] Connect to moved-to-rect after the window is realized (flutter/flutter#191773)
2026-08-28 [email protected] macOS: Compare accessibility strings as NSStrings in tests (flutter/flutter#191853)
2026-08-28 [email protected] macOS: Record test accessibility notifications in a dictionary (flutter/flutter#191854)
2026-08-28 [email protected] Roll Fuchsia Test Scripts from 6SXRz6wsA2Zm_rAqP... to u_fSMsPU22VvUsvSo... (flutter/flutter#191927)
2026-08-28 [email protected] Roll Dart SDK from cb4c39d067f3 to e030e77d5413 (3 revisions) (flutter/flutter#191923)
2026-08-27 [email protected] Migrate ios driver tests batch 2 to simulators (flutter/flutter#190930)
2026-08-27 [email protected] [flutter_test] Ensure LiveTestWidgetsFlutterBinding executes frame and pointer callbacks in the test zone (flutter/flutter#191595)
2026-08-27 [email protected] Deprecate legacy gen_defaults tool (flutter/flutter#191911)
2026-08-27 [email protected] Fix black window in the subsurface view renderer (flutter/flutter#191850)
2026-08-27 [email protected] Roll Skia from 82e972ea84b1 to 37459da0a755 (10 revisions) (flutter/flutter#191906)
2026-08-27 [email protected] iOS: Key the platform view factory registries by NSString (flutter/flutter#191855)
2026-08-27 [email protected] Sync CHANGELOG.md from stable (flutter/flutter#191895)
2026-08-27 [email protected] [flutter_tools] Honor configured build-dir across flutter project and build targets (flutter/flutter#191592)
2026-08-27 [email protected] Revert: Handle linear and radial gradients in UberSDF (flutter/flutter#191913)
2026-08-27 [email protected] Explicitly disable HCPP in hybrid_android_views integration test (flutter/flutter#191894)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
victorsanni pushed a commit to victorsanni/packages that referenced this pull request Sep 9, 2026
…r#12679)

flutter/flutter@e8dca90...8d8c1aa

2026-08-28 [email protected] Roll Packages from bd3cbc1 to cd4cdd0 (15 revisions) (flutter/flutter#191965)
2026-08-28 [email protected] Bump customer testing version to pick up forui tests (flutter/flutter#191931)
2026-08-28 [email protected] Use leading scroll offset instead of infinite offset to reveal pinned slivers (flutter/flutter#191329)
2026-08-28 [email protected] Roll Skia from 21f71051e749 to fe0b9a5261f9 (1 revision) (flutter/flutter#191960)
2026-08-28 [email protected] Share the common Linux window controller code in mixins (flutter/flutter#191930)
2026-08-28 [email protected] Roll Dart SDK from 2cf2f69fd9f0 to ae63a6eca3f1 (1 revision) (flutter/flutter#191953)
2026-08-28 [email protected] Roll Skia from 11e3a7cb0fd0 to 21f71051e749 (3 revisions) (flutter/flutter#191942)
2026-08-28 [email protected] Roll Dart SDK from e030e77d5413 to 2cf2f69fd9f0 (2 revisions) (flutter/flutter#191934)
2026-08-28 [email protected] Remove no-shuffle in flutter tools overall experience test (flutter/flutter#191861)
2026-08-28 [email protected] [flutter_tools] Gracefully handle Windows Application Control and security policy execution blocks (flutter/flutter#191912)
2026-08-28 [email protected] [tool] Migrate ConfigCommand to modular dependency injection (flutter/flutter#190743)
2026-08-28 [email protected] Roll Skia from 37459da0a755 to 11e3a7cb0fd0 (9 revisions) (flutter/flutter#191929)
2026-08-28 [email protected] Connect to moved-to-rect after the window is realized (flutter/flutter#191773)
2026-08-28 [email protected] macOS: Compare accessibility strings as NSStrings in tests (flutter/flutter#191853)
2026-08-28 [email protected] macOS: Record test accessibility notifications in a dictionary (flutter/flutter#191854)
2026-08-28 [email protected] Roll Fuchsia Test Scripts from 6SXRz6wsA2Zm_rAqP... to u_fSMsPU22VvUsvSo... (flutter/flutter#191927)
2026-08-28 [email protected] Roll Dart SDK from cb4c39d067f3 to e030e77d5413 (3 revisions) (flutter/flutter#191923)
2026-08-27 [email protected] Migrate ios driver tests batch 2 to simulators (flutter/flutter#190930)
2026-08-27 [email protected] [flutter_test] Ensure LiveTestWidgetsFlutterBinding executes frame and pointer callbacks in the test zone (flutter/flutter#191595)
2026-08-27 [email protected] Deprecate legacy gen_defaults tool (flutter/flutter#191911)
2026-08-27 [email protected] Fix black window in the subsurface view renderer (flutter/flutter#191850)
2026-08-27 [email protected] Roll Skia from 82e972ea84b1 to 37459da0a755 (10 revisions) (flutter/flutter#191906)
2026-08-27 [email protected] iOS: Key the platform view factory registries by NSString (flutter/flutter#191855)
2026-08-27 [email protected] Sync CHANGELOG.md from stable (flutter/flutter#191895)
2026-08-27 [email protected] [flutter_tools] Honor configured build-dir across flutter project and build targets (flutter/flutter#191592)
2026-08-27 [email protected] Revert: Handle linear and radial gradients in UberSDF (flutter/flutter#191913)
2026-08-27 [email protected] Explicitly disable HCPP in hybrid_android_views integration test (flutter/flutter#191894)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: desktop Running on desktop CICD Run CI/CD engine flutter/engine related. See also e: labels. platform-macos Building on or for macOS specifically team-macos Owned by the macOS platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants