[DevTools Server] Listen for URIs and register launchDevTools service. #555

kenzieschmoll · 2019-04-17T23:34:40Z

Listens for URIs over stdin
Connects to vm service port provided in URI and registers launchDevTools service / method
Adds code to launch chrome (this will be moved to dart-lang/browser_launcher when I create that package)
Fixes a bug with callMulticastService - delete method as it is not needed

packages/devtools_server/lib/src/chrome.dart

packages/devtools_server/lib/src/server.dart

DanTup · 2019-04-18T07:15:06Z

packages/devtools_server/lib/src/server.dart

+          vmServicePort =
+              vmServicePort.substring(0, vmServicePort.indexOf('/'));
+
+          final url = '$devtoolsUrl/?port=$vmServicePort#';


I think this will need to be something like uri=${encodeURIComponent(uri)}, we can't use raw ports numbers once the VM tokens land.

discussed offline. changing to final url = '$devtoolsUrl/url=${Uri.encodeComponent(uri.toString())}#';

packages/devtools_server/lib/src/chrome.dart

DanTup · 2019-04-18T07:22:31Z

packages/devtools_server/lib/src/server.dart

+    return json.decode(line) as Map<String, dynamic>;
+  });
+
+  // Example json input: [{"url":"ws://localhost:8888/ws"}]


I think this JSON should match the format of our other protocols and the output we send, with a method param, something like:

{ "method": "vm.register", "params": { "url": "blah://blah" } }

I wonder if we should also check machineMode? If we're not running in machine mode, maybe we don't need to parse this (or, we might want the interface to be simpler to paste a URL than to make it into JSON).

I missed out the ID from the example above. To work the same as our other machine modes (which is essentially JSON-RPC), the dialog would look like this:

Editor sends the request (which includes an ID):

{ "id": 1, "method": "vm.register", "params": { "url": "blah://blah" } }

Then the server should respond with either a result (which can be empty or null in this case, we don't really have any data to return):

{ "id": 1, "result": {} }

or an error:

{ "id": 1, "error": "Unable to connect to VM service" }

The code that handles this kind of thing for Flutter is here. The error message should be formatted such that it's reasonable for the editor to show the user to tell them something has gone wrong.

+1 on keeping the protocol consistent with other daemon style tools.
For now we only need to support the daemon version as this is for IDE users. Command line users are responsible for opening their own browser.

changed this to expect json in the above format.

packages/devtools_server/lib/src/server.dart

kenzieschmoll

Discussed most of this with Danny offline. Addressed comments.

packages/devtools_server/lib/src/chrome.dart

packages/devtools_server/lib/src/server.dart

kenzieschmoll · 2019-04-18T16:35:40Z

packages/devtools_server/lib/src/server.dart

+          vmServicePort =
+              vmServicePort.substring(0, vmServicePort.indexOf('/'));
+
+          final url = '$devtoolsUrl/?port=$vmServicePort#';


discussed offline. changing to final url = '$devtoolsUrl/url=${Uri.encodeComponent(uri.toString())}#';

kenzieschmoll · 2019-04-18T17:07:13Z

packages/devtools_server/lib/src/server.dart

+    return json.decode(line) as Map<String, dynamic>;
+  });
+
+  // Example json input: [{"url":"ws://localhost:8888/ws"}]


changed this to expect json in the above format.

packages/devtools_server/lib/src/server.dart

jacob314 · 2019-04-18T17:39:42Z

packages/devtools_server/lib/src/chrome.dart

