[iOS] Fix platfotm view called multiple times

[zhongwuzw]

Description

Related Prs: #17336
cc. @blasten

Users may return a newly created embedded view when call view method of FlutterPlatformView protocol. After #17336, we call view method of FlutterPlatformView protocol multiple times, causes GetPlatformViewRect return incorrect rect.

Related Issues

flutter/flutter#59751

Tests

I added the following tests:

N/A.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the contributor guide and followed the process outlined there for submitting PRs.
• I signed the CLA.
• I read and followed the C++, Objective-C, Java style guides for the engine.
• I read the tree hygiene wiki page, which explains my responsibilities.
• I updated/added relevant documentation.
• All existing and new tests are passing.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.
  • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
  • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
  • I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

[fluttergithubbot]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[cyanglaz]

@blasten As I remembered, after landing unobstructed platform views, we recommend against creating new views each time when view is called.
I have opened an issue to track documentation efforts. flutter/flutter#56736

@zhongwuzw Is there a way you can workaround the issue by not creating a new UIView each frame?

[zhongwuzw]

@blasten As I remembered, after landing unobstructed platform views, we recommend against creating new views each time when view is called.
I have opened an issue to track documentation efforts. flutter/flutter#56736

@zhongwuzw Is there a way you can workaround the issue by not creating a new UIView each frame?

@cyanglaz Did I miss something? 😄 I think we only create a new view when init rather than each frame? my PR fixes flutter/flutter#59751 because we call view of FlutterPlatformView twice when we create UIView, it's unnecessary.

[cyanglaz]

@zhongwuzw You are right, "we only create a new view when init rather than each frame". I think the engine calls the view method multiple times that's why we recommend not to initialize the UIView instance in the view method.

I think this PR does fix the issue. However, I'm more worried about plugins starting to call UIView's constructor in view, which is a bad practice. I wonder if we can maybe print out a warning if the UIView was created multiple times during a single frame.

[zhongwuzw]

@cyanglaz To print out a warning, we need to call view multiple twice for the check, I think maybe it would cause performance issues. What do you think? 🤔

[cyanglaz]

@zhongwuzw FML_DCHECK only runs in debug mode. I think maybe we can utilize it? Something like:

UIView* platformView = [embedded_view view];
FML_DCHECK(platformView == [embedded_view view], "The [view] method should not return a new instance of UIView during the same frame.");

[zhongwuzw]

@zhongwuzw FML_DCHECK only runs in debug mode. I think maybe we can utilize it? Something like:

UIView* platformView = [embedded_view view];
FML_DCHECK(platformView == [embedded_view view], "The [view] method should not return a new instance of UIView during the same frame.");

@cyanglaz I added.

[cyanglaz]

Code looks good! Can we test this? Maybe in https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm?

[zhongwuzw]

@cyanglaz To test whether FML_DCHECK works? 🤔 we just add an FML_DCHECK, maybe we don't need it.

[cyanglaz]

@zhongwuzw It would be nice if we have a test to make sure that if [view] returns different instance in a single frame, the plugin will receive the crash.

[zhongwuzw]

@cyanglaz I think we can't catch the crash(abort()). 🤔

[cyanglaz]

@zhongwuzw
After thinking about it twice, I actually liked your original idea better. I wanted to apologize for the back and force. I think we should revert all the changes after my review and get the code back to the original state of the PR. 

There was one thing I was afraid of, which was that after the fix, engine devs could still call [views_[0] view] to get the view and mess things up without knowing. (That was the major reason I didn't like the change)After another look at the code, I think we can actually get rid of views_ because there was no other critical sessions in the code that uses views_, expect some usage of .count(), which can be substituted by root_views_[0].count() or interceptors_[0].count()

Then we can actually test it by checking the instance of the platform view to always be the same even the factory returns a new instance every time. 

WDYT? Would you be willing to do that? :) I appreciate your patients in advance. 

[cyanglaz]

Offline discussion: The code looks good.
Need some updates in the tests:

1. Remove the enableViewCreateOnceCheck in the unit test
2. Possibly add the same "only call once" check in the scenario app and make sure the scenario app doesn't crash.

[zhongwuzw]

@cyanglaz Done.

[cyanglaz]

LGTM!

[fluttergithubbot]

This pull request is not suitable for automatic merging in its current state.

• The status or check suite Linux Android AOT Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

[cyanglaz]

@zhongwuzw CI seems still failing and upstream is green. Maybe rebase and try again?