[iOS] clean ups to platform view controller #54335

jonahwilliams · 2024-08-03T18:21:30Z

Splits FlutterPlatformViews.mm into the primarily C++ class FlutterPlatformViewsController/Pool/Overlay and all of the touch interceptor / gesture stuff.

Renames FlutterPlatformViewsController to PlatformViewsController, because, you know.
Renames FlutterPlatformViewLayer and FlutterPlatformViewLayerPool to OverlayLayer and OverlayLayerPool

chinmaygarde · 2024-08-03T20:22:16Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm

 #import <UIKit/UIKit.h>
 #import <XCTest/XCTest.h>
 #include "fml/synchronization/count_down_latch.h"
+#include "shell/platform/darwin/ios/framework/Source/platform_views_controller.h"


Rename this file to get rid of the redundant Flutter?

Feeling lazy

chinmaygarde · 2024-08-03T20:29:08Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm


+BOOL BlurRadiusEqualToBlurRadius(CGFloat radius1, CGFloat radius2) {
+  const CGFloat epsilon = 0.01;
+  return radius1 - radius2 < epsilon;


This doesn't seem quite right. Going past negative epsilon won't be caught. (radius1 > (radius2 - epsilon)) && (radius1 < (radius2 + epsilon)) perhaps. Or if you are passing in a potentially zero epsion, check for direct equality too.

GitHub didn't update. Did you miss staging this?

chinmaygarde · 2024-08-03T20:30:23Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm


-    auto flutterPlatformViewsController =
-        std::make_shared<flutter::FlutterPlatformViewsController>();
+    auto flutterPlatformViewsController = std::make_shared<flutter::PlatformViewsController>();


Use underscore case?

Ah, this is a ObjC++ TU. I guess this is fine.

We should probably document a standard for this in our style guide; we're super inconsistent. I generally go for underscore case for C++ objects like this and reserve lowerCamelCase for Obj-C objects and ivars, but I'd be fine with anything so long as it's consistent.

chinmaygarde · 2024-08-03T20:32:53Z

shell/platform/darwin/ios/framework/Source/platform_views_controller.h

+class IOSSurface;
+
+/// @brief State holder for a Flutter overlay layer.
+struct OverlayLayer {


Move this and the pool to its own TU, OOL the cdtors, and add a headerdoc while you are at it too?

chinmaygarde · 2024-08-03T20:42:40Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.mm

+// We only fail this recognizer when asked to do so by the Flutter framework (which does so by
+// invoking an acceptGesture method on the platform_views channel). And this is how we allow the
+// Flutter framework to delay or prevent the embedded view from getting a touch sequence.
+@interface DelayingGestureRecognizer : UIGestureRecognizer <UIGestureRecognizerDelegate>


IMO this should be "namespaced". This looks like a common class name and we'll end up with collisions in some cases.

shell/platform/darwin/ios/framework/Source/platform_views_controller.h

chinmaygarde · 2024-08-06T18:35:50Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm


+BOOL BlurRadiusEqualToBlurRadius(CGFloat radius1, CGFloat radius2) {
+  const CGFloat epsilon = 0.01;
+  return radius1 - radius2 < epsilon;


GitHub didn't update. Did you miss staging this?

jonahwilliams · 2024-08-06T18:49:54Z

Oops, fixed missing update to blur radius check

cbracken

There are a ton of nits and doc comment rewriting we should do, but that make sense to do in a followup, and keep this as just a restructuring of the code plus the top-level class rename.

lgtm -- this looks great. Thanks for cleaning up.

cbracken · 2024-08-06T20:14:32Z

shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h


 namespace flutter {
-class FlutterPlatformViewsController;
+class PlatformViewsController;


Thank you. We should reserve the Flutter name prefix for where we're forced to, in Obj-C code.

cbracken · 2024-08-06T20:17:05Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm


-    auto flutterPlatformViewsController =
-        std::make_shared<flutter::FlutterPlatformViewsController>();
+    auto flutterPlatformViewsController = std::make_shared<flutter::PlatformViewsController>();


We should probably document a standard for this in our style guide; we're super inconsistent. I generally go for underscore case for C++ objects like this and reserve lowerCamelCase for Obj-C objects and ivars, but I'd be fine with anything so long as it's consistent.

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.mm

cbracken · 2024-08-06T20:26:13Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.mm

 @end
+
+@implementation UIView (FirstResponder)
+- (BOOL)flt_hasFirstResponderInViewHierarchySubtree {


We should remove the flt_ prefix here, but there are a bunch of these and it might make sense to land cleanups in a followup rather than along with the refactoring.

Should it just be "FLTHasFirstResponder..."?

Just hasFirstResponderInViewHierarchySubtree since it's a method. There shouldn't be any need for an flt prefix at all.

I believe the prefix is correct since it's a category on a UIKit class.

Methods in a category should be prefixed with a lowercase version of the prefix used for the category name followed by an underscore (e.g., gtm_myCategoryMethodOnAString:) in order to prevent collisions in Objective-C’s global namespace.

https://google.github.io/styleguide/objcguide.html#category-naming

cbracken · 2024-08-06T20:29:17Z

shell/platform/darwin/ios/framework/Source/platform_views_controller.mm


-FLUTTER_ASSERT_ARC


Should this file be renamed .cc if there's no more Obj-C in it?

We're still doing objective C stuff in here

We should leave this in, in that case, to blow up if we compile with ARC disabled.

…s_Internal.mm Co-authored-by: Chris Bracken <[email protected]>

…#54335)

…152985) flutter/engine@206e86e...5a0fd5f 2024-08-07 [email protected] [Impeller] Statically check stage interfaces for precision mismatches. (flutter/engine#54375) 2024-08-07 [email protected] Roll Skia from f04ba9cd0e4b to 968a00456bc5 (4 revisions) (flutter/engine#54378) 2024-08-06 [email protected] Roll Dart SDK from dedfa3393296 to f5d136899082 (5 revisions) (flutter/engine#54376) 2024-08-06 [email protected] [Impeller] Reorder pipeline construction in content context. (flutter/engine#54373) 2024-08-06 [email protected] [iOS] clean ups to platform view controller (flutter/engine#54335) 2024-08-06 [email protected] [Impeller] ensure precision matches for buggy vulkan drivers. (flutter/engine#54372) 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],[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

…lutter#152985) flutter/engine@206e86e...5a0fd5f 2024-08-07 [email protected] [Impeller] Statically check stage interfaces for precision mismatches. (flutter/engine#54375) 2024-08-07 [email protected] Roll Skia from f04ba9cd0e4b to 968a00456bc5 (4 revisions) (flutter/engine#54378) 2024-08-06 [email protected] Roll Dart SDK from dedfa3393296 to f5d136899082 (5 revisions) (flutter/engine#54376) 2024-08-06 [email protected] [Impeller] Reorder pipeline construction in content context. (flutter/engine#54373) 2024-08-06 [email protected] [iOS] clean ups to platform view controller (flutter/engine#54335) 2024-08-06 [email protected] [Impeller] ensure precision matches for buggy vulkan drivers. (flutter/engine#54372) 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],[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

#54820 on stable branch. That PR was using some of the symbol changes in #54335, so I had so switch some back: e0a630d Cherry-pick request: flutter/flutter#154712 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

[iOS] clean ups to platform view controller

5d69b4d

github-actions bot added the platform-ios label Aug 3, 2024

licenses

1513f94

chinmaygarde reviewed Aug 3, 2024

View reviewed changes

jonahwilliams added 2 commits August 6, 2024 10:46

++

35cd631

++

25a7e63

jonahwilliams marked this pull request as ready for review August 6, 2024 18:26

chinmaygarde approved these changes Aug 6, 2024

View reviewed changes

++

98f28a4

jonahwilliams added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Aug 6, 2024

jonahwilliams requested a review from cbracken August 6, 2024 19:18

cbracken approved these changes Aug 6, 2024

View reviewed changes

Update shell/platform/darwin/ios/framework/Source/FlutterPlatformView…

40e9ca5

…s_Internal.mm Co-authored-by: Chris Bracken <[email protected]>

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 6, 2024

auto-submit bot merged commit 483ff70 into flutter:main Aug 6, 2024

engine-flutter-autoroll mentioned this pull request Aug 6, 2024

Roll Flutter Engine from 206e86ee8a40 to 483ff70eced7 (2 revisions) flutter/flutter#152971

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 6, 2024

483ff70ec [iOS] clean ups to platform view controller (flutter/engine…

0fc0ba8

…#54335)

engine-flutter-autoroll mentioned this pull request Aug 6, 2024

Roll Flutter Engine from 206e86ee8a40 to 85d989ed2b23 (3 revisions) flutter/flutter#152973

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 6, 2024

483ff70ec [iOS] clean ups to platform view controller (flutter/engine…

15c673b

…#54335)

engine-flutter-autoroll mentioned this pull request Aug 7, 2024

Roll Flutter Engine from 206e86ee8a40 to c20371dab213 (4 revisions) flutter/flutter#152975

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 7, 2024

483ff70ec [iOS] clean ups to platform view controller (flutter/engine…

a1b8709

…#54335)

engine-flutter-autoroll mentioned this pull request Aug 7, 2024

Roll Flutter Engine from 206e86ee8a40 to 5a0fd5fbecc6 (6 revisions) flutter/flutter#152980

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 7, 2024

483ff70ec [iOS] clean ups to platform view controller (flutter/engine…

ebfe2a8

…#54335)

engine-flutter-autoroll mentioned this pull request Aug 7, 2024

Roll Flutter Engine from 206e86ee8a40 to 5a0fd5fbecc6 (6 revisions) flutter/flutter#152985

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 7, 2024

483ff70ec [iOS] clean ups to platform view controller (flutter/engine…

efeb8c9

…#54335)

jmagman added a commit to jmagman/engine that referenced this pull request Sep 6, 2024

Fix up mixmatches from flutter#54335

e0a630d

jmagman mentioned this pull request Sep 6, 2024

[CP-stable] fix iOS platform view clipping path intersection #55007

Merged

8 tasks


		FLUTTER_ASSERT_ARC

[iOS] clean ups to platform view controller #54335

[iOS] clean ups to platform view controller #54335

Uh oh!

Conversation

jonahwilliams commented Aug 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Aug 6, 2024

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonahwilliams commented Aug 3, 2024 •

edited

Loading