[webview_flutter_wkwebview] Implement one callback method for review of the design

[bparrishMines]

1. Implementation of WKNavigationDelegate.didFinishNavigation. This changes the callback to be required at instantiation.
2. The FlutterApi for NSObject was also created to show how inheritance will be handled.

Since Objective-C uses reference counting, extra steps had to be taken to avoid circular references. In Objective-C, the addition of the FlutterApi lead to a circular reference as shown in the diagram below. S arrows means an instance holds a strong reference to the instance it points to. W means the equivalent, but with a weak reference.

NavigationDelegate--S-->FlutterApi--S-->BinaryMessenger--S-->HostApi--W-->InstanceManager
^-----------------------------------------------S------------------------------------------/

NavigationDelegate--S-->FlutterApi--W-->InstanceManager
^------------------------S-------------------------------/

As shown above, HostApi with classes containing callback methods will hold only a weak reference to InstanceManager to break the circular reference.

I included a test to ensure the circular reference is broken and verified the circular reference exists if HostApi had a strong reference to InstanceManager.

Unlike Android, the iOS engine doesn't actually keep a direct reference to any plugins that are registered. So, a strong reference InstanceManager can be stored in an empty MethodChannel implementation passed to the registrar.

No version change:
Part of flutter/flutter#93732 and doesn't make any changes to the current implementation.

No CHANGELOG change: Incremental unused code doesn't need to be noted in the CHANGELOG.

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 relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

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

[bparrishMines]

Note: flutter/flutter#103644

[stuartmorgan]

I continue to be concerned about how much complexity and boilerplate there is for direct wrappers 😐

I know the main test will be what feature addition PRs actually look like (in theory, a lot simpler!), but I do wonder how error-prone updating the wrapper for WKWebView changes will end up being.

[stuartmorgan]

Why is this logging instead of sending the error somewhere?

[bparrishMines]

I wasn't sure what else to do with the error. This occurs when the Dart code fails to handle the callback method or the FlutterBinaryMessenger fails to send the message. Typically, most plugins have just been eating these kind of errors, but I assumed it would be better to at least print it out. A possible alternative would be to have a general FlutterApi that waits for errors received by the host api? However, that could also return an error and then we are in the same situation.

[stuartmorgan]

Having to do object tracking for every callback definitely makes the subclass-and-override approach to delegates more appealing. But if finalizer behavior makes that impractical, I guess we're stuck with this.

[bparrishMines]

I definitely agree. So I looked for another solution and the comment you made about copyWith in the doc gave me a new idea that wouldn't require tracking any callback methods.

It keeps the same concept of all classes being stored as weak references in the Dart InstanceManager and strong references in the InstanceManager of the native language.

But instead of storing the Dart functions as strong references in the Dart InstanceManager, a copy of the Dart class is stored as a strong reference. The copied instance would keep the functions passed in the original instance reachable by the Flutter api.

And on the native side, the instance is also stored as a weak reference rather than each function.

This pattern would still allow

1. Dart weak reference to release the native strong reference.
2. The native weak reference would release the copy of the the Dart weak reference that is stored as a strong reference.

[bparrishMines]

The copy method should also be made as a library private method. I'm still on the fence of annotating it with @protected or @internal.

[stuartmorgan]

The approach seems reasonable at a high level; it means we have to do all the copy and equality code correctly everwhere, but that seems easier to reason about locally than tracking all of the function lifetimes individually.

[bparrishMines]

I believe these were caused by compiling with Flutter version 3.0.0. I can remove if needed.

[stuartmorgan]

Yes, we should hold off on the 9->11 changes.

[stuartmorgan]

Sorry for the delay here; LGTM with the iOS 11.0 changes reverted across the three files.

It's definitely not easy to reason about all the strong and weak reference management here, but I think I followed all of the logic :)

[stuartmorgan]

Yes, we should hold off on the 9->11 changes.

[stuartmorgan]

Do you want to typedef this somewhere? It's shown up a few times.

[bparrishMines]

I assumed we wanted to follow https://dart.dev/guides/language/effective-dart/design#prefer-inline-function-types-over-typedefs. The most important line being:

But in most cases, users want to see what the function type actually is right where it’s used, and the function type syntax gives them that clarity.

[stuartmorgan]

Ah, I didn't realize there was a style guideline for that. SG.

(Does this read better or worse without the comma after url?)

[bparrishMines]

I noticed that too and already removed the comma!