Skip to content
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

[flutter_tools] remove VmService screenshot for native devices. #135462

Merged
merged 6 commits into from
Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 2 additions & 30 deletions packages/flutter_tools/lib/src/commands/screenshot.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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}) {
Expand All @@ -33,21 +32,20 @@ class ScreenshotCommand extends FlutterCommand {
aliases: <String>[ '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.',
);
argParser.addOption(
_kType,
valueHelp: 'type',
help: 'The type of screenshot to retrieve.',
allowed: const <String>[_kDeviceType, _kSkiaType, _kRasterizerType],
allowed: const <String>[_kDeviceType, _kSkiaType],
allowedHelp: const <String, String>{
_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."',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this PR is only removing the ability to do flutter screenshot --type=rasterizer --vm-service-url=$URL?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, though the Skia picture type will also eventually become irrelevant as that is only used for SKP debugging

},
defaultsTo: _kDeviceType,
);
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -173,30 +169,6 @@ class ScreenshotCommand extends FlutterCommand {
return true;
}

Future<bool> 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(
Expand Down
10 changes: 5 additions & 5 deletions packages/flutter_tools/lib/src/resident_runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1017,17 +1017,17 @@ abstract class ResidentHandlers {
}

Future<bool> _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));
});
}
Expand Down
5 changes: 0 additions & 5 deletions packages/flutter_tools/lib/src/vmservice.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -1054,10 +1053,6 @@ class FlutterVmService {
);
}

Future<vm_service.Response?> screenshot() {
return _checkedCallServiceExtension(kScreenshotMethod);
}

Future<vm_service.Response?> screenshotSkp() {
return _checkedCallServiceExtension(kScreenshotSkpMethod);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,6 @@ void main() {
.run(<String>['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(<String>['screenshot', '--type=rasterizer', '--vm-service-url=http://localhost:8181']),
throwsA(isException.having((Exception exception) => exception.toString(), 'message', contains('dummy'))),
);
});


Expand All @@ -44,11 +39,6 @@ void main() {
.run(<String>['screenshot', '--type=skia']),
throwsToolExit(message: 'VM Service URI must be specified for screenshot type skia')
);

await expectLater(() => createTestCommandRunner(ScreenshotCommand(fs: MemoryFileSystem.test()))
.run(<String>['screenshot', '--type=rasterizer',]),
throwsToolExit(message: 'VM Service URI must be specified for screenshot type rasterizer'),
);
});

testUsingContext('device screenshots require device', () async {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(<FakeVmServiceRequest>[
listViews,
FakeVmServiceRequest(
method: 'ext.flutter.debugAllowBanner',
args: <String, Object?>{
'isolateId': fakeUnpausedIsolate.id,
'enabled': 'false',
},
),
FakeVmServiceRequest(
method: '_flutter.screenshot',
args: <String, Object>{},
jsonResponse: <String, Object>{
'screenshot': base64.encode(<int>[1, 2, 3, 4]),
},
),
FakeVmServiceRequest(
method: 'ext.flutter.debugAllowBanner',
args: <String, Object?>{
'isolateId': fakeUnpausedIsolate.id,
'enabled': 'true',
},
),
], logger: logger, fileSystem: fileSystem);
final TerminalHandler terminalHandler = setUpTerminalHandler(<FakeVmServiceRequest>[], 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(), <int>[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 {
Expand Down Expand Up @@ -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(
<FakeVmServiceRequest>[
listViews,
FakeVmServiceRequest(
method: 'ext.flutter.debugAllowBanner',
args: <String, Object?>{
'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(
<FakeVmServiceRequest>[
listViews,
FakeVmServiceRequest(
method: 'ext.flutter.debugAllowBanner',
args: <String, Object?>{
'isolateId': fakeUnpausedIsolate.id,
'enabled': 'false',
},
),
const FakeVmServiceRequest(
method: '_flutter.screenshot',
// Failed response,
errorCode: RPCErrorCodes.kInternalError,
),
FakeVmServiceRequest(
method: 'ext.flutter.debugAllowBanner',
args: <String, Object?>{
'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();
Expand Down
7 changes: 0 additions & 7 deletions packages/flutter_tools/test/general.shard/vmservice_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -480,9 +476,6 @@ void main() {
final List<FlutterView> 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);

Expand Down