From 7b55d05902b69b7505ecec44739a1e00f281a2a9 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 3 Apr 2023 16:41:22 -0700 Subject: [PATCH] [Impeller] Respect enable-impeller command line setting over info.plist settting --- shell/common/switches_unittests.cc | 19 ++++++++++++ shell/platform/darwin/common/command_line.h | 5 +++- shell/platform/darwin/common/command_line.mm | 5 ++-- .../framework/Source/FlutterDartProject.mm | 30 +++++++++++-------- .../Source/FlutterDartProjectTest.mm | 26 ++++++++++++++++ .../Source/FlutterDartProject_Internal.h | 3 +- 6 files changed, 72 insertions(+), 16 deletions(-) diff --git a/shell/common/switches_unittests.cc b/shell/common/switches_unittests.cc index f6a6d28886e63..4d96729ec9899 100644 --- a/shell/common/switches_unittests.cc +++ b/shell/common/switches_unittests.cc @@ -93,5 +93,24 @@ TEST(SwitchesTest, EnableEmbedderAPI) { } } +TEST(SwitchesTest, NoEnableImpeller) { + { + // enable + fml::CommandLine command_line = + fml::CommandLineFromInitializerList({"command", "--enable-impeller"}); + EXPECT_TRUE(command_line.HasOption("enable-impeller")); + Settings settings = SettingsFromCommandLine(command_line); + EXPECT_EQ(settings.enable_impeller, true); + } + { + // disable + fml::CommandLine command_line = fml::CommandLineFromInitializerList( + {"command", "--enable-impeller=false"}); + EXPECT_TRUE(command_line.HasOption("enable-impeller")); + Settings settings = SettingsFromCommandLine(command_line); + EXPECT_EQ(settings.enable_impeller, false); + } +} + } // namespace testing } // namespace flutter diff --git a/shell/platform/darwin/common/command_line.h b/shell/platform/darwin/common/command_line.h index 807b8faae1267..43f3afac6d37f 100644 --- a/shell/platform/darwin/common/command_line.h +++ b/shell/platform/darwin/common/command_line.h @@ -5,13 +5,16 @@ #ifndef FLUTTER_SHELL_PLATFORM_DARWIN_COMMON_COMMAND_LINE_H_ #define FLUTTER_SHELL_PLATFORM_DARWIN_COMMON_COMMAND_LINE_H_ +#import + #include "flutter/fml/command_line.h" #include "flutter/fml/macros.h" namespace flutter { -fml::CommandLine CommandLineFromNSProcessInfo(); +fml::CommandLine CommandLineFromNSProcessInfo( + NSProcessInfo* processInfoOrNil = nil); } // namespace flutter diff --git a/shell/platform/darwin/common/command_line.mm b/shell/platform/darwin/common/command_line.mm index 55533103c3ce5..d9f42e634fde6 100644 --- a/shell/platform/darwin/common/command_line.mm +++ b/shell/platform/darwin/common/command_line.mm @@ -8,10 +8,11 @@ namespace flutter { -fml::CommandLine CommandLineFromNSProcessInfo() { +fml::CommandLine CommandLineFromNSProcessInfo(NSProcessInfo* processInfoOrNil) { std::vector args_vector; + auto processInfo = processInfoOrNil ? processInfoOrNil : [NSProcessInfo processInfo]; - for (NSString* arg in [NSProcessInfo processInfo].arguments) { + for (NSString* arg in processInfo.arguments) { args_vector.emplace_back(arg.UTF8String); } diff --git a/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm b/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm index 1830ad67eb4cb..a9bbe4fa7bda9 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm @@ -76,11 +76,11 @@ return [NSBundle bundleWithIdentifier:bundleID]; } -flutter::Settings FLTDefaultSettingsForBundle(NSBundle* bundle) { - auto command_line = flutter::CommandLineFromNSProcessInfo(); +flutter::Settings FLTDefaultSettingsForBundle(NSBundle* bundle, NSProcessInfo* processInfoOrNil) { + auto command_line = flutter::CommandLineFromNSProcessInfo(processInfoOrNil); // Precedence: - // 1. Settings from the specified NSBundle. + // 1. Settings from the specified NSBundle (except for enable-impeller). // 2. Settings passed explicitly via command-line arguments. // 3. Settings from the NSBundle with the default bundle ID. // 4. Settings from the main NSBundle and default values. @@ -206,15 +206,21 @@ BOOL enableWideGamut = nsEnableWideGamut ? nsEnableWideGamut.boolValue : NO; settings.enable_wide_gamut = enableWideGamut; - // Whether to enable Impeller. First, look in the app bundle. - NSNumber* enableImpeller = [bundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]; - if (enableImpeller == nil) { - // If it isn't in the app bundle, look in the main bundle. - enableImpeller = [mainBundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]; - } - // Change the default only if the option is present. - if (enableImpeller != nil) { - settings.enable_impeller = enableImpeller.boolValue; + // TODO(dnfield): We should reverse the order for all these settings so that command line options + // are preferred to plist settings. https://github.com/flutter/flutter/issues/124049 + // Whether to enable Impeller. If the command line explicitly + // specified an option for this, ignore what's in the plist. + if (!command_line.HasOption("enable-impeller")) { + // Next, look in the app bundle. + NSNumber* enableImpeller = [bundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]; + if (enableImpeller == nil) { + // If it isn't in the app bundle, look in the main bundle. + enableImpeller = [mainBundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]; + } + // Change the default only if the option is present. + if (enableImpeller != nil) { + settings.enable_impeller = enableImpeller.boolValue; + } } NSNumber* enableTraceSystrace = [mainBundle objectForInfoDictionaryKey:@"FLTTraceSystrace"]; diff --git a/shell/platform/darwin/ios/framework/Source/FlutterDartProjectTest.mm b/shell/platform/darwin/ios/framework/Source/FlutterDartProjectTest.mm index 5092053483171..7e180603a0a1a 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterDartProjectTest.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterDartProjectTest.mm @@ -93,6 +93,32 @@ - (void)testEnableImpellerSettingIsCorrectlyParsed { [mockMainBundle stopMocking]; } +- (void)testEnableImpellerSettingIsCorrectlyOverriddenByCommandLine { + id mockMainBundle = OCMPartialMock([NSBundle mainBundle]); + OCMStub([mockMainBundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]).andReturn(@"NO"); + id mockProcessInfo = OCMPartialMock([NSProcessInfo processInfo]); + NSArray* arguments = @[ @"process_name", @"--enable-impeller" ]; + OCMStub([mockProcessInfo arguments]).andReturn(arguments); + + auto settings = FLTDefaultSettingsForBundle(nil, mockProcessInfo); + // Check settings.enable_impeller value is same as the value on command line. + XCTAssertEqual(settings.enable_impeller, YES); + [mockMainBundle stopMocking]; +} + +- (void)testDisableImpellerSettingIsCorrectlyOverriddenByCommandLine { + id mockMainBundle = OCMPartialMock([NSBundle mainBundle]); + OCMStub([mockMainBundle objectForInfoDictionaryKey:@"FLTEnableImpeller"]).andReturn(@"YES"); + id mockProcessInfo = OCMPartialMock([NSProcessInfo processInfo]); + NSArray* arguments = @[ @"process_name", @"--enable-impeller=false" ]; + OCMStub([mockProcessInfo arguments]).andReturn(arguments); + + auto settings = FLTDefaultSettingsForBundle(nil, mockProcessInfo); + // Check settings.enable_impeller value is same as the value on command line. + XCTAssertEqual(settings.enable_impeller, NO); + [mockMainBundle stopMocking]; +} + - (void)testDisableImpellerAppBundleSettingIsCorrectlyParsed { NSString* bundleId = [FlutterDartProject defaultBundleIdentifier]; id mockAppBundle = OCMClassMock([NSBundle class]); diff --git a/shell/platform/darwin/ios/framework/Source/FlutterDartProject_Internal.h b/shell/platform/darwin/ios/framework/Source/FlutterDartProject_Internal.h index 50a42fe77f318..3888a785eb421 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterDartProject_Internal.h +++ b/shell/platform/darwin/ios/framework/Source/FlutterDartProject_Internal.h @@ -14,7 +14,8 @@ NS_ASSUME_NONNULL_BEGIN NSBundle* FLTFrameworkBundleInternal(NSString* bundleID, NSURL* searchURL); -flutter::Settings FLTDefaultSettingsForBundle(NSBundle* bundle = nil); +flutter::Settings FLTDefaultSettingsForBundle(NSBundle* _Nullable bundle = nil, + NSProcessInfo* _Nullable processInfoOrNil = nil); @interface FlutterDartProject ()