Rename hcpp flag and enable 2 #174241
Rename hcpp flag and enable 2 #174241
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request renames the EnableSurfaceControl flag to EnableHCPP across various files and enables the HCPP path for platform views on Android. The changes are mostly straightforward renames. My review focuses on ensuring consistency and clarity after these renames. I've pointed out a few places where comments, variable names, or function names are now misleading and should be updated to reflect the new HCPP flag. These changes will improve the maintainability of the code.
| @@ -50,8 +50,7 @@ public class FlutterLoader { | |||
| "io.flutter.embedding.android.EnableVulkanGPUTracing"; | |||
| private static final String DISABLE_MERGED_PLATFORM_UI_THREAD_KEY = | |||
| "io.flutter.embedding.android.DisableMergedPlatformUIThread"; | |||
| private static final String ENABLE_SURFACE_CONTROL = | |||
| "io.flutter.embedding.android.EnableSurfaceControl"; | |||
| private static final String ENABLE_SURFACE_CONTROL = "io.flutter.embedding.android.EnableHCPP"; | |||
There was a problem hiding this comment.
The value of this constant has been updated to the new HCPP flag, but the constant's name ENABLE_SURFACE_CONTROL is now misleading and inconsistent with the change. To improve code clarity and maintainability, please rename it to ENABLE_HCPP and update its usages throughout the file.
| private static final String ENABLE_SURFACE_CONTROL = "io.flutter.embedding.android.EnableHCPP"; | |
| private static final String ENABLE_HCPP = "io.flutter.embedding.android.EnableHCPP"; |
| @@ -844,7 +844,7 @@ void _addMetadataToManifest(String testDirectory, List<(String, String)> keyPair | |||
|
|
|||
| void _addSurfaceControlSupportToManifest(String testDirectory) { | |||
| final List<(String, String)> keyPairs = <(String, String)>[ | |||
| ('io.flutter.embedding.android.EnableSurfaceControl', 'true'), | |||
| ('io.flutter.embedding.android.EnableHCPP', 'true'), | |||
There was a problem hiding this comment.
This function now adds support for HCPP, but its name _addSurfaceControlSupportToManifest is misleading as it refers to the old flag. For better code clarity, please consider renaming it to something like _addHCPPSupportToManifest.
| /// Whether to enable SurfaceControl swapchain. | ||
| final bool enableSurfaceControl; | ||
| final bool enableHCPP; |
There was a problem hiding this comment.
The documentation for enableHCPP appears to be outdated. It still refers to SurfaceControl swapchain, which was relevant for the previous flag name. Please update the comment to accurately describe what enableHCPP does.
| /// Whether to enable SurfaceControl swapchain. | |
| final bool enableSurfaceControl; | |
| final bool enableHCPP; | |
| /// Whether to enable HCPP (Hybrid Composition Platform Views). | |
| final bool enableHCPP; |
| @@ -347,7 +347,7 @@ public void itSetsEnableSurfaceControlFromMetaData() { | |||
| FlutterJNI mockFlutterJNI = mock(FlutterJNI.class); | |||
| FlutterLoader flutterLoader = new FlutterLoader(mockFlutterJNI); | |||
| Bundle metaData = new Bundle(); | |||
| metaData.putBoolean("io.flutter.embedding.android.EnableSurfaceControl", true); | |||
| metaData.putBoolean("io.flutter.embedding.android.EnableHCPP", true); | |||
There was a problem hiding this comment.
While this line is correctly updated to use the new EnableHCPP flag, the surrounding test method is still named itSetsEnableSurfaceControlFromMetaData. For consistency and clarity, please consider renaming the test method to itSetsEnableHCPPFromMetaData.
Renames the public facing flag, and enables the branching in the pv channel based on the flag.
draft
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.