added new lifecycle state #39945

chunhtai · 2019-09-05T23:58:03Z

Description

This pr is the framework counter part of flutter/engine#11913

Related Issues

Tests

I added the following tests:

Replace this with a list of the tests that you added as part of this PR. A change in behaviour with no test covering it
will likely get reverted accidentally sooner or later. PRs must include tests for all changed/updated/fixed behaviors. See Test Coverage.

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 Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

fluttergithubbot · 2019-09-05T23:58:05Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

packages/flutter/lib/src/scheduler/binding.dart

xster · 2019-09-07T00:05:44Z

LGTM otherwise

chunhtai · 2019-09-27T18:37:26Z

dev/integration_tests/ios_add2app_life_cycle/build_and_test.sh

remind myself need to remove local engine once engine change is in

chunhtai · 2019-09-27T18:43:20Z

dev/integration_tests/ios_add2app_life_cycle/flutterapp/lib/main.dart

@jmagman I copy the test from ios_add2app
The test is using XCTestCase to launch the app, and use tap a native button so that it will bring up a flutter view controller. The assertion is in dart side. Is there a way for dart code to notify the engine that the app has crashed?

Sorry I just saw this. I'm not sure now a dart assert expresses itself in an iOS process... does the app not crash, it just goes white or something?

chunhtai · 2019-09-30T20:53:08Z

ready for review, Added integration test ios_add2app_life_cycle
Here is how the test work.
When main.dart first run, it will cut off semantics hook to prevent flutter app sending semantics update notification to ios embedding.
the flutter app will create a widget that will spy on life cycle changes, and it will rewire semantics hook when it receives expected life cycle sequence. At this point, flutter app will start sending semantics update to ios embeding.

On the Native test side, it will initialize engine and bring up flutterviewcontroller. After that, it will be waiting to receive semantics update with a time out of 30 seconds.

If the flutter app does not receive correct life cycle sequence (detached -> inactive -> resumed), it will never rewire the semantics hook. The native app will timeout after 30 seconds and fail the test.
@goderbauer @jmagman

jmagman · 2019-10-03T19:50:14Z

...os_add2app_life_cycle/ios_add2app.xcodeproj/xcshareddata/xcschemes/ios_add2appTests.xcscheme

You can remove this scheme and add ios_add2appTests to ios_add2app.xcscheme's tests. Then the test command becomes:

set -o pipefail && xcodebuild \ -workspace ios_add2app.xcworkspace \ -scheme ios_add2app \ -sdk "iphonesimulator$os_version" \ -destination "OS=$os_version,name=iPhone X" test | $PRETTY

