Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@cbracken
Copy link
Member

This migrates PlatformViewController from C++ to Objective-C. Generally, we try to keep the embedder interfaces and components written in Objective-C except for the few places where C++ interfaces are requried to interface with engine APIs such as Shell and PlatformView (e.g. the PlatformViewIOS subclass). Now that the implementation is Objective-C, the class and file are renamed to match Objective-C naming conventions.

This allows us to take advantage of ARC and weak references, which eliminates the need for std::shared_ptr, fml::WeakPtr etc. Further, this eliminates some particularly unintuitive behaviour wherein this class was owned via a std::shared_ptr held by FlutterEngine, and injected into many other classes (e.g. AccessibilityBridge) via a std::shared_ptr& reference -- such that only one instance of the std::shared_ptr actually ever existed, presumably to avoid std::shared_ptr refcounting overhead. Given that this overhead was only incurred a single time at engine initialisation, this seems like overkill. One might ask why it wasn't therefore held in a std::unique_ptr and a std::unique_ptr& reference passed around. Likely, this was because we wanted to take a fml::WeakPtr reference on it.

Regardless, none of this is necessary any longer now that we can inject __weak FlutterPlatformViewsController* instances to classes that use it.

To be clear, this patch makes no attempt whatsoever to simplify or clean up the interface or implementation of this class. This class ties together far too many concepts and is injected into far too many places, and we should break it up and simplify it. However, the goal of this patch was simply to port to an Objective-C interface that plays nicely with the rest of the iOS embedder. This does include a couple minor cleanups in #include/#import order and usage to match our style guide.

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 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.

- (size_t)embeddedViewCount;

// TODO(cbracken): Delete. This is unused.
- (size_t)layerPoolSize;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll delete this in a followup. This is just to keep things consistent for now.


@end

@interface FlutterPlatformViewsController (Testing)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we feel like, we could move this to a separate header, but this header is already internal so not particularly necessary/urgent.


// TODO(cbracken): Eliminate the use of globals.
// Becomes NO if Apple's API changes and blurred backdrop filters cannot be applied.
BOOL canApplyBlurBackdrop = YES;
Copy link
Member Author

@cbracken cbracken Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will go in a followup. It's unnecessary. It's ordered here because oddly enough, it helped minimise the diff.

//
// The Slices are deleted by the PlatformViewsController.reset().
@property(nonatomic, readonly)
std::unordered_map<int64_t, std::unique_ptr<flutter::EmbedderViewSlice>>& slices;
Copy link
Member Author

@cbracken cbracken Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and a bunch below: notice that the properties here are declared as reference types (std::unordered_map&), which allows for self.slices[foo] = bar rather than the underlying ivar type std::unordered_map, in order to avoid self.slices returning a copy.

This however, requires (a) manually declaring the ivar and (b) manually writing the getter.

These should eventually be migrated to Obj-C types like NSMutableDictionary*.

@cbracken
Copy link
Member Author

@hellohuanlin Looks like renaming the file made the diff 100% diff instead of clear. Here's the version without the rename where you can see the diffs clearly:
https://github.com/flutter/engine/pull/56766/files

Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This migrates PlatformViewController from C++ to Objective-C. Generally,
we try to keep the embedder interfaces and components written in
Objective-C except for the few places where C++ interfaces are requried
to interface with engine APIs such as Shell and PlatformView (e.g. the
PlatformViewIOS subclass). Now that the implementation is Objective-C,
the class and file are renamed to match Objective-C naming conventions.

This allows us to take advantage of ARC and weak references, which
eliminates the need for std::shared_ptr, fml::WeakPtr etc. Further, this
eliminates some particularly unintuitive behaviour wherein this class
was owned via a std::shared_ptr held by FlutterEngine, and injected into
many other classes (e.g. AccessibilityBridge) via a std::shared_ptr&
reference -- such that only one instance of the std::shared_ptr actually
ever existed, presumably to avoid std::shared_ptr refcounting overhead.
Given that this overhead was only incurred a single time at engine
initialisation, this seems like overkill. One might ask why it wasn't
therefore held in a `std::unique_ptr` and a `std::unique_ptr&` reference
passed around. Likely, this was because we wanted to take a
`fml::WeakPtr` reference on it.

Regardless, none of this is necessary any longer now that we can inject
`__weak FlutterPlatformViewsController*` instances to classes that use
it.

To be clear, this patch makes no attempt whatsoever to simplify or clean
up the interface or implementation of this class. This class ties
together far too many concepts and is injected into far too many places,
and we should break it up and simplify it. However, the goal of this
patch was simply to port to an Objective-C interface that plays nicely
with the rest of the iOS embedder. This does include a couple minor
cleanups in `#include`/`#import` order and usage to match our style
guide.
@cbracken cbracken added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 26, 2024
@auto-submit auto-submit bot merged commit fb64399 into flutter:main Nov 26, 2024
33 checks passed
@cbracken cbracken deleted the objc-platformviews-controller branch November 26, 2024 01:23
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 26, 2024
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 26, 2024
…159461)

