Add an iOS PlatformViewsController for creating/disposing UIViews. #6569

amirh · 2018-10-17T17:03:55Z

Note that this is my first time writing ObjC, don't assume I know what I'm doing 😄, feedback is welcomed.

Adds a FlutterPlatformViewFactory protocol - a simple factory protocol to be implemented by plugins
that exposes a UIView for embeeding in Flutter apps.
Adds a FlutterPlatformView protocol, which is used to associate a
dispose callback with a UIView created by a FlutterPlatformViewFactory.
Exposes a registerViewFactory method in FlutterPluginRegistrar.
Implements the flutter/platform_views system channel on iOS, allowing
Dart code to ask for creation/destruction of UIViews.

flutter/flutter#19030

amirh · 2018-10-17T17:06:43Z

dnfield · 2018-10-17T17:14:28Z

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

+#include "flutter/fml/platform/darwin/scoped_nsobject.h"
+#include "flutter/shell/platform/darwin/ios/framework/Headers/FlutterChannels.h"
+
+@implementation FlutterPlatformViewsController {


How is this meant to be different from a FlutterViewController?

I also wonder if we're further confusing some already confusing naming conventions. We have

FlutterViewController, which is a UIViewController

FlutterView which is a UIView

PlatformViewIOS, which is a shell::PlatformView (but not a view in the sense of a UIView or UIViewController

It might be better if we could extend FlutterViewController to do the job that this class is doing now, especially with some of the refactoring I've been working on to disentangle the engine from the view controller.

Good point, I agree the name is confusing as this is not a ViewController but the controller for the platform views... lets see if we can come up with a better name (let me know if you have a good suggestion).

As for keeping this logic inside FlutterViewController vs keeping it in a separate class, we can definitely do both, I actually discussed this with Chinmay and we didn't have a really strong preference, we leaned to encapsulating it in it's own file as FlutterViewController is starting to get a little big (at least before the coming refactor).
Keeping it out of FlutterViewController might also make this specific part of the logic a little easier to unit test (the only dependency a test needs to provide is a messenger in order to exercise the platform views code).

dnfield · 2018-10-18T00:30:44Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+NS_ASSUME_NONNULL_BEGIN
+
+/**
+ An instance of a `UIView` created by a `FlutterPlatformViewFactory`.


I've reformatted the documentation in #6447 - the format being used previously was inconsistent and didn't play with the doc generator very well. This one should have * all the way down each line.

done.
Do you know if there's a way to configure Xcode to play well with these leading * ?

dnfield · 2018-10-18T00:30:50Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+@protocol FlutterPlatformView <NSObject>
+- (UIView*)view;
+/**
+ Called when the Flutter engine no longer needs this `view`.


dnfield · 2018-10-18T00:31:42Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+ The implementation of this method should create a new `UIView` and return a `FlutterPlatformView`
+ that wraps it.
+
+ - Parameters:


This format won't really work in our docs. See https://github.com/flutter/engine/pull/6447/files#diff-2acb8a0db370213c3467450b9e70984dR43 for example

(@param can be used multiple times, and will format as a table)

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

chinmaygarde · 2018-10-23T21:07:54Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+#ifndef FLUTTER_FLUTTERPLATFORMVIEWS_H_
+#define FLUTTER_FLUTTERPLATFORMVIEWS_H_
+
+#import <CoreMedia/CoreMedia.h>


Unnecessary header import.

chinmaygarde · 2018-10-23T21:08:23Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+ * This protocol is used to associate a view with a dispose callback.
+ */
+FLUTTER_EXPORT
+@protocol FlutterPlatformView <NSObject>


Let's get rid of this protocol entirely and make the create call return a view.

chinmaygarde · 2018-10-23T21:11:49Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+ *   code, this will be null. Otherwise this will be the value sent from the Dart code as decoded by
+ *   `createArgsCodec`.
+ */
+- (NSObject<FlutterPlatformView>*)createWithFrame:(CGRect)frame


Objective-C method names usually don't have conjunctions. Also, don't shorten arguments to args. See NSInvocation for prior art on this.

Same comment for Id and Identifier here and elsewhere.

chinmaygarde · 2018-10-23T21:15:47Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+#define FLUTTER_FLUTTERPLATFORMVIEWS_H_
+
+#import <CoreMedia/CoreMedia.h>
+#import <Foundation/Foundation.h>


Foundation is implied by UIKit.

chinmaygarde · 2018-10-23T21:16:25Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+#import <UIKit/UIKit.h>
+
+#include "FlutterCodecs.h"
+#include "FlutterMacros.h"


Nit: Always #import Objective-C header. Though we usually add explicit guards to all our headers.

chinmaygarde · 2018-10-23T21:17:50Z

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

+ private:
+  fml::scoped_nsobject<FlutterMethodChannel> channel_;
+  std::map<std::string, fml::scoped_nsobject<NSObject<FlutterPlatformViewFactory>>> factories_;
+  std::map<long long, fml::scoped_nsobject<NSObject<FlutterPlatformView>>> views_;


Please replace this with one of the types in stdint.h

amirh

Thanks for the review! PTAL.

amirh · 2018-10-23T21:27:55Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+#ifndef FLUTTER_FLUTTERPLATFORMVIEWS_H_
+#define FLUTTER_FLUTTERPLATFORMVIEWS_H_
+
+#import <CoreMedia/CoreMedia.h>


amirh · 2018-10-23T21:31:01Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+#define FLUTTER_FLUTTERPLATFORMVIEWS_H_
+
+#import <CoreMedia/CoreMedia.h>
+#import <Foundation/Foundation.h>


amirh · 2018-10-23T21:31:07Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+#import <UIKit/UIKit.h>
+
+#include "FlutterCodecs.h"
+#include "FlutterMacros.h"


amirh · 2018-10-23T21:32:04Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+ * This protocol is used to associate a view with a dispose callback.
+ */
+FLUTTER_EXPORT
+@protocol FlutterPlatformView <NSObject>


amirh · 2018-10-23T21:33:49Z

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h

+ *   code, this will be null. Otherwise this will be the value sent from the Dart code as decoded by
+ *   `createArgsCodec`.
+ */
+- (NSObject<FlutterPlatformView>*)createWithFrame:(CGRect)frame


amirh · 2018-10-23T21:43:25Z

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

+ private:
+  fml::scoped_nsobject<FlutterMethodChannel> channel_;
+  std::map<std::string, fml::scoped_nsobject<NSObject<FlutterPlatformViewFactory>>> factories_;
+  std::map<long long, fml::scoped_nsobject<NSObject<FlutterPlatformView>>> views_;


* Adds a FlutterPlatformViewFactory protocol - a simple factory protocol to be implemented by plugins that exposes a UIView for embeeding in Flutter apps. * Adds a FlutterPlatformView protocol, which is used to associate a dispose callback with a `UIView` created by a FlutterPlatformViewFactory. * Exposes a registerViewFactory method in FlutterPluginRegistrar. * Implements the `flutter/platform_views` system channel on iOS, allowing Dart code to ask for creation/destruction of UIViews.

chinmaygarde

Nit but lgtm otherwise. Thanks!

chinmaygarde · 2018-10-24T21:26:40Z

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

+  void OnMethodCall(FlutterMethodCall* call, FlutterResult& result);
+  void OnCreate(FlutterMethodCall* call, FlutterResult& result);
+  void OnDispose(FlutterMethodCall* call, FlutterResult& result);
+};


Disallow copy and assign here. See the section about this in the style guide https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types.

flutter/engine@2586e94...9f2e2ba git log 2586e94..9f2e2ba --no-merges --oneline 9f2e2ba Add/expose API for Paragraph.getBoxesForRange BoxHeightStyle and BoxWidthStyle. (flutter/engine#6644) 9669b70 Add an iOS PlatformViewsController for creating/disposing UIViews. (flutter/engine#6569) 6c2477b fix setter for viewOpaque (flutter/engine#6653) 2bd8c94 Roll src/third_party/skia a7682c8c73e4..4f598e8c829a (8 commits) (flutter/engine#6654) e1e6093 Realize kernel asset mappings on a worker thread if one is available. (flutter/engine#6648) d3874a9 Roll src/third_party/skia 3b57279fd65a..a7682c8c73e4 (9 commits) (flutter/engine#6652) 3dddf4d Roll src/third_party/skia 8429c7930291..3b57279fd65a (2 commits) (flutter/engine#6650) 7b3bb62 Roll src/third_party/skia 3aca39df7b22..8429c7930291 (5 commits) (flutter/engine#6647) 26e8840 Dart SDK roll for 2018-10-23 9829c51 Roll src/third_party/skia b46c4d0925ad..3aca39df7b22 (12 commits) (flutter/engine#6643) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.

flutter/engine@2586e94...914551b git log 2586e94..914551b --no-merges --oneline 914551b Dart SDK roll for 2018-10-24 9f2e2ba Add/expose API for Paragraph.getBoxesForRange BoxHeightStyle and BoxWidthStyle. (flutter/engine#6644) 9669b70 Add an iOS PlatformViewsController for creating/disposing UIViews. (flutter/engine#6569) 6c2477b fix setter for viewOpaque (flutter/engine#6653) 2bd8c94 Roll src/third_party/skia a7682c8c73e4..4f598e8c829a (8 commits) (flutter/engine#6654) e1e6093 Realize kernel asset mappings on a worker thread if one is available. (flutter/engine#6648) d3874a9 Roll src/third_party/skia 3b57279fd65a..a7682c8c73e4 (9 commits) (flutter/engine#6652) 3dddf4d Roll src/third_party/skia 8429c7930291..3b57279fd65a (2 commits) (flutter/engine#6650) 7b3bb62 Roll src/third_party/skia 3aca39df7b22..8429c7930291 (5 commits) (flutter/engine#6647) 26e8840 Dart SDK roll for 2018-10-23 9829c51 Roll src/third_party/skia b46c4d0925ad..3aca39df7b22 (12 commits) (flutter/engine#6643) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.

flutter/engine@2586e94...ff0525f git log 2586e94..ff0525f --no-merges --oneline ff0525f Add missing entry-points. (flutter/engine#6634) 08771fe Revert dart before engine commit 26e8840 (flutter/engine#6656) 914551b Dart SDK roll for 2018-10-24 9f2e2ba Add/expose API for Paragraph.getBoxesForRange BoxHeightStyle and BoxWidthStyle. (flutter/engine#6644) 9669b70 Add an iOS PlatformViewsController for creating/disposing UIViews. (flutter/engine#6569) 6c2477b fix setter for viewOpaque (flutter/engine#6653) 2bd8c94 Roll src/third_party/skia a7682c8c73e4..4f598e8c829a (8 commits) (flutter/engine#6654) e1e6093 Realize kernel asset mappings on a worker thread if one is available. (flutter/engine#6648) d3874a9 Roll src/third_party/skia 3b57279fd65a..a7682c8c73e4 (9 commits) (flutter/engine#6652) 3dddf4d Roll src/third_party/skia 8429c7930291..3b57279fd65a (2 commits) (flutter/engine#6650) 7b3bb62 Roll src/third_party/skia 3aca39df7b22..8429c7930291 (5 commits) (flutter/engine#6647) 26e8840 Dart SDK roll for 2018-10-23 9829c51 Roll src/third_party/skia b46c4d0925ad..3aca39df7b22 (12 commits) (flutter/engine#6643) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.

flutter/engine@2586e94...a6e816b git log 2586e94..a6e816b --no-merges --oneline a6e816b Roll src/third_party/skia 4f598e8c829a..dfca8f6adb6b (7 commits) (flutter/engine#6658) ff0525f Add missing entry-points. (flutter/engine#6634) 08771fe Revert dart before engine commit 26e8840 (flutter/engine#6656) 914551b Dart SDK roll for 2018-10-24 9f2e2ba Add/expose API for Paragraph.getBoxesForRange BoxHeightStyle and BoxWidthStyle. (flutter/engine#6644) 9669b70 Add an iOS PlatformViewsController for creating/disposing UIViews. (flutter/engine#6569) 6c2477b fix setter for viewOpaque (flutter/engine#6653) 2bd8c94 Roll src/third_party/skia a7682c8c73e4..4f598e8c829a (8 commits) (flutter/engine#6654) e1e6093 Realize kernel asset mappings on a worker thread if one is available. (flutter/engine#6648) d3874a9 Roll src/third_party/skia 3b57279fd65a..a7682c8c73e4 (9 commits) (flutter/engine#6652) 3dddf4d Roll src/third_party/skia 8429c7930291..3b57279fd65a (2 commits) (flutter/engine#6650) 7b3bb62 Roll src/third_party/skia 3aca39df7b22..8429c7930291 (5 commits) (flutter/engine#6647) 26e8840 Dart SDK roll for 2018-10-23 9829c51 Roll src/third_party/skia b46c4d0925ad..3aca39df7b22 (12 commits) (flutter/engine#6643) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary.

flutter/engine@2586e94...a6e816b git log 2586e94..a6e816b --no-merges --oneline a6e816b Roll src/third_party/skia 4f598e8c829a..dfca8f6adb6b (7 commits) (flutter/engine#6658) ff0525f Add missing entry-points. (flutter/engine#6634) 08771fe Revert dart before engine commit 26e8840 (flutter/engine#6656) 914551b Dart SDK roll for 2018-10-24 9f2e2ba Add/expose API for Paragraph.getBoxesForRange BoxHeightStyle and BoxWidthStyle. (flutter/engine#6644) 9669b70 Add an iOS PlatformViewsController for creating/disposing UIViews. (flutter/engine#6569) 6c2477b fix setter for viewOpaque (flutter/engine#6653) 2bd8c94 Roll src/third_party/skia a7682c8c73e4..4f598e8c829a (8 commits) (flutter/engine#6654) e1e6093 Realize kernel asset mappings on a worker thread if one is available. (flutter/engine#6648) d3874a9 Roll src/third_party/skia 3b57279fd65a..a7682c8c73e4 (9 commits) (flutter/engine#6652) 3dddf4d Roll src/third_party/skia 8429c7930291..3b57279fd65a (2 commits) (flutter/engine#6650) 7b3bb62 Roll src/third_party/skia 3aca39df7b22..8429c7930291 (5 commits) (flutter/engine#6647) 26e8840 Dart SDK roll for 2018-10-23 9829c51 Roll src/third_party/skia b46c4d0925ad..3aca39df7b22 (12 commits) (flutter/engine#6643) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC&flutter#39;d on the roll, and stop the roller if necessary.

googlebot added the cla: yes label Oct 17, 2018

amirh requested a review from chinmaygarde October 17, 2018 17:04

amirh force-pushed the platform_views_controller branch from 335e125 to 1dc1968 Compare October 17, 2018 17:06

dnfield reviewed Oct 17, 2018

View reviewed changes

amirh force-pushed the platform_views_controller branch 2 times, most recently from 7228ff0 to c2f0487 Compare October 17, 2018 18:26

dnfield reviewed Oct 18, 2018

View reviewed changes

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h Outdated Show resolved Hide resolved

dnfield reviewed Oct 18, 2018

View reviewed changes

shell/platform/darwin/ios/framework/Headers/FlutterPlatformViews.h Show resolved Hide resolved

amirh mentioned this pull request Oct 18, 2018

Plumb the iOS PlatformViewsController into flow. #6603

Merged

chinmaygarde reviewed Oct 23, 2018

View reviewed changes

amirh commented Oct 23, 2018

View reviewed changes

amirh added 4 commits October 23, 2018 14:48

make PlatformViewsController a C++ class

c8535bb

review comments followup and expose viewId to the plugin

e9551d7

review comments followup

f62c939

amirh force-pushed the platform_views_controller branch from efccad2 to f62c939 Compare October 23, 2018 21:49

chinmaygarde approved these changes Oct 24, 2018

View reviewed changes

review comments followup

01bce7b

amirh merged commit 9669b70 into flutter:master Oct 24, 2018

amirh deleted the platform_views_controller branch October 24, 2018 22:07

engine-flutter-autoroll mentioned this pull request Oct 25, 2018

Roll engine 2586e9412257..9f2e2ba7b9d9 (10 commits) flutter/flutter#23486

Closed

This was referenced Oct 25, 2018

Roll engine 2586e9412257..914551bfa46c (11 commits) flutter/flutter#23488

Closed

Roll engine 2586e9412257..ff0525f59486 (13 commits) flutter/flutter#23505

Closed

engine-flutter-autoroll mentioned this pull request Oct 25, 2018

Roll engine 2586e9412257..a6e816b2a67c (14 commits) flutter/flutter#23513

Merged

Add an iOS PlatformViewsController for creating/disposing UIViews. #6569

Add an iOS PlatformViewsController for creating/disposing UIViews. #6569

Uh oh!

Conversation

amirh commented Oct 17, 2018

Uh oh!

amirh commented Oct 17, 2018

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!

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!

amirh 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!

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!

chinmaygarde 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development