Read AndroidManifest.xml and emit manifest-impeller-(enabled|disabled) analytics (flutter#150791)

matanlurey · web-flow · commit a6a8caaa7338 · 2024-06-25T22:41:23.000Z
Work towards flutter#132712. After this PR, after a completed `flutter build apk` command, we: - Emit a `manifest-impeller-disabled` command if `io.flutter.embedding.android.EnableImpeller` is `'false'`. - Emit a `manifest-impeller-disabled` command if `io.flutter.embedding.android.EnableImpeller` is _missing_. - Emit a `manifest-impeller-enabled` command if `io.flutter.embedding.android.EnableImpeller` is `'true'`. We will need to change the default (see `_impellerEnabledByDefault` in `project.dart`) before releasing, otherwise we will misreport `manifest-impeller-disabled` at a much higher rate than actual. If there is a way to instead compute the default instead of hard-coding, that would have been good. See <https://docs.flutter.dev/perf/impeller#android> for details on the key-value pair. --- I also did a tad of TLC, by removing the (now-defunct) `Usage` events for `flutter build ios`, so they are consistent. /cc @zanderso, @chinmaygarde, @jonahwilliams
diff --git a/packages/flutter_tools/lib/src/commands/build_apk.dart b/packages/flutter_tools/lib/src/commands/build_apk.dart
@@ -137,12 +137,26 @@ class BuildApkCommand extends BuildSubCommand {
     validateBuild(androidBuildInfo);
     displayNullSafetyMode(androidBuildInfo.buildInfo);
     globals.terminal.usesTerminalUi = true;
+    final FlutterProject project = FlutterProject.current();
     await androidBuilder?.buildApk(
-      project: FlutterProject.current(),
+      project: project,
       target: targetFile,
       androidBuildInfo: androidBuildInfo,
       configOnly: configOnly,
     );
+
+    // When an app is successfully built, record to analytics whether Impeller
+    // is enabled or disabled. Note that 'computeImpellerEnabled' will default
+    // to false if not enabled explicitly in the manifest.
+    final bool impellerEnabled = project.android.computeImpellerEnabled();
+    final String buildLabel = impellerEnabled
+          ? 'manifest-impeller-enabled'
+          : 'manifest-impeller-disabled';
+    globals.analytics.send(Event.flutterBuildInfo(
+      label: buildLabel,
+      buildType: 'android',
+    ));
+
     return FlutterCommandResult.success();
   }
 }
diff --git a/packages/flutter_tools/lib/src/commands/build_ios.dart b/packages/flutter_tools/lib/src/commands/build_ios.dart
@@ -24,7 +24,6 @@ import '../ios/application_package.dart';
 import '../ios/mac.dart';
 import '../ios/plist_parser.dart';
 import '../project.dart';
-import '../reporting/reporting.dart';
 import '../runner/flutter_command.dart';
 import 'build.dart';
 
@@ -769,7 +768,8 @@ abstract class _BuildIOSSubCommand extends BuildSubCommand {
       );
 
       // When an app is successfully built, record to analytics whether Impeller