(I know you copied this pattern from the add2app tests, I've been meaning to change it there, too).

I got error 'Scheme ios_add2app is not currently configured for the test action'. do you mean the other way around? deleting ios_add2app and only keep iod_add2appTests?

To configure it for the test action:

Product > Scheme > Manage Schemes... > ios_add2app > Edit...

Test > Test > + > ios_add2appTests > Add

Then you can delete the ios_add2appTests scheme. I'm happy to come show you!

jmagman · 2019-10-03T19:51:37Z

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/AppDelegate.m

You don't need to override the getter, you can remove this.

jmagman · 2019-10-03T19:53:02Z

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/AppDelegate.m

Set with the property setters, not the backing instance variables here so the KVO notifications get posted correctly.

self.mainViewController = ... self.navigationController = ...

jmagman · 2019-10-03T19:53:17Z

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/AppDelegate.h

jmagman · 2019-10-03T19:53:26Z

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/AppDelegate.m

Remove this too.

jmagman · 2019-10-03T20:00:30Z

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/MainViewController.m

Is this wrapper method necessary? Can -showFullScreenCold grab what it needs from the AppDelegate?

jmagman · 2019-10-03T21:05:05Z

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/MainViewController.m

Sorry I'm not familiar with the message channel patterns, is this used anywhere?

jmagman · 2019-10-03T21:06:27Z

dev/integration_tests/ios_add2app_life_cycle/ios_add2appTests/IntegrationTests.m

You don't need the relative paths.

#import "AppDelegate.h" #import "FullScreenViewController.h"

jmagman · 2019-10-03T21:06:43Z

dev/integration_tests/ios_add2app_life_cycle/ios_add2appTests/IntegrationTests.m

This looks unused?

jmagman · 2019-10-03T21:08:02Z

dev/integration_tests/ios_add2app_life_cycle/ios_add2appTests/IntegrationTests.m

Were you seeing memory bloat here? I would imagine the pool drains between tests but I don't know.

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/MainViewController.m

packages/flutter/lib/src/scheduler/binding.dart

chunhtai · 2019-10-04T17:28:41Z

@jmagman addressed all comments, ready for review

codecov · 2019-10-04T17:54:47Z

Codecov Report

Merging #39945 into master will decrease coverage by 3.51%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #39945      +/-   ##
==========================================
- Coverage   62.21%    58.7%   -3.52%     
==========================================
  Files         199      194       -5     
  Lines       19234    18977     -257     
==========================================
- Hits        11966    11140     -826     
- Misses       7268     7837     +569

Flag	Coverage Δ
#flutter_tool	`58.7% <ø> (-3.52%)`	⬇️

Impacted Files	Coverage Δ
...kages/flutter_tools/lib/src/commands/generate.dart	`20% <0%> (-65.19%)`	⬇️
...ages/flutter_tools/lib/src/commands/build_web.dart	`29.41% <0%> (-64.71%)`	⬇️
packages/flutter_tools/lib/src/web/compile.dart	`10.34% <0%> (-59.66%)`	⬇️
packages/flutter_tools/lib/src/web/workflow.dart	`33.33% <0%> (-55.56%)`	⬇️
...ages/flutter_tools/lib/src/macos/macos_device.dart	`8% <0%> (-43.86%)`	⬇️
...ages/flutter_tools/lib/src/linux/linux_device.dart	`40% <0%> (-31.43%)`	⬇️
...ackages/flutter_tools/lib/src/commands/unpack.dart	`6.75% <0%> (-24.42%)`	⬇️
packages/flutter_tools/lib/src/run_cold.dart	`19.51% <0%> (-22.85%)`	⬇️
...ter_tools/lib/src/build_system/targets/assets.dart	`64.86% <0%> (-22.1%)`	⬇️
packages/flutter_tools/lib/src/project.dart	`65.23% <0%> (-21.19%)`	⬇️
... and 108 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02aac50...5fdfd15. Read the comment docs.

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/AppDelegate.m

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/MainViewController.h

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/MainViewController.m

dev/integration_tests/ios_add2app_life_cycle/ios_add2appTests/IntegrationTests.m

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/AppDelegate.m

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/MainViewController.m

chunhtai · 2019-10-07T21:17:37Z

@jmagman All comments addressed, thanks!

jmagman

Thank you for all the updates! LGTM with nit.

dev/integration_tests/ios_add2app_life_cycle/ios_add2appTests/IntegrationTests.m

goderbauer · 2019-10-16T21:41:09Z

(PR triage): @chunhtai what's the status of this one?

chunhtai · 2019-10-17T00:19:50Z

@goderbauer
This can be merged, but I am not confident to merge this along with engine pr when i am on vacation. I will be resume the merge after I come back from vacation. Let me know if I should close this.

xster · 2019-11-06T01:41:19Z

You're going to land this one soon to match flutter/engine#13642 right?

chunhtai · 2019-11-06T02:01:07Z

You're going to land this one soon to match flutter/engine#13642 right?

yes, I am waiting for luci build to finish before i can tag the sha

xster · 2019-11-06T02:06:23Z

understood. Thanks

xster · 2019-11-06T04:08:20Z

\o/

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Sep 5, 2019

googlebot added the cla: yes label Sep 5, 2019

chunhtai added the c: API break Backwards-incompatible API changes label Sep 6, 2019

chunhtai requested review from goderbauer and xster September 6, 2019 20:16

xster reviewed Sep 7, 2019

View reviewed changes

packages/flutter/lib/src/scheduler/binding.dart Outdated Show resolved Hide resolved

Piinks changed the title ~~added new lfecycle state~~ added new lifecycle state Sep 13, 2019

Piinks added the a: existing-apps Integration with existing apps via the add-to-app flow label Sep 16, 2019

goderbauer added the work in progress; do not review label Sep 18, 2019

chunhtai force-pushed the issues/39832 branch from 08b2540 to 2d6a900 Compare September 23, 2019 18:35

chunhtai commented Sep 27, 2019

View reviewed changes

chunhtai force-pushed the issues/39832 branch from 126cdba to 1308129 Compare September 30, 2019 20:31

chunhtai removed the work in progress; do not review label Sep 30, 2019

chunhtai requested a review from jmagman October 3, 2019 16:24

jmagman reviewed Oct 3, 2019

View reviewed changes

dev/integration_tests/ios_add2app_life_cycle/ios_add2app/MainViewController.m Outdated Show resolved Hide resolved

jmagman requested changes Oct 3, 2019

View reviewed changes

packages/flutter/lib/src/scheduler/binding.dart Outdated Show resolved Hide resolved

chunhtai force-pushed the issues/39832 branch from 97b0170 to 4e7e989 Compare October 4, 2019 17:21

jmagman requested changes Oct 7, 2019

View reviewed changes

chunhtai force-pushed the issues/39832 branch from 4e7e989 to 55dbfc8 Compare October 7, 2019 21:16

jmagman approved these changes Oct 7, 2019

View reviewed changes

dev/integration_tests/ios_add2app_life_cycle/ios_add2appTests/IntegrationTests.m Outdated Show resolved Hide resolved

jmagman mentioned this pull request Oct 10, 2019

Create add-to-app embedding integration test #33107

Closed

chunhtai force-pushed the issues/39832 branch from c726b45 to c58e45d Compare October 28, 2019 20:11

chunhtai force-pushed the issues/39832 branch 2 times, most recently from 14b3649 to ac9874a Compare November 4, 2019 23:43

flar mentioned this pull request Nov 6, 2019

Added new lifecycle enum flutter/engine#11913

Merged

chunhtai force-pushed the issues/39832 branch from 5c125d4 to e1ff1b1 Compare November 6, 2019 02:20

chunhtai added 3 commits November 5, 2019 18:30

added new lfecycle state

ce1d38f

update

b3e176d

update

5fdfd15

chunhtai force-pushed the issues/39832 branch from e1ff1b1 to 5fdfd15 Compare November 6, 2019 02:32

chunhtai merged commit 7aebde1 into flutter:master Nov 6, 2019

DaveShuckerow mentioned this pull request Nov 6, 2019

Tests are timing out on Travis flutter/devtools#1311

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

added new lifecycle state #39945

added new lifecycle state #39945

Uh oh!

Conversation

chunhtai commented Sep 5, 2019

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Sep 5, 2019

Uh oh!

Uh oh!

xster commented Sep 7, 2019

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!

chunhtai commented Sep 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

chunhtai commented Oct 4, 2019

Uh oh!

codecov bot commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chunhtai commented Oct 7, 2019

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer commented Oct 16, 2019

Uh oh!

chunhtai commented Oct 17, 2019

Uh oh!

xster commented Nov 6, 2019

Uh oh!

chunhtai commented Nov 6, 2019

Uh oh!

chunhtai Oct 3, 2019 •

edited

Loading

jmagman Oct 4, 2019 •

edited

Loading

codecov bot commented Oct 4, 2019 •

edited

Loading