State Restoration for iOS by goderbauer · Pull Request #23495 · flutter/engine

goderbauer · 2021-01-07T19:48:16Z

Description

Wires up state restoration in the iOS embedder. The same has been done for Android in #18042.

Related Issues

Fixes flutter/flutter#62915.

Tests

I added the following tests:

Test for the new RestorationPlugin

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.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

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

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

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

cbracken

A few initial comments. Overall looks good. @gaaclarke for the OCMock test, which I'm less familiar with.

cbracken · 2021-01-13T21:25:23Z

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

+#pragma mark - State Restoration
+
+- (BOOL)application:(UIApplication*)application shouldSaveApplicationState:(NSCoder*)coder {
+  [coder encodeInt64:self.lastAppModificationTime forKey:@"mod-date"];


Optional: not used much but consider yanking this to a const. Sadly, can't use constexpr since it's an NSString.

Well it is duplicated across the file so I think it's a good idea.

cbracken · 2021-01-13T21:27:10Z

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

+
+#import <UIKit/UIKit.h>
+
+#include "flutter/fml/memory/weak_ptr.h"


Doesn't look like you're using weak_ptr here (anymore).

Removed import.

cbracken · 2021-01-13T21:29:37Z

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

+             restorationEnabled:(BOOL)waitForData NS_DESIGNATED_INITIALIZER;
+
+- (NSData*)restorationData;
+- (void)restorationData:(NSData*)data;


This should be - (void)setRestorationData:(NSData*)data;. The getter is named correctly.

Use a property as previously mentioned.

cbracken · 2021-01-13T21:32:13Z

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

+
+- (NSData*)restorationData;
+- (void)restorationData:(NSData*)data;
+- (void)restorationComplete;


This should be setRestorationComplete or similar.

Ref: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingMethods.html

cbracken · 2021-01-13T21:35:45Z

Added @gaaclarke for additional set of eyes, particularly around the OCMock test.

gaaclarke

The tests look good to me. I just had some comments mostly around non-idiomatic code and the usage of properties.

gaaclarke · 2021-01-13T21:48:39Z

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

+  NSDate* fileDate;
+  [[[NSBundle mainBundle] executableURL] getResourceValue:&fileDate
+                                                   forKey:NSURLContentModificationDateKey
+                                                    error:nil];


We should probably not gobble up this error. Its worth having an assert at least.

I missed this comment earlier, will fix after meeting.

gaaclarke · 2021-01-13T21:51:25Z

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

+- (instancetype)initWithChannel:(FlutterMethodChannel*)channel
+             restorationEnabled:(BOOL)waitForData NS_DESIGNATED_INITIALIZER;
+
+- (NSData*)restorationData;


Idiomatic objc would use a property for this.

changed to property.

gaaclarke · 2021-01-13T21:52:13Z

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

+             restorationEnabled:(BOOL)waitForData NS_DESIGNATED_INITIALIZER;
+
+- (NSData*)restorationData;
+- (void)restorationData:(NSData*)data;


Use a property as previously mentioned.

gaaclarke · 2021-01-13T21:53:38Z

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

+// found in the LICENSE file.
+
+#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterRestorationPlugin.h"
+


Please assert ARC or non ARC at the top. There are macros in FlutterMacros.

You mean FLUTTER_ASSERT_NOT_ARC or FLUTTER_ASSERT_ARC? In existing code I am only seeing those used in test code. Should they also appear in this non-test code?

Added FLUTTER_ASSERT_NOT_ARC.

gaaclarke · 2021-01-13T21:55:09Z

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

+  BOOL _waitForData;
+  BOOL _restorationEnabled;
+  FlutterResult _pendingRequest;
+  NSData* _restorationData;


You need to add a dealloc to this class that deletes this.

gaaclarke · 2021-01-13T21:56:05Z

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

+  if ([[call method] isEqualToString:@"put"]) {
+    FlutterStandardTypedData* data = [call arguments];
+    NSData* newData = [[data data] retain];
+    if (_restorationData != nil) {


If you use properties you don't have to do this manual reference counting work.

gaaclarke · 2021-01-13T21:56:33Z

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

+    }
+    _restorationData = newData;
+    result(nil);
+  } else if ([[call method] isEqualToString:@"get"]) {


There is no call to result in this method.

The call to result is delayed until we have something to send over, see _pendingResult.

gaaclarke · 2021-01-13T21:57:03Z

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

+  if (_restorationData != nil) {
+    [_restorationData release];
+  }
+  _restorationData = newData;


Another place where properties will make your life easier.

gaaclarke · 2021-01-13T22:01:10Z

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

+  XCTAssertNil([restorationPlugin restorationData]);
+
+  __block id capturedResult;
+  methodCall = [FlutterMethodCall methodCallWithMethodName:@"get" arguments:nil];


Optional: It would be nice if these method names where placed as consts in the header so we don't have to duplicate them. No one should have to refer to them by string literal.

goderbauer

Thanks for the review. Addressed all comments. PTAL.

goderbauer · 2021-01-13T22:04:43Z

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

+#pragma mark - State Restoration
+
+- (BOOL)application:(UIApplication*)application shouldSaveApplicationState:(NSCoder*)coder {
+  [coder encodeInt64:self.lastAppModificationTime forKey:@"mod-date"];


goderbauer · 2021-01-13T22:05:11Z

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

+
+#import <UIKit/UIKit.h>
+
+#include "flutter/fml/memory/weak_ptr.h"


Removed import.

goderbauer · 2021-01-13T22:05:51Z

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

+             restorationEnabled:(BOOL)waitForData NS_DESIGNATED_INITIALIZER;
+
+- (NSData*)restorationData;
+- (void)restorationData:(NSData*)data;


goderbauer · 2021-01-13T22:05:56Z

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

+
+- (NSData*)restorationData;
+- (void)restorationData:(NSData*)data;
+- (void)restorationComplete;


goderbauer · 2021-01-13T22:19:24Z

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

+// found in the LICENSE file.
+
+#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterRestorationPlugin.h"
+


You mean FLUTTER_ASSERT_NOT_ARC or FLUTTER_ASSERT_ARC? In existing code I am only seeing those used in test code. Should they also appear in this non-test code?

goderbauer · 2021-01-13T22:27:08Z

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

+  if ([[call method] isEqualToString:@"put"]) {
+    FlutterStandardTypedData* data = [call arguments];
+    NSData* newData = [[data data] retain];
+    if (_restorationData != nil) {


goderbauer · 2021-01-13T22:27:47Z

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

+    }
+    _restorationData = newData;
+    result(nil);
+  } else if ([[call method] isEqualToString:@"get"]) {


The call to result is delayed until we have something to send over, see _pendingResult.

goderbauer · 2021-01-13T22:28:35Z

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

+  if (_restorationData != nil) {
+    [_restorationData release];
+  }
+  _restorationData = newData;


goderbauer · 2021-01-13T22:34:40Z

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

+  BOOL _waitForData;
+  BOOL _restorationEnabled;
+  FlutterResult _pendingRequest;
+  NSData* _restorationData;


goderbauer · 2021-01-13T22:37:05Z

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

+// found in the LICENSE file.
+
+#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterRestorationPlugin.h"
+


Added FLUTTER_ASSERT_NOT_ARC.

gaaclarke

Thanks looking much better, lgtm modulo changing pendingRequest to a property as well.

PS declare the property in the .mm file with a class extension.

gaaclarke · 2021-01-13T23:20:22Z

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

 static NSString* kUIBackgroundMode = @"UIBackgroundModes";
 static NSString* kRemoteNotificationCapabitiliy = @"remote-notification";
 static NSString* kBackgroundFetchCapatibility = @"fetch";
+static NSString* kRestorationStateAppModificationKey = @"mod-date";


These should really be static NSString* const, sorry I know you didn't set the precedent =(

gaaclarke · 2021-01-13T23:22:05Z

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

+- (instancetype)initWithChannel:(FlutterMethodChannel*)channel
+             restorationEnabled:(BOOL)waitForData NS_DESIGNATED_INITIALIZER;
+
+@property(nonatomic, retain) NSData* restorationData;


s/retain/strong for forward compatibility with ARC

gaaclarke · 2021-01-13T23:23:48Z

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

+}
+
+- (void)dealloc {
+  if (_restorationData != nil) {


there is no need to do a nil check here, calls to nil are ignored in objective-c

gaaclarke · 2021-01-13T23:24:44Z

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

+      result([self dataForFramework]);
+      return;
+    }
+    _pendingRequest = [result retain];


This is a leak, you should switch pendingRequest to a property and used self.pendingRequest = result; The property should be copy for blocks. In more cases your code will clean up once you turn this to a property, too. Sorry I didn't catch it earlier.

gaaclarke

LGTM thanks!

cbracken

lgtm!

fluttergithubbot · 2021-01-14T01:29:02Z

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

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

fluttergithubbot · 2021-01-14T19:09:02Z

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

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

flutter-dashboard bot added the platform-ios label Jan 7, 2021

google-cla bot added the cla: yes label Jan 7, 2021

goderbauer force-pushed the iosrestoration branch from 3c945f4 to 9c9931a Compare January 7, 2021 20:14

goderbauer changed the title ~~Iosrestoration~~ State Restoration for iOS Jan 7, 2021

goderbauer requested a review from cbracken January 7, 2021 21:23

thecalamiity reviewed Jan 7, 2021

View reviewed changes

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

thecalamiity reviewed Jan 7, 2021

View reviewed changes

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

goderbauer mentioned this pull request Jan 8, 2021

Add ios and hot reload/restart infos to restoration docs flutter/flutter#73585

Merged

cbracken reviewed Jan 13, 2021

View reviewed changes

cbracken requested a review from gaaclarke January 13, 2021 21:35

gaaclarke reviewed Jan 13, 2021

View reviewed changes

goderbauer commented Jan 13, 2021

View reviewed changes

gaaclarke reviewed Jan 13, 2021

View reviewed changes

goderbauer requested a review from gaaclarke January 14, 2021 00:15

gaaclarke approved these changes Jan 14, 2021

View reviewed changes

cbracken approved these changes Jan 14, 2021

View reviewed changes

goderbauer 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 Jan 14, 2021

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 Jan 14, 2021

goderbauer force-pushed the iosrestoration branch from e6fee6c to 7c2858f Compare January 14, 2021 18:07

goderbauer 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 Jan 14, 2021

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 Jan 14, 2021

goderbauer 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 Jan 14, 2021

goderbauer added 5 commits January 14, 2021 13:26

almost compiles

8c5ace4

it works

fb1272b

Reset on hot restart

4901439

tests

6b8fac9

cleanup

0fa6966

goderbauer added 9 commits January 14, 2021 13:26

licenses

b12df56

format

4f1676a

remove import

5390cd4

format

e562956

review comments

35d059e

review

968e876

format

5559926

another review

fbcb797

format

c08e07f

goderbauer force-pushed the iosrestoration branch from 7c2858f to c08e07f Compare January 14, 2021 21:27

merge conflict

78fe7c5

fluttergithubbot merged commit 0123541 into flutter:master Jan 14, 2021

engine-flutter-autoroll mentioned this pull request Jan 15, 2021

Roll Engine from ee2ac2a5afc3 to 0123541c394c (7 revisions) flutter/flutter#74004

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2021

0123541 State Restoration for iOS (flutter/engine#23495)

015ac69

goderbauer mentioned this pull request Jan 15, 2021

Enable State Restoration for iOS flutter/samples#647

Merged

gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 20, 2021

State Restoration for iOS (flutter#23495)

d2c6732

goderbauer deleted the iosrestoration branch January 22, 2021 17:59

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

State Restoration for iOS (flutter#23495)

5174ace

zhongwuzw mentioned this pull request Mar 14, 2022

[iOS] Fixes state restoration not work #31511

Merged

8 tasks

cyanglaz mentioned this pull request Oct 27, 2022

Migrate darwin common "framework_shared" target to ARC #37049

Merged

8 tasks


		#import <UIKit/UIKit.h>

		#include "flutter/fml/memory/weak_ptr.h"

		// found in the LICENSE file.

		#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterRestorationPlugin.h"

Conversation

goderbauer commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

Uh oh!

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!

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!

cbracken commented Jan 13, 2021

Uh oh!

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

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!

goderbauer 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

goderbauer commented Jan 7, 2021 •

edited

Loading