iOS FlutterTextureRegistry should be a proxy, not the engine itself

[endless7]

Because the plugin retains the texture registrar there will be a cycle. We make the proxy object a weak reference back to the engine serve as the texture registry object that is returned instead.

Related issue: flutter/flutter#102990

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], including [Features we expect every widget to implement].
• [✓] I signed the [CLA].
• [✓] I listed at least one issue that this PR fixes in the description above.
• [✓] 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.

[endless7]

cc @stuartmorgan

[cyanglaz]

This is great! Thanks!
On macOS, both FlutterEngine and FlutterRenderer are "FlutterPluginRegistry".
https://github.com/flutter/engine/blob/main/shell/platform/darwin/macos/framework/Headers/FlutterEngine.h#L24
https://github.com/flutter/engine/blob/main/shell/platform/darwin/macos/framework/Source/FlutterRenderer.h#L15

Maybe while we are here it would be nice to migrate macOS to use FlutterTextureRegistryRelay? It doesn't have to be in the same PR but if we want to do this, it might make sense to put FlutterTextureRegistryRelay in a common code base. Maybe https://github.com/flutter/engine/tree/main/shell/platform/darwin/graphics?

cc @cbracken for thoughts on macOS.

[cyanglaz]

optional nits:

Suggested change

	self = [super init];
	if (self != nil) {
	if (self = [super init]) {

this is following the pattern in the objc document: https://developer.apple.com/documentation/objectivec/nsobject/1418641-init?language=objc

[endless7]

done

[cyanglaz]

optional nits, escape early:

Suggested change

	if (self.parent) {
	return [self.parent registerTexture:texture];
	} else {
	FML_LOG(WARNING) << "Using on an empty registry.";
	}
	return 0;
	if (!self.parent) {
	FML_LOG(WARNING) << "Using on an empty registry.";
	return 0;
	}
	return [self.parent registerTexture:texture];

[endless7]

done

[cyanglaz]

LGTM
talked offline with @cbracken. He has been doing related MacOS refactoring and would also like to take a look at this.

[chinmaygarde]

@cyanglaz Can we land this now and file a followup perhaps? This looks good to go.

[cyanglaz]

Yes, I'm ok to land it now. We will need a secondary review. @gaaclarke @cbracken @stuartmorgan

@endless7 Could you update your PR description to have more details, also please bring back the list and check the check marks accordingly :)

[endless7]

@cyanglaz Updated. Anything else to do?

[jmagman]

re-ping @cyanglaz

[jmagman]

@cyanglaz says the PR description looks good, now waiting for a second review from @gaaclarke, @cbracken, or @stuartmorgan.

[hellohuanlin]

can you add a comment explaining that this relay is used to solve the issue with plugin retaining the engine?

[endless7]

done.

[jmagman]

Use /** */
comments as in the other headers.

From this comment I don't really understand when this would be used. Returned from what?

[endless7]

@jmagman, are these comments ok?

[jmagman]

How about:

/**
 * Wrapper around a weakly held collection of registered textures.
 *
 * Avoids a retain cycle between plugins and the engine.
 */
@interface FlutterTextureRegistryRelay : NSObject <FlutterTextureRegistry>

/**
 * A weak reference to a FlutterEngine that will be passed texture registration.
 */
@property(nonatomic, assign) NSObject<FlutterTextureRegistry>* parent;

[endless7]

Thanks, this would be better.

[jmagman]

@endless7 Once the test failures, merge conflicts are fixed, and @hellohuanlin's requested comment is added, this has been approved and will be ready to land.