Add firstFrameDidRender to FlutterViewController #10816

dnfield · 2019-08-09T23:36:58Z

Earl Grey wants this to be able to more reliably/sensibly detect when the rendering is read. Android offers something somewhat similar to this - tagged @matthew-carroll to help evaluate consistency there.

/cc @AlbertWang0116

dnfield · 2019-08-09T23:42:56Z

/cc @gaaclarke who may find this interesting when you get back

jmagman · 2019-08-10T00:03:29Z

testing/scenario_app/ios/Scenarios/ScenariosTests/FlutterViewControllerTest.m

+  [rootVC presentViewController:self.flutterViewController animated:NO completion:nil];
+  NSRunLoop* runLoop = [NSRunLoop currentRunLoop];
+  int countDownMs = 2000;
+  while (!flutterViewController.firstFrameRendered && countDownMs > 0) {


You should almost never have to manually spin the main run loop in tests. The above testFirstFrameCallback test should be:

XCTestExpectation *callbackExpecation = [self expectationWithDescription:@"callback"]; [self.flutterViewController setFlutterViewDidRenderCallback:^{ [callbackExpecation fulfill]; }]; ... [rootVC presentViewController:self.flutterViewController animated:NO completion:nil]; [self waitForExpectationsWithTimeout:2.0 handler:nil];

For this one you can use a XCTKVOExpectation. Something like (I didn't test this):

XCTestExpectation *firstFrameRenderedExpecation = [self keyValueObservingExpectationForObject:flutterViewController keyPath:@"setFirstFrameRendered" expectedValue:@YES]; ... [rootVC presentViewController:self.flutterViewController animated:NO completion:nil]; [self waitForExpectationsWithTimeout:2.0 handler:nil];

Or better yet, can you get rid of firstFrameDidRender and setFirstFrameRendered if it's just for this purpose, and fulfill the expectation however _flutterViewRenderedCallback is set (I don't know how _flutterViewRenderedCallback works).

What is this actually testing that's different than the above test? It looks like they are both checking that the _flutterViewRenderedCallback was called.

jmagman · 2019-08-10T00:06:41Z

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

+ * This method is called before the callback set by any previous call to
+ * `setFlutterViewDidRenderCallback` is called, if any.
+ */
+- (void)firstFrameDidRender;


It seems like this is just for tests. Can you just detect the -callViewRenderedCallback message in the test instead?

@AlbertWang0116 how do you feel about this becoming a readonly BOOL?

@property(nonatomic, readonly) BOOL firstFrameRendered;

It works. But if the setter is ensured being called symmetrically (NO->YES->NO->YES...), EarlGrey can perform a more optimal event-based (other than polling-based) synchronization that wait for the Flutter app to be loaded.

I think this is the case for now, and if we would like to guarantee that, probably add a doc-string to the readwrite version property saying that it should be assigned with NO/YES symmetrically? (So it can be the reference for future update on this class)

@dnfield If needed, you can accomplish this KVO contract with a backing instance variable without hand-shake documentation. Note with a getter the KVO key to observe on the unit test/Earl Grey side is renderingFrames and not isRenderingFrames (common mistake).

@interface FlutterViewController @property (nonatomic, readonly, getter = isRenderingFrames) BOOL renderingFrames; @end

@interface FlutterViewController () <FlutterBinaryMessenger> @property (nonatomic, readwrite, getter = isRenderingFrames) BOOL renderingFrames; @end

@implementation FlutterViewController { ... } @synthesize renderingFrames = _renderingFrames;

+ (BOOL)automaticallyNotifiesObserversForRenderingFrames { return NO; } - (void)setRenderingFrames:(BOOL)renderingFrames { if (_renderingFrames != renderingFrames) { [self willChangeValueForKey:@"renderingFrames"]; _renderingFrames = renderingFrames; [self didChangeValueForKey:@"renderingFrames"]; } }

or you can stick to isRenderingFrames everywhere if you don't want to deal with the confusion of the property getter.

jmagman · 2019-08-10T00:25:57Z

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

@@ -264,6 +264,7 @@ - (void)installSplashScreenViewIfNecessary {
 }

 - (void)callViewRenderedCallback {
+  [self firstFrameDidRender];


In here in the implementation would be an interface:
@property(nonatomic, readwrite) BOOL firstFrameRendered;
and here would be:
self.firstFrameRendered = YES;

One thing we need to be able to do is set this and unset it each time the viewcontroller appears/disappears. Will that still work with a property?

Assuming the property is always set in the view controller implementation, then yes.

In that case a better name might be isRenderingFrames. If that works for @AlbertWang0116 I'll make that change.

Just went ahead and made them - I think this makes themost sense.

isRenderingFrames sounds to me that it changes the value for each frame lifecycle. But it actually indicates the first frame (after view controller being loaded) is rendering?

It will be set to YES after at least one frame has rendered after showing the view controller, and set to now upon dismissing the view controller.

Right now, it's not guaranteed to only go YES / NO / YES / NO but it sounds like Jenn's suggestion could fix that.

jmagman · 2019-08-10T00:32:59Z

jmagman deleted a comment from xster 5 minutes ago

🤦‍♀ that was an accident. @xster said:

In testing/scenario_app/ios/Scenarios/ScenariosTests/FlutterViewControllerTest.m:

> @@ -6,6 +6,16 @@
 #import <XCTest/XCTest.h>
 #import "AppDelegate.h"
 
+@interface FlutterViewControllerFirstFrameOverride : FlutterViewController
+@property(nonatomic) BOOL firstFrameRendered;
+@end
+
+@implementation FlutterViewControllerFirstFrameOverride
+- (void)firstFrameDidRender {

\o/

That's my general feedback too. Either make it actually functional in production like on Android or make it test only and private.

dnfield · 2019-08-10T03:41:17Z

In that case a better name might be `isRenderingFrames`. If that works for Albert, I'll make that change.

…

On Fri, Aug 9, 2019 at 8:35 PM jmagman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In shell/platform/darwin/ios/framework/Source/FlutterViewController.mm <#10816 (comment)>: > @@ -264,6 +264,7 @@ - (void)installSplashScreenViewIfNecessary { } - (void)callViewRenderedCallback { + [self firstFrameDidRender]; Assuming the property is always set in the view controller implementation, then yes. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#10816?email_source=notifications&email_token=ACBYVROLFD4NOQYMNRDUUOLQDYZRPA5CNFSM4IKX7ZTKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBF6PEI#discussion_r312688799>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBYVRL7GRYXRINXC54VYMLQDYZRPANCNFSM4IKX7ZTA> .

…red_ios

jmagman · 2019-08-10T06:18:05Z

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

@@ -395,6 +397,7 @@ - (void)surfaceUpdated:(BOOL)appeared {
    [_engine.get() platformViewsController] -> SetFlutterViewController(self);


I don't know this code, but would it make more sense to set isRenderingFrames to YES in the if of this statement to make it symmetrical? It seems like an unexpected side-effect for (void)callViewRenderedCallback to set it.

We present the surface before we actually are rendering frames

I have to double check this - I'm realizing this might not get invoked again in a serize of push/pop/push

No, it's ok - we install the first frame callback every time we appear.

jmagman · 2019-08-10T06:20:42Z

testing/scenario_app/ios/Scenarios/ScenariosTests/FlutterViewControllerTest.m

+
+  XCTAssertFalse(self.flutterViewController.isRenderingFrames);
+
+  [self keyValueObservingExpectationForObject:self.flutterViewController


I forget the exact API but there's another one that let's you set the expected value as a parameter to the expectation. @YES will probably work even though it's a primitive, though I might be wrong. Then you won't need the assert true at the end.

jmagman

LGTM!

xster · 2019-08-13T00:38:47Z

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

+ * True if at least one frame has rendered and the ViewController has appeared.
+ *
+ * This property is reset to false when the ViewController disappears.
+ */


I think this now does the same thing as what io.flutter.embedding.android.hasRenderedFirstFrame() does. Though it's named differently :D

Can we keep the names consistent?

I'm not sure that name works here. This can change from true to false depending on State and back to true again.

Android does that too on the same instance. Though you could made an argument to change the Android name too.

cc @matthew-carroll

+1 for consistency. Or isRenderingFirstFrame, which can be used on both iOS and Android?

I don't think isRenderingFirstFrame is quite right either - this currently works as indicating the state as to whether Flutter has rendered at least one frame since the view has become visible, or if the view is not visible and thus no frames will be rendered.

renderingFrames seems problematic for multiple reasons. First, it is easily confused with a noun, e.g., these are some frames that are being rendered. Second, it doesn't have any sense of temporal context in the name. I think that hasRenderedFirstFrame() implies a somewhat specific place in time. There was a time before any frames were rendered, and then a time after the first frame was rendered, and this method tells if that moment has passed, as opposed to the somewhat ambiguous on-going "time of rendering frames". I think it's totally fine that hasRenderedFirstFrame() can toggle between true/false, so long as the reason is documented. That toggle is not arbitrary, it only occurs when specific resets or detachments occur. In other words a singular Flutter experience will never flip that value from true to false - only introducing a new Flutter experience, or detaching and re-attaching the original Flutter experience will result in that flag flipping, which coincides with Flutter's visual display disappearing and later re-appearing, thus repeating the concept of a "1st frame".

This part: "In other words a singular Flutter experience will never flip that value from true to false - only introducing a new Flutter experience, or detaching and re-attaching the original Flutter experience " really confused me. Let's discuss offline

The scenario most on my mind for this is showing the camera.

FlutterViewController appears -> isRenderingFrames = NO
Frame callback fires -> isRenderingFrames = YES
Camera starts -> isRenderingFrames = NO
Camera goes away -> isRenderingFrames = YES

In this situation, I find it confusing to think about which frame is the "first" frame, and I don't want to know if a frame "has" rendered - I want to know if at least one frame has rendered and Flutter could still potentially continue to render.

Updated property name based on discussion - PTAL

xster · 2019-08-13T00:39:38Z

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

+ *
+ * This property is reset to false when the ViewController disappears.
+ */
+@property(nonatomic, readonly) BOOL isRenderingFrames;


See above. But if anything, your doc is in the past tense so this would be hasRenderedFrames. (At which point, we might as well go hasRenderedFirstFrame :D)

xster · 2019-08-13T00:43:56Z

Would be nice to get the names consistent but otherwise LGTM

…red_ios

jmagman · 2019-08-13T04:03:07Z

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

 - (void)callViewRenderedCallback {
-  self.isRenderingFrames = YES;
+  [self setRenderingFrames:YES];


Still use dot notation for properties: self.renderingFrames = YES

AlbertWang0116 · 2019-08-13T04:53:11Z

LGTM. Thanks Dan!

xster · 2019-08-13T22:59:57Z

LGTM

jmagman · 2019-08-13T23:17:18Z

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

 - (void)callViewRenderedCallback {
+  self.isDisplayingFlutterUI = YES;


I hate to keep harping on this PR, but did we figure out if it's a problem that this isn't set in a queue-confined way to guarantee YES -> NO -> YES -> NO?

No - I think the bigger problem is that we're not guaranteed to call this accurately though. I'll think about this some more.

fixed in c6923c6

dnfield · 2019-08-14T16:10:07Z

I'm going to go ahead and land this - Chinmay worked with me on the latest code offline, I bleieve I've addressed all feedback, and I have a few LGTMs :)

If anyone has any follow ups on this, I'm happy to open a new CL for it.

…ngine#10816)

…ngine#10816) (#38553) [email protected]:flutter/engine.git/compare/d75af60810f6...90656d8 git log d75af60..90656d8 --no-merges --oneline 2019-08-14 [email protected] Add isDisplayingFlutterUI to FlutterViewController (flutter/engine#10816) 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 ([email protected]), and stop the roller if necessary.

Add firstFrameDidRender to FlutterViewController

7ef626a

dnfield requested review from chinmaygarde, xster and matthew-carroll August 9, 2019 23:36

googlebot added cla: yes labels Aug 9, 2019

dnfield requested a review from jmagman August 9, 2019 23:53

jmagman requested changes Aug 10, 2019

View reviewed changes

jmagman reviewed Aug 10, 2019

View reviewed changes

flutter deleted a comment from xster Aug 10, 2019

dnfield added 4 commits August 9, 2019 22:22

Merge remote-tracking branch 'upstream/master' into first_frame_rende…

0714141

…red_ios

Review

bcac5d7

expect kvo set

3a45160

fix order

c75d125

jmagman reviewed Aug 10, 2019

View reviewed changes

expectedValue

4d086b4

jmagman approved these changes Aug 10, 2019

View reviewed changes

format

7f75757

xster reviewed Aug 13, 2019

View reviewed changes

dnfield added 2 commits August 12, 2019 20:27

Merge remote-tracking branch 'upstream/master' into first_frame_rende…

bc567ee

…red_ios

property guarantees

f34d20e

jmagman reviewed Aug 13, 2019

View reviewed changes

property usage

efb1aec

rename prop, fix metaprogramming

8b40fc0

fml_dlog remove

2046e2d

format

d8fec4e

jmagman reviewed Aug 13, 2019

View reviewed changes

fix potential race

c6923c6

dnfield merged commit 90656d8 into flutter:master Aug 14, 2019

dnfield deleted the first_frame_rendered_ios branch August 14, 2019 16:10

engine-flutter-autoroll mentioned this pull request Aug 14, 2019

Roll engine d75af60810f6..90656d88244d (1 commits) flutter/flutter#38553

Merged

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

90656d8 Add isDisplayingFlutterUI to FlutterViewController (flutter/e…

ec1c67e

…ngine#10816)

		@@ -395,6 +397,7 @@ - (void)surfaceUpdated:(BOOL)appeared {
		[_engine.get() platformViewsController] -> SetFlutterViewController(self);


		XCTAssertFalse(self.flutterViewController.isRenderingFrames);

		[self keyValueObservingExpectationForObject:self.flutterViewController

		- (void)callViewRenderedCallback {
		self.isDisplayingFlutterUI = YES;

Add firstFrameDidRender to FlutterViewController #10816

Add firstFrameDidRender to FlutterViewController #10816

Conversation

dnfield commented Aug 9, 2019

dnfield commented Aug 9, 2019

jmagman Aug 10, 2019 • edited Loading

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

jmagman Aug 13, 2019 • edited Loading

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

dnfield Aug 10, 2019 • edited Loading

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

jmagman commented Aug 10, 2019 • edited Loading

dnfield commented Aug 10, 2019 via email

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

jmagman 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

matthew-carroll Aug 13, 2019 • edited Loading

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 commented Aug 13, 2019

Choose a reason for hiding this comment

AlbertWang0116 commented Aug 13, 2019

xster commented Aug 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented Aug 14, 2019

jmagman Aug 10, 2019 •

edited

Loading

jmagman Aug 13, 2019 •

edited

Loading

dnfield Aug 10, 2019 •

edited

Loading

jmagman commented Aug 10, 2019 •

edited

Loading

matthew-carroll Aug 13, 2019 •

edited

Loading