Merge AccessibilityBridge and AccessibilityBridgeDelegate

[dkwingsmt]

This PR merges AccessibilityBridgeDelegate into AccessibilityBridge, affecting common, mac, and windows. As a result, accessibility bridges and node delegates no longer call the engine for the bridge or the view, but get them as constructor arguments. AccessibilityBridge classes still refer to the engine, but only to dispatch events to embedder APIs.

Part of the multiwindow project (design doc for macOS): This reduces the usage of the following single-view APIs:

• mac/FlutterEngine#accessibilityBridge
• mac/FlutterEngine#viewController
• windows/FlutterWindowsEngine::accessibilityBridge
• windows/FlutterWindowsEngine::viewController

TODO:

• Isolate FlutterPlatformNodeDelegateMac from the engine
• Apply the same change to the Windows embedding
• Documentations change
• Update design doc (internal doc)

Why do we choose this method?

In the current HEAD:

• The bridge must be initialized with a delegate and owns it.
• The delegate is initialized with the engine.
• The delegate also needs to use the bridge, however, not by directly referring to the bridge, but by calling flutter_engine_.accessibilityBridge.

The ultimate goal of the change is get rid of the usage of FlutterEngine#accessibilityBridge and FlutterEngine#viewController. Instead, the delegate will be provided a bridge and a view controller at initialization. In this way, the engine can initialize multiple sets of bridge-and-delegates, each for a window.

However, if we still keep the bridge and the delegate separated, the bridge must be created with a delegate, and a delegate must be created with a bridge. To resolve this circular dependency, we might either assign the delegate with the bridge later, or assign the bridge with the delegate later. Neither way is error-prone enough though: we can not guarantee at compile time that both classes have all dependents ready when other methods are called.

But since both the bridge and the delegate correspond to one window, there's no reason to separate them. If we merge the two classes, then their communication becomes intra-class.

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.

[dkwingsmt]

@chunhtai This PR is an alternative proposal to from #36573 by merging AccessibilityBridgeDelegate into AccessibilityBridge so that there are no circular referencing problem.

I haven't implemented everything yet (see PR body for TODOs), however the current state should be a sufficient POC that the "merging" strategy works (the mac unittests pass). Let me know if you think it's worth moving forward.

[chunhtai]

I have a bit concern about using inheritence vs composition which may add more complexity if embedding starts overriding existing default method. but I think this is probably a better way to approach the dependency cycle problem. Should probably loop in @cbracken who wrote the code to wire up bridge in windows embedding.

[dkwingsmt]

Can you illustrate what you mean by

using inheritence vs composition which may add more complexity if embedding starts overriding existing default method

I don't think I'm adding extra inheritance, since the current engine also needs to inherit the delegate node.

About overriding existing default method, the FlutterEngine's new method is just for unit testing.

[chunhtai]

Can you illustrate what you mean by

using inheritence vs composition which may add more complexity if embedding starts overriding existing default method

I meant it used to be composition (the bridge compose bridge delegate to call native API), but it becomes inheritance now. which may be a bit hard to maintain if embedding starts overriding method like AddFlutterSemanticsNodeUpdate or CommitUpdates to inject their own call loop. It is not a strong opinion to not to merge delegate and bridge though.

#Edit: you can prevent override by using final keyword. so probably not too big of a deal

[cbracken]

@dkwingsmt can you elaborate a bit (in the description) on the circular reference issue you mention in the comments above?

I'm also a bit on the fence in terms of inheritance vs composition. Agreed that we're effectively trading subclassing one class rather than another, but curious to understand the circular reference issue you mention above to understand why this form is an improvement.

[dkwingsmt]

@cbracken Sure. (I've added the following text to the PR description.)

The circular dependency can be seen in the previous PR, #36573.

• In the current HEAD:
  • The bridge must be initialized with a delegate and owns it.
  • The delegate is initialized with the engine.
  • The delegate also needs to use the bridge, however, not by directly referring to the bridge, but by calling flutter_engine_.accessibilityBridge.
• The ultimate goal of the change is get rid of the usage of FlutterEngine#accessibilityBridge and FlutterEngine#viewController. Instead, the delegate should use a provided bridge and a provided view controller.
• But in this way, the bridge must be created with a delegate, and a delegate must be created with a bridge. This is the circular dependency.
• In the previous PR, I tried either assigning the delegate with the bridge later, or assigning the bridge with the delegate later. But neither is error-prone enough.
• I discovered that, since both the bridge and the delegate correspond to one window, there is no reason to separate them. Therefore in this PR, I propose to merge the two classes, resolving the circular dependency.

[dkwingsmt]

@cbracken @chunhtai This PR should be ready to review. Although, I might need some help on the documentations, since it feels like I should merge the two classes' the documentation but I don't know enough to do so.

[loic-sharma]

The comment above needs to be updated

[auto-submit]

auto label is removed for flutter/engine, pr: 36597, due to - The status or check suite Windows Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

[auto-submit]

auto label is removed for flutter/engine, pr: 36597, due to - The status or check suite Mac Host clang-tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.

[auto-submit]

auto label is removed for flutter/engine, pr: 36597, due to - The status or check suite Linux linux_arm_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.