From acf43c758701033db42d2ced6ae9a42b01d58fac Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Thu, 19 Dec 2024 12:54:42 -0800 Subject: [PATCH] [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 https://github.com/Dart-Code/Dart-Code/issues/5365 Change-Id: I80ff4d78a3df1f77740a9b8f61ef5499bd008eda Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/399681 Commit-Queue: Ben Konyi Reviewed-by: Kenzie Davisson Reviewed-by: Ben Konyi --- pkg/dds/lib/devtools_server.dart | 31 +++++----- pkg/dds/test/devtools_server/uri_test.dart | 67 ++++++++++++++++++++++ 2 files changed, 84 insertions(+), 14 deletions(-) create mode 100644 pkg/dds/test/devtools_server/uri_test.dart diff --git a/pkg/dds/lib/devtools_server.dart b/pkg/dds/lib/devtools_server.dart index 684fb61c6e5..909d9579b0a 100644 --- a/pkg/dds/lib/devtools_server.dart +++ b/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 uriParams, - String? page, + static String buildUriToLaunch( Uri devToolsUri, + String? page, + Map? 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(); } diff --git a/pkg/dds/test/devtools_server/uri_test.dart b/pkg/dds/test/devtools_server/uri_test.dart new file mode 100644 index 00000000000..ad0ccba7d0b --- /dev/null +++ b/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'); + }); + }); +}