Skip to content

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Apr 14, 2021

Part 1 of #80440

The goal of this PR is to move the resident runners closer to a state where enough of the implementation is shared to enable 1) profile/release/debug autodetect for flutter attach and 2) simultaneous connections to web and non-web devices. I also wanted to remove the vast amounts of duplicated test logic due to the web and non-web resident runners having different service extension impls as well as remove the mocks from the terminal handler test and re-imagine it as a fairly high level interaction test.

Changes in resident_runner.dart:

  • Creates a new class ResidentHandlers which contains the vm service delegation, make it easier to share with the web impl for now. This required exposing Logger and FileSystem as protected members.
    ( Removes all of the wrapper vm service functionality from the FlutterDevice class. Now that the logic is in a single place it doesn't make as much sense to split it up.
  • Updates FlutterDevice to conditionally use getVM's isolate list instead of listing the flutter views. The flutter view RPC is not supported on the web, but since there is only ever one isolate we can use the VM list. This required exposing the TargetPlatform field on FlutterDevice, which was already part of the constructor.

This does not update all of the test cases, but does delete the old test cases that interacted with a FlutterDevice directly.
This does not replace the web implementation, that will be done in part 2

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 14, 2021
@google-cla google-cla bot added the cla: yes label Apr 14, 2021
@jonahwilliams jonahwilliams marked this pull request as ready for review April 15, 2021 15:26
@jonahwilliams jonahwilliams requested a review from jmagman April 15, 2021 15:40

/// Appends a number to a filename in order to make it unique under a
/// directory.
File getUniqueFile(Directory dir, String baseName, String ext) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getUniqueFile does not need any state from the FSUtils class, so making it a top level method

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

LGTM

@jonahwilliams jonahwilliams merged commit 9904a7f into flutter:master Apr 17, 2021
@jonahwilliams jonahwilliams deleted the refactor_core_resident_runner_logic branch April 17, 2021 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants