Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Manage iOS contexts separately #12078

Merged
gw280 merged 5 commits into from
Sep 11, 2019
Merged

Manage iOS contexts separately #12078

gw280 merged 5 commits into from
Sep 11, 2019

Conversation

@gw280
Copy link
Contributor

@gw280 gw280 commented Sep 9, 2019

Add #pragma clang diagnostic ignored "-Wobjc-method-access" to ignore the warnings. This is because we're unit testing using non-public APIs (this was discussed and approved in #11798)

@gw280 gw280 requested a review from dnfield September 9, 2019 17:09
@gw280
Copy link
Contributor Author

gw280 commented Sep 9, 2019

Sorry I think the discussion surrounding private APIs for testing was discussed on Discord, not in the PR. In any case, we decided to go ahead with it.

dnfield
dnfield reviewed Sep 9, 2019
View reviewed changes
testing/scenario_app/ios/Scenarios/ScenariosTests/FlutterViewTest.m Outdated
@property(nonatomic, strong) FlutterEngine* flutterEngine;
@end

#pragma clang diagnostic push
Copy link
Contributor

@dnfield dnfield Sep 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this to just around the offending area, so we don't open it up for the whole file?

dnfield
dnfield approved these changes Sep 9, 2019
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

chinmaygarde
chinmaygarde suggested changes Sep 9, 2019
View reviewed changes
Copy link
Contributor

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sort of a change makes me really really nervous. We have never ignored warnings when they became inconvenient. As written, this code has a very real issue and I would really rather not characterize the fix as undertaking a yak shave if the workaround involves selectively disabling a warning.

@gw280
Copy link
Contributor Author

gw280 commented Sep 9, 2019

This sort of a change makes me really really nervous. We have never ignored warnings when they became inconvenient. As written, this code has a very real issue and I would really rather not characterize the fix as undertaking a yak shave if the workaround involves selectively disabling a warning.

So my understanding was that this was strictly temporary until we can figure out a better way to test this, hence the meeting tomorrow and the large TODO comment. I'd like for this to not be blocked on a test issue like this, ugliness notwithstanding.

@dnfield
Copy link
Contributor

dnfield commented Sep 9, 2019

@chinmaygarde - this change is only within a unit test.

Do you think it's ok to do this while we work on wiring up a better way to test these things? Otherwise, we can't land the fix with a test, and we risk landing a fix that we will quickly regress because it has no tests.

@chinmaygarde
Copy link
Contributor

chinmaygarde commented Sep 9, 2019

this change is only within a unit test.

Code in unit-tests is still code we have to maintain. The same engineering practices and standard ought to apply to all code.

I am convinced we should work on wiring up a harness ASAP. Will work on this now. George, feel free to land this (you have Dan's LGTM already) while I/we do this.

@gw280
Copy link
Contributor Author

gw280 commented Sep 9, 2019

I found some more issues with this locally so I'm going to shelve this until we meet to discuss the testing changes needed to support this change.

@gw280 gw280 force-pushed the gwright-separate-contexts-warning-fix branch from 5ab2f84 to 2ea692f Compare September 11, 2019 17:38
@gw280 gw280 changed the title Manage iOS contexts separately (with warning fix) Manage iOS contexts separately Sep 11, 2019
@gw280 gw280 merged commit 4ac0663 into flutter:master Sep 11, 2019
flar added a commit that referenced this pull request Sep 12, 2019
@flar
Revert "Manage iOS contexts separately (#12078)"
6148ce5 
This reverts commit 4ac0663.
@flar flar mentioned this pull request Sep 12, 2019
Merged
flar added a commit that referenced this pull request Sep 12, 2019
@flar
Revert "Manage iOS contexts separately (#12078)" (#12233)
da84d59 
This reverts commit 4ac0663.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2019
@engine-flutter-autoroll
4ac0663 Manage iOS contexts separately (flutter/engine#12078)
1640b36
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 12, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Sep 12, 2019
@engine-flutter-autoroll
Roll engine 28d7900..da84d59 (2 commits) (#40309)
b939921 
git@github.com:flutter/engine.git/compare/28d7900ed767...da84d59

git log 28d7900..da84d59 --no-merges --oneline
2019-09-12 50503328+flar@users.noreply.github.com Revert "Manage iOS contexts separately (#12078)" (flutter/engine#12233)
2019-09-11 gw@gwright.org.uk Manage iOS contexts separately (flutter/engine#12078)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 12, 2019
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2019
@engine-flutter-autoroll
4ac0663 Manage iOS contexts separately (flutter/engine#12078)
34c6fdd
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Sep 12, 2019
@engine-flutter-autoroll
Roll engine 7ea9884..bbb1f12 (69 commits) (#40315)
8a36a36 
git@github.com:flutter/engine.git/compare/7ea9884ab00e...bbb1f12

git log 7ea9884..bbb1f12 --no-merges --oneline
2019-09-12 50503328+flar@users.noreply.github.com Adjust iOS frame start times to match the platform info (flutter/engine#11802)
2019-09-12 skia-flutter-autoroll@skia.org Roll src/third_party/skia 50f377e275c3..7c47d41067d4 (3 commits) (flutter/engine#12231)
2019-09-12 50503328+flar@users.noreply.github.com Revert "Manage iOS contexts separately (#12078)" (flutter/engine#12233)
2019-09-11 gw@gwright.org.uk Manage iOS contexts separately (flutter/engine#12078)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 120e7d6766e4..50f377e275c3 (7 commits) (flutter/engine#12224)
2019-09-11 mklim@google.com Revert "Roll src/third_party/dart be66176534..ec7ec4ecf7 (37 commits)" (flutter/engine#12223)
2019-09-11 iska.kaushik@gmail.com Do not generate kernel platform files on topaz tree (flutter/engine#12222)
2019-09-11 hterkelsen@users.noreply.github.com Don't disable toString in release mode for dart:ui classes (flutter/engine#12204)
2019-09-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from 7gDBN... to u7Q31... (flutter/engine#12221)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia d96ef09317d6..120e7d6766e4 (2 commits) (flutter/engine#12220)
2019-09-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from vDk46... to _nS67... (flutter/engine#12219)
2019-09-11 iska.kaushik@gmail.com Namespace patched SDK names to not conflict with Topaz (flutter/engine#12218)
2019-09-11 bkonyi@google.com Roll src/third_party/dart ca7baa4013..4d5e15abde (29 commits)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 14318c140949..d96ef09317d6 (2 commits) (flutter/engine#12216)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia b23a4f9d9442..14318c140949 (2 commits) (flutter/engine#12215)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 26ac0467cb4c..b23a4f9d9442 (2 commits) (flutter/engine#12214)
2019-09-11 bkonyi@google.com Roll src/third_party/dart 7bbfd532de..ca7baa4013 (3 commits)
2019-09-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from R1yqu... to 7gDBN... (flutter/engine#12212)
2019-09-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from spUG2... to vDk46... (flutter/engine#12210)
2019-09-11 chinmaygarde@google.com Roll buildroot and Fuchsia toolchain and unblock the Fuchsia/Linux autoroller manually. (flutter/engine#12209)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 4fe30e15c06c..26ac0467cb4c (2 commits) (flutter/engine#12207)
2019-09-11 chinmaygarde@google.com Only build the x64 variant of Fuchsia on the try-jobs. (flutter/engine#12206)
2019-09-10 hterkelsen@users.noreply.github.com Don't load Roboto by default (flutter/engine#12205)
2019-09-10 bkonyi@google.com Roll src/third_party/dart 300c3333d1..7bbfd532de (5 commits)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia 66d8006c2bb1..4fe30e15c06c (11 commits) (flutter/engine#12202)
2019-09-10 yjbanov@google.com add a convenience CLI tool for building and testing web engine (flutter/engine#12197)
2019-09-10 jason-simmons@users.noreply.github.com [flutter_runner] Generate symbols for the Dart VM profiler (flutter/engine#12048)
2019-09-10 yjbanov@google.com Add custom test plugin that supports screenshot tests (flutter/engine#12079)
2019-09-10 chinmaygarde@google.com Move the Fuchsia tryjob into a its own step and disable LTO. (flutter/engine#12190)
2019-09-10 bkonyi@google.com Roll src/third_party/dart 62f78a7abb..300c3333d1 (6 commits)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia b88894c8811b..66d8006c2bb1 (5 commits) (flutter/engine#12178)
2019-09-10 jason-simmons@users.noreply.github.com [flutter_runner] Port the accessibility bridge from Topaz (flutter/engine#12054)
2019-09-10 liyuqian@google.com Smooth out iOS irregular input events delivery (flutter/engine#11817)
2019-09-10 bkonyi@google.com Roll src/third_party/dart ccb6ba948b..62f78a7abb (3 commits)
2019-09-10 jonahwilliams@google.com Make ImageShader implement Shader for web ui (flutter/engine#12161)
2019-09-10 bkonyi@google.com Roll src/third_party/dart 2e8d912848..ccb6ba948b (30 commits)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia 9e5c47936b17..b88894c8811b (3 commits) (flutter/engine#12151)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia 1bf30ce852e0..9e5c47936b17 (2 commits) (flutter/engine#12129)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8cae1e95a23b..1bf30ce852e0 (2 commits) (flutter/engine#12106)
2019-09-10 skia-flutter-autoroll@skia.org Roll fuchsia/clang/mac-amd64 from H1Qjc... to HfPKR... (flutter/engine#12088)
2019-09-10 chinmaygarde@google.com Don't launch the observatory by default on each embedder unit-test invocation. (flutter/engine#12087)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia c2d84bfa7421..8cae1e95a23b (4 commits) (flutter/engine#12086)
2019-09-10 bkonyi@google.com Roll src/third_party/dart fb14babf59..2e8d912848 (65 commits)
2019-09-10 chinmaygarde@google.com Guard availability of user notification related methods to iOS 10.0 (flutter/engine#12084)
2019-09-09 bmparr@google.com Add capability to add AppDelegate as UNUserNotificationCenterDelegate (flutter/engine#9864)
...
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine 28d7900..da84d59 (2 commits) (flutter#40309)
c78272e 
git@github.com:flutter/engine.git/compare/28d7900ed767...da84d59

git log 28d7900..da84d59 --no-merges --oneline
2019-09-12 50503328+flar@users.noreply.github.com Revert "Manage iOS contexts separately (flutter#12078)" (flutter/engine#12233)
2019-09-11 gw@gwright.org.uk Manage iOS contexts separately (flutter/engine#12078)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@engine-flutter-autoroll @Inconnu08
Roll engine 7ea9884..bbb1f12 (69 commits) (flutter#40315)
7f5a82e 
git@github.com:flutter/engine.git/compare/7ea9884ab00e...bbb1f12

git log 7ea9884..bbb1f12 --no-merges --oneline
2019-09-12 50503328+flar@users.noreply.github.com Adjust iOS frame start times to match the platform info (flutter/engine#11802)
2019-09-12 skia-flutter-autoroll@skia.org Roll src/third_party/skia 50f377e275c3..7c47d41067d4 (3 commits) (flutter/engine#12231)
2019-09-12 50503328+flar@users.noreply.github.com Revert "Manage iOS contexts separately (flutter#12078)" (flutter/engine#12233)
2019-09-11 gw@gwright.org.uk Manage iOS contexts separately (flutter/engine#12078)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 120e7d6766e4..50f377e275c3 (7 commits) (flutter/engine#12224)
2019-09-11 mklim@google.com Revert "Roll src/third_party/dart be66176534..ec7ec4ecf7 (37 commits)" (flutter/engine#12223)
2019-09-11 iska.kaushik@gmail.com Do not generate kernel platform files on topaz tree (flutter/engine#12222)
2019-09-11 hterkelsen@users.noreply.github.com Don't disable toString in release mode for dart:ui classes (flutter/engine#12204)
2019-09-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from 7gDBN... to u7Q31... (flutter/engine#12221)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia d96ef09317d6..120e7d6766e4 (2 commits) (flutter/engine#12220)
2019-09-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from vDk46... to _nS67... (flutter/engine#12219)
2019-09-11 iska.kaushik@gmail.com Namespace patched SDK names to not conflict with Topaz (flutter/engine#12218)
2019-09-11 bkonyi@google.com Roll src/third_party/dart ca7baa4013..4d5e15abde (29 commits)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 14318c140949..d96ef09317d6 (2 commits) (flutter/engine#12216)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia b23a4f9d9442..14318c140949 (2 commits) (flutter/engine#12215)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 26ac0467cb4c..b23a4f9d9442 (2 commits) (flutter/engine#12214)
2019-09-11 bkonyi@google.com Roll src/third_party/dart 7bbfd532de..ca7baa4013 (3 commits)
2019-09-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from R1yqu... to 7gDBN... (flutter/engine#12212)
2019-09-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from spUG2... to vDk46... (flutter/engine#12210)
2019-09-11 chinmaygarde@google.com Roll buildroot and Fuchsia toolchain and unblock the Fuchsia/Linux autoroller manually. (flutter/engine#12209)
2019-09-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 4fe30e15c06c..26ac0467cb4c (2 commits) (flutter/engine#12207)
2019-09-11 chinmaygarde@google.com Only build the x64 variant of Fuchsia on the try-jobs. (flutter/engine#12206)
2019-09-10 hterkelsen@users.noreply.github.com Don't load Roboto by default (flutter/engine#12205)
2019-09-10 bkonyi@google.com Roll src/third_party/dart 300c3333d1..7bbfd532de (5 commits)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia 66d8006c2bb1..4fe30e15c06c (11 commits) (flutter/engine#12202)
2019-09-10 yjbanov@google.com add a convenience CLI tool for building and testing web engine (flutter/engine#12197)
2019-09-10 jason-simmons@users.noreply.github.com [flutter_runner] Generate symbols for the Dart VM profiler (flutter/engine#12048)
2019-09-10 yjbanov@google.com Add custom test plugin that supports screenshot tests (flutter/engine#12079)
2019-09-10 chinmaygarde@google.com Move the Fuchsia tryjob into a its own step and disable LTO. (flutter/engine#12190)
2019-09-10 bkonyi@google.com Roll src/third_party/dart 62f78a7abb..300c3333d1 (6 commits)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia b88894c8811b..66d8006c2bb1 (5 commits) (flutter/engine#12178)
2019-09-10 jason-simmons@users.noreply.github.com [flutter_runner] Port the accessibility bridge from Topaz (flutter/engine#12054)
2019-09-10 liyuqian@google.com Smooth out iOS irregular input events delivery (flutter/engine#11817)
2019-09-10 bkonyi@google.com Roll src/third_party/dart ccb6ba948b..62f78a7abb (3 commits)
2019-09-10 jonahwilliams@google.com Make ImageShader implement Shader for web ui (flutter/engine#12161)
2019-09-10 bkonyi@google.com Roll src/third_party/dart 2e8d912848..ccb6ba948b (30 commits)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia 9e5c47936b17..b88894c8811b (3 commits) (flutter/engine#12151)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia 1bf30ce852e0..9e5c47936b17 (2 commits) (flutter/engine#12129)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia 8cae1e95a23b..1bf30ce852e0 (2 commits) (flutter/engine#12106)
2019-09-10 skia-flutter-autoroll@skia.org Roll fuchsia/clang/mac-amd64 from H1Qjc... to HfPKR... (flutter/engine#12088)
2019-09-10 chinmaygarde@google.com Don't launch the observatory by default on each embedder unit-test invocation. (flutter/engine#12087)
2019-09-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia c2d84bfa7421..8cae1e95a23b (4 commits) (flutter/engine#12086)
2019-09-10 bkonyi@google.com Roll src/third_party/dart fb14babf59..2e8d912848 (65 commits)
2019-09-10 chinmaygarde@google.com Guard availability of user notification related methods to iOS 10.0 (flutter/engine#12084)
2019-09-09 bmparr@google.com Add capability to add AppDelegate as UNUserNotificationCenterDelegate (flutter/engine#9864)
...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@chinmaygarde chinmaygarde chinmaygarde requested changes

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gw280 @dnfield @chinmaygarde @googlebot