From d13cd8846e5ea7446a1120581b5091234c9a9eca Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 28 Sep 2023 17:21:14 -0700 Subject: [PATCH] [flutter_tools] remove VmService screenshot for native devices. (#135462) * This is completely broken on the Impeller renderer, see: https://github.com/flutter/flutter/issues/135052 * Even on the Skia renderer, this gives a software rasterized screenshot which will absolutely look different from a native rendering screenshot. I plan to remove this functionality from the engine. --- .../lib/src/commands/screenshot.dart | 32 +------ .../lib/src/resident_runner.dart | 10 +-- packages/flutter_tools/lib/src/vmservice.dart | 5 -- .../hermetic/screenshot_command_test.dart | 10 --- .../general.shard/terminal_handler_test.dart | 90 +------------------ .../test/general.shard/vmservice_test.dart | 7 -- 6 files changed, 10 insertions(+), 144 deletions(-) diff --git a/packages/flutter_tools/lib/src/commands/screenshot.dart b/packages/flutter_tools/lib/src/commands/screenshot.dart index 21b531ecf59..db3686e49f6 100644 --- a/packages/flutter_tools/lib/src/commands/screenshot.dart +++ b/packages/flutter_tools/lib/src/commands/screenshot.dart @@ -18,7 +18,6 @@ const String _kType = 'type'; const String _kVmServiceUrl = 'vm-service-url'; const String _kDeviceType = 'device'; const String _kSkiaType = 'skia'; -const String _kRasterizerType = 'rasterizer'; class ScreenshotCommand extends FlutterCommand { ScreenshotCommand({required this.fs}) { @@ -33,7 +32,7 @@ class ScreenshotCommand extends FlutterCommand { aliases: [ 'observatory-url' ], // for historical reasons valueHelp: 'URI', help: 'The VM Service URL to which to connect.\n' - 'This is required when "--$_kType" is "$_kSkiaType" or "$_kRasterizerType".\n' + 'This is required when "--$_kType" is "$_kSkiaType".\n' 'To find the VM service URL, use "flutter run" and look for ' '"A Dart VM Service ... is available at" in the output.', ); @@ -41,13 +40,12 @@ class ScreenshotCommand extends FlutterCommand { _kType, valueHelp: 'type', help: 'The type of screenshot to retrieve.', - allowed: const [_kDeviceType, _kSkiaType, _kRasterizerType], + allowed: const [_kDeviceType, _kSkiaType], allowedHelp: const { _kDeviceType: "Delegate to the device's native screenshot capabilities. This " 'screenshots the entire screen currently being displayed (including content ' 'not rendered by Flutter, like the device status bar).', _kSkiaType: 'Render the Flutter app as a Skia picture. Requires "--$_kVmServiceUrl".', - _kRasterizerType: 'Render the Flutter app using the rasterizer. Requires "--$_kVmServiceUrl."', }, defaultsTo: _kDeviceType, ); @@ -116,8 +114,6 @@ class ScreenshotCommand extends FlutterCommand { await runScreenshot(outputFile); case _kSkiaType: success = await runSkia(outputFile); - case _kRasterizerType: - success = await runRasterizer(outputFile); } return success ? FlutterCommandResult.success() @@ -173,30 +169,6 @@ class ScreenshotCommand extends FlutterCommand { return true; } - Future runRasterizer(File? outputFile) async { - final Uri vmServiceUrl = Uri.parse(stringArg(_kVmServiceUrl)!); - final FlutterVmService vmService = await connectToVmService(vmServiceUrl, logger: globals.logger); - final vm_service.Response? response = await vmService.screenshot(); - if (response == null) { - globals.printError( - 'The screenshot request failed, probably because the device was ' - 'disconnected', - ); - return false; - } - outputFile ??= globals.fsUtils.getUniqueFile( - fs.currentDirectory, - 'flutter', - 'png', - ); - final IOSink sink = outputFile.openWrite(); - sink.add(base64.decode(response.json?['screenshot'] as String)); - await sink.close(); - _showOutputFileInfo(outputFile); - ensureOutputIsNotJsonRpcError(outputFile); - return true; - } - static void checkOutput(File outputFile, FileSystem fs) { if (!fs.file(outputFile.path).existsSync()) { throwToolExit( diff --git a/packages/flutter_tools/lib/src/resident_runner.dart b/packages/flutter_tools/lib/src/resident_runner.dart index 742132b908c..1b63932ce60 100644 --- a/packages/flutter_tools/lib/src/resident_runner.dart +++ b/packages/flutter_tools/lib/src/resident_runner.dart @@ -1017,17 +1017,17 @@ abstract class ResidentHandlers { } Future _takeVmServiceScreenshot(FlutterDevice device, File outputFile) async { - final bool isWebDevice = device.targetPlatform == TargetPlatform.web_javascript; + if (device.targetPlatform != TargetPlatform.web_javascript) { + return false; + } assert(supportsServiceProtocol); return _toggleDebugBanner(device, () async { - final vm_service.Response? response = isWebDevice - ? await device.vmService!.callMethodWrapper('ext.dwds.screenshot') - : await device.vmService!.screenshot(); + final vm_service.Response? response = await device.vmService!.callMethodWrapper('ext.dwds.screenshot'); if (response == null) { throw Exception('Failed to take screenshot'); } - final String data = response.json![isWebDevice ? 'data' : 'screenshot'] as String; + final String data = response.json!['data'] as String; outputFile.writeAsBytesSync(base64.decode(data)); }); } diff --git a/packages/flutter_tools/lib/src/vmservice.dart b/packages/flutter_tools/lib/src/vmservice.dart index 78a5b92bd77..60874eccd4d 100644 --- a/packages/flutter_tools/lib/src/vmservice.dart +++ b/packages/flutter_tools/lib/src/vmservice.dart @@ -28,7 +28,6 @@ const String kFlushUIThreadTasksMethod = '_flutter.flushUIThreadTasks'; const String kRunInViewMethod = '_flutter.runInView'; const String kListViewsMethod = '_flutter.listViews'; const String kScreenshotSkpMethod = '_flutter.screenshotSkp'; -const String kScreenshotMethod = '_flutter.screenshot'; const String kRenderFrameWithRasterStatsMethod = '_flutter.renderFrameWithRasterStats'; const String kReloadAssetFonts = '_flutter.reloadAssetFonts'; @@ -1054,10 +1053,6 @@ class FlutterVmService { ); } - Future screenshot() { - return _checkedCallServiceExtension(kScreenshotMethod); - } - Future screenshotSkp() { return _checkedCallServiceExtension(kScreenshotSkpMethod); } diff --git a/packages/flutter_tools/test/commands.shard/hermetic/screenshot_command_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/screenshot_command_test.dart index 42954dc440a..01725cc9913 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/screenshot_command_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/screenshot_command_test.dart @@ -31,11 +31,6 @@ void main() { .run(['screenshot', '--type=skia', '--vm-service-url=http://localhost:8181']), throwsA(isException.having((Exception exception) => exception.toString(), 'message', contains('dummy'))), ); - - await expectLater(() => createTestCommandRunner(ScreenshotCommand(fs: MemoryFileSystem.test())) - .run(['screenshot', '--type=rasterizer', '--vm-service-url=http://localhost:8181']), - throwsA(isException.having((Exception exception) => exception.toString(), 'message', contains('dummy'))), - ); }); @@ -44,11 +39,6 @@ void main() { .run(['screenshot', '--type=skia']), throwsToolExit(message: 'VM Service URI must be specified for screenshot type skia') ); - - await expectLater(() => createTestCommandRunner(ScreenshotCommand(fs: MemoryFileSystem.test())) - .run(['screenshot', '--type=rasterizer',]), - throwsToolExit(message: 'VM Service URI must be specified for screenshot type rasterizer'), - ); }); testUsingContext('device screenshots require device', () async { diff --git a/packages/flutter_tools/test/general.shard/terminal_handler_test.dart b/packages/flutter_tools/test/general.shard/terminal_handler_test.dart index e6a38e5d39a..33f615c85ad 100644 --- a/packages/flutter_tools/test/general.shard/terminal_handler_test.dart +++ b/packages/flutter_tools/test/general.shard/terminal_handler_test.dart @@ -1015,38 +1015,14 @@ void main() { expect(logger.statusText, contains('Screenshot written to flutter_01.png (0kB)')); }); - testWithoutContext('s, can take screenshot on debug device that does not support screenshot', () async { + testWithoutContext('s, will not take screenshot on non-web device without screenshot tooling support', () async { final BufferLogger logger = BufferLogger.test(); final FileSystem fileSystem = MemoryFileSystem.test(); - final TerminalHandler terminalHandler = setUpTerminalHandler([ - listViews, - FakeVmServiceRequest( - method: 'ext.flutter.debugAllowBanner', - args: { - 'isolateId': fakeUnpausedIsolate.id, - 'enabled': 'false', - }, - ), - FakeVmServiceRequest( - method: '_flutter.screenshot', - args: {}, - jsonResponse: { - 'screenshot': base64.encode([1, 2, 3, 4]), - }, - ), - FakeVmServiceRequest( - method: 'ext.flutter.debugAllowBanner', - args: { - 'isolateId': fakeUnpausedIsolate.id, - 'enabled': 'true', - }, - ), - ], logger: logger, fileSystem: fileSystem); + final TerminalHandler terminalHandler = setUpTerminalHandler([], logger: logger, fileSystem: fileSystem); await terminalHandler.processTerminalInput('s'); - expect(logger.statusText, contains('Screenshot written to flutter_01.png (0kB)')); - expect(fileSystem.currentDirectory.childFile('flutter_01.png').readAsBytesSync(), [1, 2, 3, 4]); + expect(logger.statusText, isNot(contains('Screenshot written to'))); }); testWithoutContext('s, can take screenshot on debug web device that does not support screenshot', () async { @@ -1133,66 +1109,6 @@ void main() { expect(fileSystem.currentDirectory.childFile('flutter_01.png'), isNot(exists)); }); - testWithoutContext('s, bails taking screenshot on debug device if debugAllowBanner throws RpcError', () async { - final BufferLogger logger = BufferLogger.test(); - final FileSystem fileSystem = MemoryFileSystem.test(); - final TerminalHandler terminalHandler = setUpTerminalHandler( - [ - listViews, - FakeVmServiceRequest( - method: 'ext.flutter.debugAllowBanner', - args: { - 'isolateId': fakeUnpausedIsolate.id, - 'enabled': 'false', - }, - // Failed response, - errorCode: RPCErrorCodes.kInternalError, - ), - ], - logger: logger, - fileSystem: fileSystem, - ); - - await terminalHandler.processTerminalInput('s'); - - expect(logger.errorText, contains('Error')); - }); - - testWithoutContext('s, bails taking screenshot on debug device if flutter.screenshot throws RpcError, restoring banner', () async { - final BufferLogger logger = BufferLogger.test(); - final FileSystem fileSystem = MemoryFileSystem.test(); - final TerminalHandler terminalHandler = setUpTerminalHandler( - [ - listViews, - FakeVmServiceRequest( - method: 'ext.flutter.debugAllowBanner', - args: { - 'isolateId': fakeUnpausedIsolate.id, - 'enabled': 'false', - }, - ), - const FakeVmServiceRequest( - method: '_flutter.screenshot', - // Failed response, - errorCode: RPCErrorCodes.kInternalError, - ), - FakeVmServiceRequest( - method: 'ext.flutter.debugAllowBanner', - args: { - 'isolateId': fakeUnpausedIsolate.id, - 'enabled': 'true', - }, - ), - ], - logger: logger, - fileSystem: fileSystem, - ); - - await terminalHandler.processTerminalInput('s'); - - expect(logger.errorText, contains('Error')); - }); - testWithoutContext('s, bails taking screenshot on debug device if dwds.screenshot throws RpcError, restoring banner', () async { final BufferLogger logger = BufferLogger.test(); final FileSystem fileSystem = MemoryFileSystem.test(); diff --git a/packages/flutter_tools/test/general.shard/vmservice_test.dart b/packages/flutter_tools/test/general.shard/vmservice_test.dart index 73e710c6567..2557b3a2897 100644 --- a/packages/flutter_tools/test/general.shard/vmservice_test.dart +++ b/packages/flutter_tools/test/general.shard/vmservice_test.dart @@ -442,10 +442,6 @@ void main() { method: kListViewsMethod, errorCode: RPCErrorCodes.kServiceDisappeared, ), - const FakeVmServiceRequest( - method: kScreenshotMethod, - errorCode: RPCErrorCodes.kServiceDisappeared, - ), const FakeVmServiceRequest( method: kScreenshotSkpMethod, errorCode: RPCErrorCodes.kServiceDisappeared, @@ -480,9 +476,6 @@ void main() { final List views = await fakeVmServiceHost.vmService.getFlutterViews(); expect(views, isEmpty); - final vm_service.Response? screenshot = await fakeVmServiceHost.vmService.screenshot(); - expect(screenshot, isNull); - final vm_service.Response? screenshotSkp = await fakeVmServiceHost.vmService.screenshotSkp(); expect(screenshotSkp, isNull);