-      // is enabled or disabled.
+      // is enabled or disabled. Note that we report the _lack_ of an explicit
+      // flag set as "enabled" because the default is to enable Impeller on iOS.
       final BuildableIOSApp app = await buildableIOSApp;
       final String plistPath = app.project.infoPlist.path;
       final bool? impellerEnabled = globals.plistParser.getValueFromFile<bool>(
@@ -779,11 +779,6 @@ abstract class _BuildIOSSubCommand extends BuildSubCommand {
       final String buildLabel = impellerEnabled == false
           ? 'plist-impeller-disabled'
           : 'plist-impeller-enabled';
-      BuildEvent(
-        buildLabel,
-        type: 'ios',
-        flutterUsage: globals.flutterUsage,
-      ).send();
       globals.analytics.send(Event.flutterBuildInfo(
         label: buildLabel,
         buildType: 'ios',
diff --git a/packages/flutter_tools/lib/src/project.dart b/packages/flutter_tools/lib/src/project.dart
@@ -881,6 +881,41 @@ $javaGradleCompatUrl
     }
     return AndroidEmbeddingVersionResult(AndroidEmbeddingVersion.v1, 'No `<meta-data android:name="flutterEmbedding" android:value="2"/>` in ${appManifestFile.absolute.path}');
   }
+
+  // TODO(matanlurey): Flip to true when on by default, https://github.com/flutter/flutter/issues/132712.
+  static const bool _impellerEnabledByDefault = false;
+
+  /// Returns the `io.flutter.embedding.android.EnableImpeller` manifest value.
+  ///
+  /// If there is no manifest file, or the key is not present, returns `false`.
+  bool computeImpellerEnabled() {
+    if (!appManifestFile.existsSync()) {
+      return _impellerEnabledByDefault;
+    }
+    final XmlDocument document;
+    try {
+      document = XmlDocument.parse(appManifestFile.readAsStringSync());
+    } on XmlException {
+      throwToolExit('Error parsing $appManifestFile '
+                    'Please ensure that the android manifest is a valid XML document and try again.');
+    } on FileSystemException {
+      throwToolExit('Error reading $appManifestFile even though it exists. '
+                    'Please ensure that you have read permission to this file and try again.');
+    }
+    for (final XmlElement metaData in document.findAllElements('meta-data')) {
+      final String? name = metaData.getAttribute('android:name');
+      if (name == 'io.flutter.embedding.android.EnableImpeller') {
+        final String? value = metaData.getAttribute('android:value');
+        if (value == 'true') {
+          return true;
+        }
+        if (value == 'false') {
+          return false;
+        }
+      }
+    }
+    return _impellerEnabledByDefault;
+  }
 }
 
 /// Iteration of the embedding Java API in the engine used by the Android project.
diff --git a/packages/flutter_tools/test/commands.shard/hermetic/build_ios_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/build_ios_test.dart
@@ -621,14 +621,6 @@ void main() {
         const <String>['build', 'ios', '--no-pub']
       );
 
-      expect(usage.events, contains(
-        const TestUsageEvent(
-          'build', 'ios',
-          label:'plist-impeller-enabled',
-          parameters:CustomDimensions(),
-        ),
-      ));
-
       expect(fakeAnalytics.sentEvents, contains(
         Event.flutterBuildInfo(
           label: 'plist-impeller-enabled',
@@ -684,14 +676,6 @@ void main() {
         const <String>['build', 'ios', '--no-pub']
       );
 
-      expect(usage.events, contains(
-        const TestUsageEvent(
-          'build', 'ios',
-          label:'plist-impeller-disabled',
-          parameters:CustomDimensions(),
-        ),
-      ));
-
       expect(fakeAnalytics.sentEvents, contains(
         Event.flutterBuildInfo(
           label: 'plist-impeller-disabled',
diff --git a/packages/flutter_tools/test/commands.shard/permeable/build_apk_test.dart b/packages/flutter_tools/test/commands.shard/permeable/build_apk_test.dart
@@ -127,6 +127,120 @@ void main() {
       AndroidBuilder: () => FakeAndroidBuilder(),
       Usage: () => testUsage,
     });
+
+    group('Impeller AndroidManifest.xml setting', () {
+      // Adds a key-value `<meta-data>` pair to the `<application>` tag in the
+      // cooresponding `AndroidManifest.xml` file, right before the closing
+      // `</application>` tag.
+      void writeManifestMetadata({
+        required String projectPath,
+        required String name,
+        required String value,
+      }) {
+        final String manifestPath = globals.fs.path.join(
+          projectPath,
+          'android',
+          'app',
+          'src',
+          'main',
+          'AndroidManifest.xml',
+        );
+
+        // It would be unnecessarily complicated to parse this XML file and
+        // insert the key-value pair, so we just insert it right before the
+        // closing </application> tag.
+        final String oldManifest = globals.fs.file(manifestPath).readAsStringSync();
+        final String newManifest = oldManifest.replaceFirst(
+          '</application>',
+          '    <meta-data\n'
+          '        android:name="$name"\n'
+          '        android:value="$value" />\n'
+          '    </application>',
+        );
+        globals.fs.file(manifestPath).writeAsStringSync(newManifest);
+      }
+
+      testUsingContext('a default APK build reports Impeller as disabled', () async {
+        final String projectPath = await createProject(
+          tempDir,
+          arguments: <String>['--no-pub', '--template=app', '--platform=android']
+        );
+
+        await runBuildApkCommand(projectPath);
+
+        expect(
+          fakeAnalytics.sentEvents,
+          contains(
+            Event.flutterBuildInfo(
+              label: 'manifest-impeller-disabled',
+              buildType: 'android',
+            ),
+          ),
+        );
+      }, overrides: <Type, Generator>{
+        Analytics: () => fakeAnalytics,
+        AndroidBuilder: () => FakeAndroidBuilder(),
+        FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir),
+      });
+
+      testUsingContext('EnableImpeller="true" reports an enabled event', () async {
+        final String projectPath = await createProject(
+          tempDir,
+          arguments: <String>['--no-pub', '--template=app', '--platform=android']
+        );
+
+        writeManifestMetadata(
+          projectPath: projectPath,
+          name: 'io.flutter.embedding.android.EnableImpeller',
+          value: 'true',
+        );
+
+        await runBuildApkCommand(projectPath);
+
+        expect(
+          fakeAnalytics.sentEvents,
+          contains(
+            Event.flutterBuildInfo(
+              label: 'manifest-impeller-enabled',
+              buildType: 'android',
+            ),
+          ),
+        );
+      }, overrides: <Type, Generator>{
+        Analytics: () => fakeAnalytics,
+        AndroidBuilder: () => FakeAndroidBuilder(),
+        FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir),
+      });
+
+      testUsingContext('EnableImpeller="false" reports an disabled event', () async {
+        final String projectPath = await createProject(
+          tempDir,
+          arguments: <String>['--no-pub', '--template=app', '--platform=android']
+        );
+
+        writeManifestMetadata(
+          projectPath: projectPath,
+          name: 'io.flutter.embedding.android.EnableImpeller',
+          value: 'false',
+        );
+
+        await runBuildApkCommand(projectPath);
+
+        expect(
+          fakeAnalytics.sentEvents,
+          contains(
+            Event.flutterBuildInfo(
+              label: 'manifest-impeller-disabled',
+              buildType: 'android',
+            ),
+          ),
+        );
+      }, overrides: <Type, Generator>{
+        Analytics: () => fakeAnalytics,
+        AndroidBuilder: () => FakeAndroidBuilder(),
+        FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir),
+      });
+    });
   });
 
   group('Gradle', () {
