Refactor initial route code #19684

xster · 2020-07-12T00:52:42Z

Description

Fix flutter/flutter#59895 and persistent UX issues plaguing users with confusing behavior around setInitialRoute.

Deprecate after-the-fact setInitialRoute calls which can easily have no effect. Migrate them as much as possible to constructors.

I'll update the website once this hits stable

Related Issues

Fixes flutter/flutter#59895

Tests

I added the following tests:

Added unit test for engine, integration test for view controller.

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

gaaclarke · 2020-07-13T17:47:05Z

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

+ * @param nibBundle The NIB bundle.
+ */
+- (instancetype)initWithProject:(nullable FlutterDartProject*)project
+               withInitialRoute:(nullable NSString*)initialRoute


s/withInitialRoute/intialRoute

ah yes, agree

gaaclarke · 2020-07-13T17:48:07Z

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

@@ -54,6 +55,7 @@ @implementation FlutterEngine {
  flutter::ThreadHost _threadHost;
  std::unique_ptr<flutter::Shell> _shell;
  NSString* _labelPrefix;
+  NSString* _initialRoute;


This is leaking, please use a property with copy semantics.

how do you mean? We're manually copying and releasing it no?

The releasing of it is conditional. There is still the opportunity for it to leak. It should be cleaned up in the dealloc.

We are copying it, using the property syntax just makes the memory management technique apparent.

Ah understood. PTAL

lookin good

gaaclarke · 2020-07-13T17:51:38Z

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

@@ -367,6 +373,12 @@ - (void)setupChannels {
      binaryMessenger:self.binaryMessenger
                codec:[FlutterJSONMethodCodec sharedInstance]]);