+    int port,
+  }) async {
+    final dataDir = Directory.systemTemp.createTempSync();
+    args ??= [];


use Dart default arguments instead.

oddly enough, changing this to List<String> args = const [], breaks everything 🤔

you can't add elements to a const list. Create a new list with the args to pass to Process.start.
In general, it is bad practice to modify arguments passed in to a method when you don't need to so that way of writing the code is cleaner in general.

jacob314 · 2019-04-18T17:41:41Z

packages/devtools_server/lib/src/chrome.dart

@@ -92,8 +91,8 @@ class Chrome {
        .firstWhere((line) => line.startsWith('DevTools listening'));

    return _connect(Chrome._(


this doesn't make sense without the --remoteDebuggingPort option. Remove this code. Someone who wants to connect to the debug port can do that separately in a wrapper method.

actually never mind. we can keep this but we should handle a null debugPort better.

removed parts of this code we don't need. I will add them back when I create the browser_launcher package.

jacob314 · 2019-04-18T17:43:37Z

packages/devtools_server/lib/src/chrome.dart

@@ -55,35 +54,35 @@ class Chrome {

  /// Connects to an instance of Chrome with an open debug port.
  static Future<Chrome> fromExisting(int port) async =>
-      _connect(Chrome._(port, ChromeConnection('localhost', port)));
+      _connect(Chrome._(ChromeConnection('localhost', port), debugPort: port));


we shouldn't create a ChromeConnection when there isn't a chrome remote debugging port.

removed this code.

packages/devtools_server/lib/src/server.dart

jacob314 · 2019-04-19T18:05:08Z

packages/devtools_server/lib/src/chrome.dart

-    args.addAll(urls);
-
-    await Process.start(_executable, args);
+    final processArgs = List.from(urls)..addAll(args);


you switched the order of the urls and the args.
I would write
args.toList()..addAll(urls)

jacob314 · 2019-04-19T18:05:31Z

packages/devtools_server/lib/src/server.dart

-      final url = '$devToolsUrl/url=${Uri.encodeComponent(uri.toString())}#';
+      // TODO(kenzie): modify this to append arguments (i.e. theme=dark). This
+      // likely will require passing in args.
+      final url = '$devToolsUrl/?url=${Uri.encodeComponent(uri.toString())}#';


nit: why are we adding a '#' at the end of the url?

[DevTools Server] Listen for uris and register launchDevTools method.

5b8097c

kenzieschmoll requested review from DanTup and jacob314 April 17, 2019 23:34

Fix broken test.

d7b34b2

jacob314 reviewed Apr 18, 2019

View reviewed changes

packages/devtools_server/lib/src/chrome.dart Outdated Show resolved Hide resolved

jacob314 reviewed Apr 18, 2019

View reviewed changes

packages/devtools_server/lib/src/chrome.dart Outdated Show resolved Hide resolved

jacob314 reviewed Apr 18, 2019

View reviewed changes

packages/devtools_server/lib/src/server.dart Outdated Show resolved Hide resolved

jacob314 reviewed Apr 18, 2019

View reviewed changes

packages/devtools_server/lib/src/server.dart Outdated Show resolved Hide resolved

DanTup reviewed Apr 18, 2019

View reviewed changes

Review comments.

cb936b9

kenzieschmoll commented Apr 18, 2019

View reviewed changes

jacob314 reviewed Apr 18, 2019

View reviewed changes

remove obsolete code from chrome.dart

f36b5d6

DanTup reviewed Apr 19, 2019

View reviewed changes

packages/devtools_server/lib/src/server.dart Outdated Show resolved Hide resolved

packages/devtools_server/lib/src/server.dart Outdated Show resolved Hide resolved

address review comments

39b1ee1

jacob314 reviewed Apr 19, 2019

View reviewed changes

fixes.

72a5129

jacob314 approved these changes Apr 19, 2019

View reviewed changes

kenzieschmoll merged commit 5473bed into flutter:master Apr 19, 2019

kenzieschmoll deleted the launchDevtoolsService branch April 19, 2019 19:35

kenzieschmoll mentioned this pull request Apr 23, 2019

P0: Add a DevTools server that can launch the Chrome browser. #463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DevTools Server] Listen for URIs and register launchDevTools service. #555

[DevTools Server] Listen for URIs and register launchDevTools service. #555

kenzieschmoll commented Apr 17, 2019

DanTup Apr 18, 2019

kenzieschmoll Apr 18, 2019

jacob314 Apr 18, 2019

DanTup Apr 18, 2019

DanTup Apr 18, 2019

DanTup Apr 18, 2019

jacob314 Apr 18, 2019

kenzieschmoll Apr 18, 2019

kenzieschmoll left a comment

kenzieschmoll Apr 18, 2019

kenzieschmoll Apr 18, 2019

jacob314 Apr 18, 2019

kenzieschmoll Apr 18, 2019

jacob314 Apr 19, 2019

jacob314 Apr 18, 2019

jacob314 Apr 18, 2019

kenzieschmoll Apr 18, 2019

jacob314 Apr 18, 2019

kenzieschmoll Apr 18, 2019

jacob314 Apr 19, 2019

kenzieschmoll Apr 19, 2019

jacob314 Apr 19, 2019

kenzieschmoll Apr 19, 2019

		@@ -92,8 +91,8 @@ class Chrome {
		.firstWhere((line) => line.startsWith('DevTools listening'));

		return _connect(Chrome._(

[DevTools Server] Listen for URIs and register launchDevTools service. #555

[DevTools Server] Listen for URIs and register launchDevTools service. #555

Conversation

kenzieschmoll commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenzieschmoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment