-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Read the FLTEnableImpeller flag from the right bundle #40535
Conversation
@@ -207,7 +207,7 @@ | |||
settings.enable_wide_gamut = enableWideGamut; | |||
|
|||
// Whether to enable Impeller. | |||
NSNumber* enableImpeller = [mainBundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]; | |||
NSNumber* enableImpeller = [bundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to check both, right? Otherwise we will break customers who followed our instructions to add the flag to their app's Info.plist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh. Good point. I need to take another look at this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this:
NSNumber* enableImpeller = [bundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]; | |
NSNumber* enableImpellerMainBundle = [mainBundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]; | |
NSNumber* enableImpeller = | |
enableImpellerMainBundle != nil | |
? enableImpellerMainBundle | |
: [bundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Based on the comment above, I look in the app bundle first.
1d6e39c
to
19166cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
|
||
auto settings = FLTDefaultSettingsForBundle(); | ||
// Check settings.enable_impeller value is same as the value defined in Info.plist. | ||
XCTAssertEqual(settings.enable_impeller, NO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XCTAssertTrue
and XCTAssertFalse
are a little more idiomatic, but it's a minor nit.
XCTAssertEqual(settings.enable_impeller, NO); | |
XCTAssertFalse(settings.enable_impeller); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect I'll be touching this file again soon, and I'll keep this in mind for a cleanup then.
…ter/engine#40535) (#123306) Roll Flutter Engine from 3f9fe923b037 to cd0a6ef93263 (1 revision)
…dle (flutter/engine#40535) (#123306) Commit: c0878572badbdd67517a20076685239e6b6418c7
Fixes flutter/flutter#123027. Turns out, this isn't an issue with back-grounding. This can only happen if the IO manager is torn down before worker pool has had a chance to process the decompression job, or if the UI manager was never setup. It is hard to imaging how this could happen in a real application. If it does though, we should not crash. This patch makes the change to return an error instead. The linked issue was a test environment and the failure was addressed in flutter#40535.
This PR causes the
FLTEnableImpeller
flag to be read first out of the application bundle, and next out of the main bundle.This is not exactly what is described in
engine/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm
Line 82 in e159e35
Fixes flutter/flutter#123046