+  if ([_initialRoute length] > 0) {
+    // Flutter isn't ready to receive this method call yet but the channel buffer will cache this.
+    [_navigationChannel invokeMethod:@"setInitialRoute" arguments:_initialRoute];


You should probably use the variant of invokeMethod that does error checking and print out if there is an error.

oh true. I agree. Though it looks like we're using the non-callback variant across the embedding code currently. I'd like to keep it consistent and move all call sites if we do.

gaaclarke · 2020-07-13T17:52:19Z

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

+  if ([_initialRoute length] > 0) {
+    // Flutter isn't ready to receive this method call yet but the channel buffer will cache this.
+    [_navigationChannel invokeMethod:@"setInitialRoute" arguments:_initialRoute];
+    [_initialRoute release];


Don't release something without niling it out.

ah yes, thanks

gaaclarke · 2020-07-13T17:54:26Z

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


 FLUTTER_ASSERT_ARC

 @interface FlutterEngineTest : XCTestCase
 @end

+@interface FlutterEngine (Test) <FlutterBinaryMessenger>


Put this category in a headers so tests don't have to redeclare this.

what implementation style were you thinking? Having something like Source/FlutterEngine.mm, Source/FlutterEngine_Internal.h, Source/FlutterEngine_TestFlutterBinaryMessenger.h, Source/FlutterEngine_TestSomeOtherProtocol.h, Source/FlutterEngineTest.mm?

We might end up with a big list since no other test is using this specific category yet (Y(probably)AGNI).

We can also put all "internals" into one header, but it's not very least-privilege-principle.

I think FlutterEngine_Test sounds fine. It would operate like java's visiblefortesting. Everything can just go in there.

I'd say FlutterEngine_Test is like visiblefortesting and FlutterEngine_internal is like package visible methods.

gaaclarke · 2020-07-13T17:54:48Z

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


 FLUTTER_ASSERT_ARC

 @interface FlutterEngineTest : XCTestCase
 @end

+@interface FlutterEngine (Test) <FlutterBinaryMessenger>
+- (void)setBinaryMessenger:binaryMessenger;


Declare types.

oops, thanks

gaaclarke · 2020-07-13T17:55:21Z

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

-                        nibName:(nullable NSString*)nibName
-                         bundle:(nullable NSBundle*)nibBundle {
+- (instancetype)initWithProject:(FlutterDartProject*)project
+               withInitialRoute:(NSString*)initialRoute


s/withInitialRoute/initialRoute

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

gaaclarke · 2020-07-13T17:57:19Z

testing/ios/IosUnitTests/IosUnitTests.xcodeproj/project.pbxproj

+		0AC232F424BA71D300A85907 /* SemanticsObjectTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = SemanticsObjectTest.mm; sourceTree = "<group>"; };
+		0AC232F724BA71D300A85907 /* FlutterEngineTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterEngineTest.mm; sourceTree = "<group>"; };
+		0AC2330324BA71D300A85907 /* accessibility_bridge_test.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = accessibility_bridge_test.mm; sourceTree = "<group>"; };
+		0AC2330B24BA71D300A85907 /* FlutterTextInputPluginTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FlutterTextInputPluginTest.m; sourceTree = "<group>"; };
+		0AC2330F24BA71D300A85907 /* FlutterBinaryMessengerRelayTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterBinaryMessengerRelayTest.mm; sourceTree = "<group>"; };
+		0AC2331024BA71D300A85907 /* connection_collection_test.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = connection_collection_test.mm; sourceTree = "<group>"; };
+		0AC2331224BA71D300A85907 /* FlutterEnginePlatformViewTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterEnginePlatformViewTest.mm; sourceTree = "<group>"; };
+		0AC2331924BA71D300A85907 /* FlutterPluginAppLifeCycleDelegateTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FlutterPluginAppLifeCycleDelegateTest.m; sourceTree = "<group>"; };
+		0AC2332124BA71D300A85907 /* FlutterViewControllerTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterViewControllerTest.mm; sourceTree = "<group>"; };


Is this for easy navigation? It's hard to read the xcode project. I'm ok with this but jsut a heads up that the less there is in the xcode project the less there is to conflict.

ya, this is pretty optional. I don't feel strongly either way. This is just so people can look at the test they're about to run in this project before they run it (and potentially set breakpoints).

xster

Thanks for review. Sorry for the slow turnaround.

xster · 2020-07-31T19:47:40Z

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

+ * @param nibBundle The NIB bundle.
+ */
+- (instancetype)initWithProject:(nullable FlutterDartProject*)project
+               withInitialRoute:(nullable NSString*)initialRoute


ah yes, agree

xster · 2020-07-31T19:52:17Z

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

@@ -54,6 +55,7 @@ @implementation FlutterEngine {
  flutter::ThreadHost _threadHost;
  std::unique_ptr<flutter::Shell> _shell;
  NSString* _labelPrefix;
+  NSString* _initialRoute;


how do you mean? We're manually copying and releasing it no?

xster · 2020-07-31T19:54:51Z

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

+  if ([_initialRoute length] > 0) {
+    // Flutter isn't ready to receive this method call yet but the channel buffer will cache this.
+    [_navigationChannel invokeMethod:@"setInitialRoute" arguments:_initialRoute];
+    [_initialRoute release];


ah yes, thanks

xster · 2020-07-31T20:12:40Z

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

@@ -367,6 +373,12 @@ - (void)setupChannels {
      binaryMessenger:self.binaryMessenger
                codec:[FlutterJSONMethodCodec sharedInstance]]);

+  if ([_initialRoute length] > 0) {
+    // Flutter isn't ready to receive this method call yet but the channel buffer will cache this.
+    [_navigationChannel invokeMethod:@"setInitialRoute" arguments:_initialRoute];


oh true. I agree. Though it looks like we're using the non-callback variant across the embedding code currently. I'd like to keep it consistent and move all call sites if we do.

xster · 2020-07-31T20:19:45Z

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


 FLUTTER_ASSERT_ARC

 @interface FlutterEngineTest : XCTestCase
 @end

+@interface FlutterEngine (Test) <FlutterBinaryMessenger>


what implementation style were you thinking? Having something like Source/FlutterEngine.mm, Source/FlutterEngine_Internal.h, Source/FlutterEngine_TestFlutterBinaryMessenger.h, Source/FlutterEngine_TestSomeOtherProtocol.h, Source/FlutterEngineTest.mm?

We might end up with a big list since no other test is using this specific category yet (Y(probably)AGNI).

We can also put all "internals" into one header, but it's not very least-privilege-principle.

xster · 2020-07-31T20:20:38Z

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


 FLUTTER_ASSERT_ARC

 @interface FlutterEngineTest : XCTestCase
 @end

+@interface FlutterEngine (Test) <FlutterBinaryMessenger>
+- (void)setBinaryMessenger:binaryMessenger;


oops, thanks

xster · 2020-07-31T20:21:46Z

testing/ios/IosUnitTests/IosUnitTests.xcodeproj/project.pbxproj

+		0AC232F424BA71D300A85907 /* SemanticsObjectTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = SemanticsObjectTest.mm; sourceTree = "<group>"; };
+		0AC232F724BA71D300A85907 /* FlutterEngineTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterEngineTest.mm; sourceTree = "<group>"; };
+		0AC2330324BA71D300A85907 /* accessibility_bridge_test.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = accessibility_bridge_test.mm; sourceTree = "<group>"; };
+		0AC2330B24BA71D300A85907 /* FlutterTextInputPluginTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FlutterTextInputPluginTest.m; sourceTree = "<group>"; };
+		0AC2330F24BA71D300A85907 /* FlutterBinaryMessengerRelayTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterBinaryMessengerRelayTest.mm; sourceTree = "<group>"; };
+		0AC2331024BA71D300A85907 /* connection_collection_test.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = connection_collection_test.mm; sourceTree = "<group>"; };
+		0AC2331224BA71D300A85907 /* FlutterEnginePlatformViewTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterEnginePlatformViewTest.mm; sourceTree = "<group>"; };
+		0AC2331924BA71D300A85907 /* FlutterPluginAppLifeCycleDelegateTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FlutterPluginAppLifeCycleDelegateTest.m; sourceTree = "<group>"; };
+		0AC2332124BA71D300A85907 /* FlutterViewControllerTest.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = FlutterViewControllerTest.mm; sourceTree = "<group>"; };


ya, this is pretty optional. I don't feel strongly either way. This is just so people can look at the test they're about to run in this project before they run it (and potentially set breakpoints).

gaaclarke

LGTM modulo the initalRoute (potential) leak.

gaaclarke · 2020-08-07T19:58:01Z

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

+ *   FlutterDefaultInitialRoute (or nil), it will default to the "/" route.
+ * @return YES if the call succeeds in creating and running a Flutter Engine instance; NO otherwise.
+ */
+- (BOOL)runWithEntrypoint:(nullable NSString*)entrypoint


nit: You still have 2 prepositions in your selector, that's non-standard.

fluttergithubbot · 2020-08-10T18:16:03Z

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

The status or check suite Mac Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite web_tests-6-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

fluttergithubbot · 2020-08-11T21:21:02Z

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

The status or check suite Mac iOS Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

…ants, will need a bigger refactor to clean the build and test script later

This reverts commit 8d08e6c.

fluttergithubbot added the platform-ios label Jul 12, 2020

googlebot added the cla: yes label Jul 12, 2020

auto-assign bot requested a review from iskakaushik July 12, 2020 00:53

xster requested review from gaaclarke and chinmaygarde July 12, 2020 00:56

gaaclarke reviewed Jul 13, 2020

View reviewed changes

xster force-pushed the rework-setinitialroute branch from 870b9d0 to 93fe293 Compare July 31, 2020 20:23

xster commented Jul 31, 2020

View reviewed changes

gaaclarke approved these changes Jul 31, 2020

View reviewed changes

gaaclarke reviewed Aug 7, 2020

View reviewed changes

xster force-pushed the rework-setinitialroute branch from 6e6a79b to 4fa79b7 Compare August 10, 2020 16:48

xster added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 10, 2020

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 10, 2020

xster force-pushed the rework-setinitialroute branch from 0456c08 to cf93cc7 Compare August 11, 2020 18:38

xster added 13 commits August 11, 2020 11:49

refactored apis, before adding tests

de7ab76

added 2 tests

7434811

integration test

98edf96

autoformat

4cd4b56

review

4f76a60

review

448d020

forgot to rename a test selector

4daea7d

autoformat

27d67cc

remove with everywhere

ebc6984

autoformat

4f405c5

license

ca07a37

make CI script for scenario consistent between ios and host

de5ecba

fix test

08f4d63

xster force-pushed the rework-setinitialroute branch from cf93cc7 to 08f4d63 Compare August 11, 2020 19:16

xster added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 11, 2020

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 11, 2020

unfortunately luci expects a specific non-matching set of engine vari…

453631c

…ants, will need a bigger refactor to clean the build and test script later

xster merged commit 8d08e6c into flutter:master Aug 12, 2020

xster deleted the rework-setinitialroute branch August 12, 2020 02:52

engine-flutter-autoroll mentioned this pull request Aug 12, 2020

Roll Engine from ccfc7e5179db to 7571e7c380fd (2 revisions) flutter/flutter#63567

Merged

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

8d08e6c Refactor initial route code (flutter/engine#19684)

47ae274

gaaclarke added a commit to gaaclarke/engine that referenced this pull request Aug 12, 2020

Revert "Refactor initial route code (flutter#19684)"

c3b446a

This reverts commit 8d08e6c.

gaaclarke added a commit that referenced this pull request Aug 12, 2020

Revert "Refactor initial route code (#19684)" (#20450)

2ac5b34

This reverts commit 8d08e6c.

xster mentioned this pull request Aug 13, 2020

Refactor initial route code take 2 #20468

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor initial route code #19684

Refactor initial route code #19684

xster commented Jul 12, 2020 •

edited

Loading

gaaclarke Jul 13, 2020

xster Jul 31, 2020

gaaclarke Jul 13, 2020

xster Jul 31, 2020

gaaclarke Jul 31, 2020

xster Aug 7, 2020

gaaclarke Aug 7, 2020

gaaclarke Jul 13, 2020

xster Jul 31, 2020

gaaclarke Jul 13, 2020

xster Jul 31, 2020

gaaclarke Jul 13, 2020

xster Jul 31, 2020

gaaclarke Jul 31, 2020

gaaclarke Jul 13, 2020

xster Jul 31, 2020

gaaclarke Jul 13, 2020

gaaclarke Jul 13, 2020

xster Jul 31, 2020

xster left a comment

xster Jul 31, 2020

xster Jul 31, 2020

xster Jul 31, 2020

xster Jul 31, 2020

xster Jul 31, 2020

xster Jul 31, 2020

xster Jul 31, 2020

gaaclarke left a comment

gaaclarke Aug 7, 2020

xster Aug 10, 2020

fluttergithubbot commented Aug 10, 2020

fluttergithubbot commented Aug 11, 2020

Refactor initial route code #19684

Refactor initial route code #19684

Conversation

xster commented Jul 12, 2020 • edited Loading

Description

Related Issues

Tests

Checklist

Breaking Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluttergithubbot commented Aug 10, 2020

fluttergithubbot commented Aug 11, 2020

xster commented Jul 12, 2020 •

edited

Loading