Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Remove deprecated calls to updateSemantics in PlatformDispatcher #36673

Merged
merged 5 commits into from
Oct 28, 2022
Merged

Remove deprecated calls to updateSemantics in PlatformDispatcher #36673

merged 5 commits into from
Oct 28, 2022

Conversation

a-wallen
Copy link
Contributor

@a-wallen a-wallen commented Oct 8, 2022 •
edited
Loading

Removes deprecated updateSemantics APIs from PlatformDispatcher.

Fixes flutter/flutter#112221

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added embedder Related to the embedder API platform-web Code specifically for the web engine labels Oct 8, 2022
@skia-gold
Copy link

Gold has detected about 2 new digest(s) on patchset 2.
View them at https://flutter-engine-gold.skia.org/cl/github/36673

@a-wallen a-wallen changed the title Deprecate platform_dispatcher apis in favor of updateSemantics in F… Remove deprecated calls to updateSemantics in PlatformDispatcher Oct 8, 2022
@a-wallen a-wallen force-pushed the remove_deprecated_update_semantics branch 4 times, most recently from 1994fb4 to 0dbe0e6 Compare October 26, 2022 21:40
@a-wallen a-wallen marked this pull request as ready for review October 26, 2022 22:53
@a-wallen a-wallen requested a review from goderbauer October 26, 2022 22:53
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

goderbauer
goderbauer reviewed Oct 28, 2022
View reviewed changes
lib/web_ui/lib/window.dart Outdated
Comment on lines 134 to 136
@override
void updateSemantics(SemanticsUpdate update) => platformDispatcher.updateSemantics(update, this);

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this exactly the implementation it inherits from FlutterView?

lib/web_ui/lib/src/engine/platform_dispatcher.dart Outdated
call `updateSemantics`.
''')
void updateSemantics(ui.SemanticsUpdate update) {
void updateSemantics(ui.SemanticsUpdate update, [ui.FlutterView? view]) {
Copy link
Member

Choose a reason for hiding this comment

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

In the base class, it was typed FlutterView instead of FlutterView? - why the difference?

lib/web_ui/lib/src/engine/platform_dispatcher.dart Outdated
call `updateSemantics`.
''')
void updateSemantics(ui.SemanticsUpdate update) {
void updateSemantics(ui.SemanticsUpdate update, [ui.FlutterView? view]) {
Copy link
Member

Choose a reason for hiding this comment

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

Also, why do we add the view parameter here? It appears to be unused? In the future (once the web engine is actually multi-view enabled) we will likely have to add it, but since EnginePlatformDispatcher is private API we can just do that when we need it without a breaking change?

@a-wallen a-wallen force-pushed the remove_deprecated_update_semantics branch from 0dbe0e6 to 6f7c6db Compare October 28, 2022 18:44
@a-wallen a-wallen force-pushed the remove_deprecated_update_semantics branch from 6f7c6db to b75e299 Compare October 28, 2022 18:46
@a-wallen
Copy link
Contributor Author

This PR should be test exempt now that it only removes code.

goderbauer
goderbauer approved these changes Oct 28, 2022
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@a-wallen a-wallen added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2022
@auto-submit auto-submit bot merged commit 4d8f07b into flutter:main Oct 28, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 28, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 28, 2022
@engine-flutter-autoroll
4d8f07b8f Remove deprecated calls to updateSemantics in `PlatformDisp…
39e565a 
…atcher` (flutter/engine#36673)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 28, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 28, 2022
@engine-flutter-autoroll
4d8f07b8f Remove deprecated calls to updateSemantics in `PlatformDisp…
525031c 
…atcher` (flutter/engine#36673)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 29, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 29, 2022
@engine-flutter-autoroll
4d8f07b8f Remove deprecated calls to updateSemantics in `PlatformDisp…
cb60a3b 
…atcher` (flutter/engine#36673)
a-wallen added a commit that referenced this pull request Nov 7, 2022
@a-wallen
Revert "Remove deprecated calls to updateSemantics in `PlatformDispat…
1886358 
…cher` (#36673)"

This reverts commit 4d8f07b.
@a-wallen a-wallen mentioned this pull request Nov 7, 2022
Merged
auto-submit bot pushed a commit that referenced this pull request Nov 7, 2022
@a-wallen
Revert "Remove deprecated calls to updateSemantics in `PlatformDispat…
a81a715 
…cher` (#36673)" (#37388)

This reverts commit 4d8f07b.
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Nov 9, 2022
@a-wallen
Revert "Remove deprecated calls to updateSemantics in `PlatformDispat…
d15483c 
…cher` (flutter#36673)" (flutter#37388)

This reverts commit 4d8f07b.
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
@a-wallen @schwa423
Remove deprecated calls to updateSemantics in PlatformDispatcher (f…
fd06a7c 
…lutter#36673)
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
@a-wallen @schwa423
Revert "Remove deprecated calls to updateSemantics in `PlatformDispat…
a3cba1b 
…cher` (flutter#36673)" (flutter#37388)

This reverts commit 4d8f07b.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@goderbauer goderbauer goderbauer approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App embedder Related to the embedder API needs tests platform-web Code specifically for the web engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move updateSemantics from PlatformDispatcher to FlutterView
3 participants
@a-wallen @skia-gold @goderbauer