[dds] Build DevTools URL using querystring, not fragment

DevTools now uses page URLs so we don't need to put the querystring as a fragment. The original code did work, but it complicates things for other code (such as code injecting `?wasm=true`) so should be updated.

Fixes Dart-Code/Dart-Code#5365

Change-Id: I80ff4d78a3df1f77740a9b8f61ef5499bd008eda
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/399681
Commit-Queue: Ben Konyi <[email protected]>
Reviewed-by: Kenzie Davisson <[email protected]>
Reviewed-by: Ben Konyi <[email protected]>

Author: DanTup

pkg/dds/lib/devtools_server.dart

@@ -647,7 +647,11 @@ class DevToolsServer {
     uriParams['uri'] = vmServiceUri.toString();
 
     final devToolsUri = Uri.parse(devToolsUrl);
-    final uriToLaunch = _buildUriToLaunch(uriParams, page, devToolsUri);
+    final uriToLaunch = buildUriToLaunch(
+      devToolsUri,
+      page,
+      uriParams,
+    );
 
     // TODO(dantup): When ChromeOS has support for tunneling all ports we can
     // change this to always use the native browser for ChromeOS and may wish to
@@ -746,23 +750,22 @@ class DevToolsServer {
     return false;
   }
 
-  String _buildUriToLaunch(
-    Map<String, dynamic> uriParams,
-    String? page,
+  static String buildUriToLaunch(
     Uri devToolsUri,
+    String? page,
+    Map<String, dynamic>? params,
   ) {
-    final queryStringNameValues = [];
-    uriParams.forEach((key, value) => queryStringNameValues.add(
-        '${Uri.encodeQueryComponent(key)}=${Uri.encodeQueryComponent(value)}'));
-
-    if (page != null) {
-      queryStringNameValues.add('page=${Uri.encodeQueryComponent(page)}');
-    }
-
+    page ??= '';
+    var pathSep = devToolsUri.path.endsWith('/') ? '' : '/';
+    var newPath = '${devToolsUri.path}$pathSep$page';
+    var newParams = {
+      ...devToolsUri.queryParameters,
+      ...?params,
+    };
     return devToolsUri
         .replace(
-            path: devToolsUri.path.isEmpty ? '/' : devToolsUri.path,
-            fragment: '?${queryStringNameValues.join('&')}')
+            path: newPath,
+            queryParameters: newParams.isNotEmpty ? newParams : null)
         .toString();
   }
 

pkg/dds/test/devtools_server/uri_test.dart

@@ -0,0 +1,67 @@
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:dds/devtools_server.dart';
+import 'package:test/test.dart';
+
+void main() {
+  group('DevToolsServer.buildUriToLaunch', () {
+    var build = DevToolsServer.buildUriToLaunch;
+
+    test('with no trailing slash', () async {
+      var base = Uri.parse('http://localhost:1235');
+      expect(build(base, null, {}), 'http://localhost:1235/');
+      expect(build(base, 'inspector', {}), 'http://localhost:1235/inspector');
+      expect(build(base, null, {'a': 'a'}), 'http://localhost:1235/?a=a');
+      expect(build(base, 'inspector', {'a': 'a'}),
+          'http://localhost:1235/inspector?a=a');
+    });
+
+    test('with trailing slash', () async {
+      var base = Uri.parse('http://localhost:1235/');
+
+      expect(build(base, null, {}), 'http://localhost:1235/');
+      expect(build(base, 'inspector', {}), 'http://localhost:1235/inspector');
+      expect(build(base, null, {'a': 'a'}), 'http://localhost:1235/?a=a');
+      expect(build(base, 'inspector', {'a': 'a'}),
+          'http://localhost:1235/inspector?a=a');
+    });
+
+    test('with folder and no trailing slash', () async {
+      var base = Uri.parse('http://localhost:1235/devtools');
+      expect(build(base, null, {}), 'http://localhost:1235/devtools/');
+      expect(build(base, 'inspector', {}),
+          'http://localhost:1235/devtools/inspector');
+      expect(
+          build(base, null, {'a': 'a'}), 'http://localhost:1235/devtools/?a=a');
+      expect(build(base, 'inspector', {'a': 'a'}),
+          'http://localhost:1235/devtools/inspector?a=a');
+    });
+
+    test('with folder and trailing slash', () async {
+      var base = Uri.parse('http://localhost:1235/devtools/');
+
+      expect(build(base, null, {}), 'http://localhost:1235/devtools/');
+      expect(build(base, 'inspector', {}),
+          'http://localhost:1235/devtools/inspector');
+      expect(
+          build(base, null, {'a': 'a'}), 'http://localhost:1235/devtools/?a=a');
+      expect(build(base, 'inspector', {'a': 'a'}),
+          'http://localhost:1235/devtools/inspector?a=a');
+    });
+
+    test('with existing query params', () async {
+      var base = Uri.parse('http://localhost:1235/devtools/?a=orig&b=b');
+
+      expect(
+          build(base, null, {}), 'http://localhost:1235/devtools/?a=orig&b=b');
+      expect(build(base, 'inspector', {}),
+          'http://localhost:1235/devtools/inspector?a=orig&b=b');
+      expect(build(base, null, {'a': 'a', 'c': 'c'}),
+          'http://localhost:1235/devtools/?a=a&b=b&c=c');
+      expect(build(base, 'inspector', {'a': 'a', 'c': 'c'}),
+          'http://localhost:1235/devtools/inspector?a=a&b=b&c=c');
+    });
+  });
+}