Merge AccessibilityBridge and AccessibilityBridgeDelegate

[dkwingsmt]

This PR merges AccessibilityBridgeDelegate into AccessibilityBridge, and isolates accessibility bridge's delegates and node delegates from embedding engines.

This is still a draft. TODO:

• [x] Isolate FlutterPlatformNodeDelegateMac from the engine
• [ ] Apply the same change to the Windows embedding
• [ ] Documentations change

Why is this needed?

As an alternative approach to https://github.com/flutter/engine/pull/36573, this PR tries to isolate accessibility bridge's delegates and node delegates from embedding engines, so that they will use received views from the construcot argument instead of querying engine.viewController. Different from https://github.com/flutter/engine/pull/36573, this PR merges the two classes so that there is no longer a circular referencing problem.

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 https://github.com/flutter/engine/pull/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.

[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, https://github.com/flutter/engine/pull/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.

[auto-submit[bot]]

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[bot]]

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[bot]]

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.