[Embedder API] Introduce new semantics update callback and migrate Windows

[loic-sharma]

This introduces a new embedder API to notify the embedder of semantic updates, and, migrates Windows to this new embedder API. The Linux and macOS migrations will be done in follow-up changes.

Part of flutter/flutter#114201

Background

In the old embedder API, the engine breaks up a batch of semantic updates and sends each update individually to the embedder. This is problematic as there may be dependencies across these updates, so the ordering of these updates is crucial to prevent crashes (see flutter/flutter#103808 and #35792). In other words, there was an implicit contract between the embedder and the engine on the ordering of these updates.

The new embedder API just sends the entire batch of semantic updates in one go, allowing us to remove ordering concerns from the embedder API.

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.

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

[loic-sharma]

This diff is a bit confusing. The contents of the old A11TreeIsConsistent was moved to a new test A11yTreeIsConsistentUsingLegacyCallbacks, which starts below on line 215. This A11TreeIsConsistent now tests the new embedder API.

[yaakovschectman]

LGTM, but I just want to ask if there is any particular reason to have a method which returns the callback function instead of storing the sub-callbacks as members like before and calling them all from a callback method instead?

[loic-sharma]

EDIT: After mulling this over, the two layers of "translation" callbacks adds too much cognitive overhead. I will simplify this to just a single layer of "translation" callbacks.

Original reply...

@yaakovschectman The high-level flow hasn't changed, here's how registering the embedder's callbacks work before and after this change:

1. Embedder provides callbacks in FlutterProjectArgs
2. Engine wraps the embedder's callbacks in a PlatformViewEmbedder::UpdateSemanticsCallback. This wrapper callback translates engine updates to embedder updates
3. Engine passes this "translation" callback to PlatformViewEmbedder

... there is any particular reason to have a method which returns the callback function

After this change, there are now two layers of "translation" callbacks:

1. Translate engine updates to embedder updates
2. Translate embedder updates to the legacy embedder callbacks, if needed

The CreateEmbedderSemanticsUpdateCallback creates that second "translation" callback

... instead of storing the sub-callbacks as members like before and calling them all from a callback method instead

This still works the same way as before: the embedder provides its callbacks using FlutterProjectArgs, and these embedder callbacks are captured by the "translation" callback that is passed to PlatformViewEmbedder. Here is where the embedder callbacks were captured before.

Please let me know if I misunderstood something! Also, please let me know if you have suggestions on how to improve clarity!

[loic-sharma]

@yaakovschectman I simplified this change, please take another look! Below is the updated version of this comment.

The high-level flow for the callbacks hasn't changed, here's it works for both before and after this change:

1. Embedder provides callbacks in FlutterProjectArgs
2. Engine wraps the embedder's callbacks in a PlatformViewEmbedder::UpdateSemanticsCallback. This wrapper callback translates engine updates to embedder updates
3. Engine passes this "translation" callback to PlatformViewEmbedder

... there is any particular reason to have a method which returns the callback function

After this change, there are now three possible "translation" callbacks:

1. Translate to the 'new' embedder callback update_semantics_callback
2. Translate to the 'old' embedder callbacks update_semantics_node_callback and update_semantics_custom_action_callback
3. Do nothing if the embedder registered no callbacks

The CreateEmbedderSemanticsUpdateCallback method picks which "translation" callback should be registered to the PlatformViewEmbedder.

... instead of storing the sub-callbacks as members like before and calling them all from a callback method instead

This still works the same way as before: the embedder provides its callbacks using FlutterProjectArgs, and these embedder callbacks are captured by the "translation" callback that is passed to PlatformViewEmbedder. Here is where the embedder callbacks were captured before.

Please let me know if I misunderstood something! Also, please let me know if you have suggestions on how to improve clarity!

[a-wallen]

LGTM

[chunhtai]

LGTM, do you have a pr to migrate the macOS as well?

[loic-sharma]

@chunhtai Not yet, I was waiting to in case the embedder API received feedback. The macOS changes will be very similar to the Windows changes.

EDIT: Here's a prototype of the macOS changes: loic-sharma@5ddcc8a

[cbracken]

Can these be removed now that we have the alternative? It doesn't look like we ever actually use them in tests (or maybe I'm mis-grepping).

[loic-sharma]

These are still used by the A11yTreeIsConsistentUsingLegacyCallbacks unit test to test the legacy callbacks (see the context.SetSemanticsNodeCallback and context.SetSemanticsCustomActionCallback calls)

[cbracken]

[cbracken]

/cc @chinmaygarde

[flar]

The unit test here is consistently exceeding the 120s timeouts in runs: https://ci.chromium.org/p/flutter/builders/try/Mac%20Host%20Engine

Starting a revert...

[flar]

There is also a potential fix for the race conditions: #37488

[flar]

I closed the revert in favor of the fix - #37488

[yaakovschectman]

Just noticed this accidentally removed the tooltip from the embedder callback. Will need to add that back in #37676