flutter/engine@fe45a66...fb64399

2024-11-26 [email protected] iOS: Migrate PlatformViewsController to
Objective-C (flutter/engine#56790)
2024-11-26 [email protected] Started caching
HandleGLES's hash and made them immutable (flutter/engine#56800)
2024-11-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"[iOS] Full keyboard access scrolling (#56606)" (flutter/engine#56802)
2024-11-26 [email protected] Roll Skia from 12c8bd6ac1d9 to
c1c8ff84997c (14 revisions) (flutter/engine#56801)
2024-11-25 [email protected] [Impeller] better handle allocation
herustics of Android slide in page transition. (flutter/engine#56762)
2024-11-25 [email protected] iOS: Eliminate logging of non-zero origin
platformviews (flutter/engine#56796)
2024-11-25 [email protected] [impeller] gles:
started storing the number of handle deletions to avoid reallocation
(flutter/engine#56799)
2024-11-25 [email protected] Roll Fuchsia Linux SDK from
9o0fWa2xVhmxV6Mtn... to 50xtjbMWWrqay_7m_... (flutter/engine#56795)
2024-11-25 [email protected] Roll Dart SDK from
df716eaa6ed2 to 4b49546a1dfa (1 revision) (flutter/engine#56793)
2024-11-25 [email protected] [iOS] Full
keyboard access scrolling (flutter/engine#56606)
2024-11-25 [email protected] [android] remove fml_check from
surface_texture_external_texture (flutter/engine#56760)
2024-11-25 [email protected] removed unused
variable for skia initialization (flutter/engine#56791)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 9o0fWa2xVhmx to 50xtjbMWWrqa

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure
that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 26, 2024
…visions) (#159461)" (#159498)

<!-- start_original_pr_link -->
Reverts: #159461
<!-- end_original_pr_link -->
<!-- start_initiating_author -->
Initiated by: yjbanov
<!-- end_initiating_author -->
<!-- start_revert_reason -->
Reason for reverting: engine regression
#159497
<!-- end_revert_reason -->
<!-- start_original_pr_author -->
Original PR Author: engine-flutter-autoroll
<!-- end_original_pr_author -->

<!-- start_reviewers -->
Reviewed By: {fluttergithubbot}
<!-- end_reviewers -->

<!-- start_revert_body -->
This change reverts the following previous change:

flutter/engine@fe45a66...fb64399

2024-11-26 [email protected] iOS: Migrate PlatformViewsController to
Objective-C (flutter/engine#56790)
2024-11-26 [email protected] Started caching
HandleGLES's hash and made them immutable (flutter/engine#56800)
2024-11-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"[iOS] Full keyboard access scrolling (#56606)" (flutter/engine#56802)
2024-11-26 [email protected] Roll Skia from 12c8bd6ac1d9 to
c1c8ff84997c (14 revisions) (flutter/engine#56801)
2024-11-25 [email protected] [Impeller] better handle allocation
herustics of Android slide in page transition. (flutter/engine#56762)
2024-11-25 [email protected] iOS: Eliminate logging of non-zero origin
platformviews (flutter/engine#56796)
2024-11-25 [email protected] [impeller] gles:
started storing the number of handle deletions to avoid reallocation
(flutter/engine#56799)
2024-11-25 [email protected] Roll Fuchsia Linux SDK from
9o0fWa2xVhmxV6Mtn... to 50xtjbMWWrqay_7m_... (flutter/engine#56795)
2024-11-25 [email protected] Roll Dart SDK from
df716eaa6ed2 to 4b49546a1dfa (1 revision) (flutter/engine#56793)
2024-11-25 [email protected] [iOS] Full
keyboard access scrolling (flutter/engine#56606)
2024-11-25 [email protected] [android] remove fml_check from
surface_texture_external_texture (flutter/engine#56760)
2024-11-25 [email protected] removed unused
variable for skia initialization (flutter/engine#56791)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 9o0fWa2xVhmx to 50xtjbMWWrqa

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure
that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

<!-- end_revert_body -->

Co-authored-by: auto-submit[bot] <[email protected]>
auto-submit bot pushed a commit that referenced this pull request Nov 26, 2024
…#56817)

This is a combination of 3 reverts, required to get back to the revert that caused `ios_platform_view_tests` to start failing in the framework repo. In reverse chronological order, this reverts two trivial commits plus the non-trivial commit that likely caused the breakage:

* Revert "iOS: Eliminate global in platformviews controller (#56805)" This reverts commit 752e2d7.
* Revert "iOS: Delete FlutterPlatformViewsController.layerPoolSize (#56806)" This reverts commit 21c655c.
* Revert "iOS: Migrate PlatformViewsController to Objective-C (#56790)" This reverts commit fb64399.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 26, 2024
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 26, 2024
…159504)

flutter/engine@fe45a66...bd165dc

2024-11-26 [email protected] Revert "iOS: Migrate PlatformViewsController
to Objective-C (#56790)" (flutter/engine#56817)
2024-11-26 [email protected] iOS: Rename FlutterPlatformViews_Internal.mm
(flutter/engine#56816)
2024-11-26 [email protected] iOS: Eliminate global in platformviews
controller (flutter/engine#56805)
2024-11-26 [email protected] Roll Skia from a276978ba7c8 to
f149f852c70a (1 revision) (flutter/engine#56812)
2024-11-26 [email protected] Roll Skia from d7a267d88fd6 to
a276978ba7c8 (1 revision) (flutter/engine#56811)
2024-11-26 [email protected] Roll Skia from 8d9d892657a7 to
d7a267d88fd6 (1 revision) (flutter/engine#56810)
2024-11-26 [email protected] Roll Dart SDK from
bdb76c714009 to ca02d403f1a8 (1 revision) (flutter/engine#56809)
2024-11-26 [email protected] Roll Skia from b697dd1b03b2 to
8d9d892657a7 (1 revision) (flutter/engine#56808)
2024-11-26 [email protected] Roll Skia from c1c8ff84997c to
b697dd1b03b2 (1 revision) (flutter/engine#56807)
2024-11-26 [email protected] iOS: Delete
FlutterPlatformViewsController.layerPoolSize (flutter/engine#56806)
2024-11-26 [email protected] Roll Dart SDK from
4b49546a1dfa to bdb76c714009 (1 revision) (flutter/engine#56803)
2024-11-26 [email protected] iOS: Migrate PlatformViewsController to
Objective-C (flutter/engine#56790)
2024-11-26 [email protected] Started caching
HandleGLES's hash and made them immutable (flutter/engine#56800)
2024-11-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"[iOS] Full keyboard access scrolling (#56606)" (flutter/engine#56802)
2024-11-26 [email protected] Roll Skia from 12c8bd6ac1d9 to
c1c8ff84997c (14 revisions) (flutter/engine#56801)
2024-11-25 [email protected] [Impeller] better handle allocation
herustics of Android slide in page transition. (flutter/engine#56762)
2024-11-25 [email protected] iOS: Eliminate logging of non-zero origin
platformviews (flutter/engine#56796)
2024-11-25 [email protected] [impeller] gles:
started storing the number of handle deletions to avoid reallocation
(flutter/engine#56799)
2024-11-25 [email protected] Roll Fuchsia Linux SDK from
9o0fWa2xVhmxV6Mtn... to 50xtjbMWWrqay_7m_... (flutter/engine#56795)
2024-11-25 [email protected] Roll Dart SDK from
df716eaa6ed2 to 4b49546a1dfa (1 revision) (flutter/engine#56793)
2024-11-25 [email protected] [iOS] Full
keyboard access scrolling (flutter/engine#56606)
2024-11-25 [email protected] [android] remove fml_check from
surface_texture_external_texture (flutter/engine#56760)
2024-11-25 [email protected] removed unused
variable for skia initialization (flutter/engine#56791)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 9o0fWa2xVhmx to 50xtjbMWWrqa

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
auto-submit bot pushed a commit that referenced this pull request Nov 27, 2024
)

This field is unused in the codebase/tests.

This is a reland of #56806, which was reverted as part of #56790.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
…6790)

This migrates PlatformViewController from C++ to Objective-C. Generally, we try to keep the embedder interfaces and components written in Objective-C except for the few places where C++ interfaces are requried to interface with engine APIs such as Shell and PlatformView (e.g. the PlatformViewIOS subclass). Now that the implementation is Objective-C, the class and file are renamed to match Objective-C naming conventions.

This allows us to take advantage of ARC and weak references, which eliminates the need for std::shared_ptr, fml::WeakPtr etc. Further, this eliminates some particularly unintuitive behaviour wherein this class was owned via a std::shared_ptr held by FlutterEngine, and injected into many other classes (e.g. AccessibilityBridge) via a std::shared_ptr& reference -- such that only one instance of the std::shared_ptr actually ever existed, presumably to avoid std::shared_ptr refcounting overhead. Given that this overhead was only incurred a single time at engine initialisation, this seems like overkill. One might ask why it wasn't therefore held in a `std::unique_ptr` and a `std::unique_ptr&` reference passed around. Likely, this was because we wanted to take a `fml::WeakPtr` reference on it.

Regardless, none of this is necessary any longer now that we can inject `__weak FlutterPlatformViewsController*` instances to classes that use it.

To be clear, this patch makes no attempt whatsoever to simplify or clean up the interface or implementation of this class. This class ties together far too many concepts and is injected into far too many places, and we should break it up and simplify it. However, the goal of this patch was simply to port to an Objective-C interface that plays nicely with the rest of the iOS embedder. This does include a couple minor cleanups in `#include`/`#import` order and usage to match our style guide.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
…tter/engine#56830)

This field is unused in the codebase/tests.

This is a reland of flutter/engine#56806, which was reverted as part of flutter/engine#56790.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-ios

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants