Switch to using Path URL Strategy (#3585)

* Switch to using Path URL Strategy

* Fix import

* Update extraction of page from the URL for survey

* More extractCurrentPageFromUrl + tests into url_utils

* Ensure page URL includes the current tab when there was no existing page set

* Allow navigating from /?page=foo to /bar correctly

* Restore accidentally deleted line

* Fix navigating during initState to fix URL

* Change Future.microtask to WidgetsBinding.instance.addPostFrameCallback

* Fix analysis errors after rebase on latest

* Add a comment about base href being replaced

* Handle legacy URLs by redirecting them to new style URLs

* Fix require_trailing_comma lints

* Move url_strategy to config_specific

* Remove redundant code/tests

Author: DanTup

packages/devtools_app/lib/main.dart

@@ -9,15 +9,24 @@ import 'src/analytics/analytics_controller.dart';
 import 'src/app.dart';
 import 'src/config_specific/framework_initialize/framework_initialize.dart';
 import 'src/config_specific/ide_theme/ide_theme.dart';
+import 'src/config_specific/url/url.dart';
+import 'src/config_specific/url_strategy/url_strategy.dart';
 import 'src/extension_points/extensions_base.dart';
 import 'src/extension_points/extensions_external.dart';
+import 'src/primitives/url_utils.dart';
 import 'src/screens/debugger/syntax_highlighter.dart';
 import 'src/screens/provider/riverpod_error_logger_observer.dart';
 import 'src/shared/app_error_handling.dart';
 import 'src/shared/globals.dart';
 import 'src/shared/preferences.dart';
 
 void main() async {
+  // Before switching to URL path strategy, check if this URL is in the legacy
+  // fragment format and redirect if necessary.
+  if (_handleLegacyUrl()) return;
+
+  usePathUrlStrategy();
+
   // Initialize the framework before we do anything else, otherwise the
   // StorageController won't be initialized and preferences won't be loaded.
   await initializeFramework();
@@ -44,3 +53,21 @@ void main() async {
     );
   });
 }
+
+/// Checks if the request is for a legacy URL and if so, redirects to the new
+/// equivalent.
+///
+/// Returns `true` if a redirect was performed, in which case normal app
+/// initialization should be skipped.
+bool _handleLegacyUrl() {
+  final url = getWebUrl();
+  if (url == null) return false;
+
+  final newUrl = mapLegacyUrl(url);
+  if (newUrl != null) {
+    webRedirect(newUrl);
+    return true;
+  }
+
+  return false;
+}

packages/devtools_app/lib/src/analytics/_analytics_web.dart

@@ -17,6 +17,7 @@ import '../app.dart';
 import '../config_specific/logger/logger.dart';
 import '../config_specific/server/server.dart' as server;
 import '../config_specific/url/url.dart';
+import '../primitives/url_utils.dart';
 import '../screens/performance/performance_screen.dart';
 import '../screens/profiler/profiler_screen.dart';
 import '../shared/globals.dart';
@@ -732,29 +733,8 @@ Map<String, dynamic> generateSurveyQueryParameters() {
   const fromKey = 'From';
   const internalKey = 'Internal';
 
-  // TODO(https://github.com/flutter/devtools/issues/2475): fix url structure
-  // Parsing the url via Uri.parse returns an incorrect value for fragment.
-  // Grab the fragment value manually. The url will be of the form
-  // http://127.0.0.1:9100/#/timeline?ide=IntelliJ-IDEA&uri=..., and we want the
-  // part equal to '/timeline'.
-  final url = window.location.toString();
-  const fromValuePrefix = '#/';
-  final startIndex = url.indexOf(fromValuePrefix);
-  // Use the last index because the url can be of the form
-  // 'http://127.0.0.1:9103/?#/?' and we want to be referencing the last '?'
-  // character.
-  final endIndex = url.lastIndexOf('?');
-  var fromPage = '';
-  try {
-    fromPage = url.substring(
-      startIndex + fromValuePrefix.length,
-      endIndex,
-    );
-  } catch (_) {
-    // Fail gracefully if finding the [fromPage] value throws an exception.
-  }
-
   final internalValue = (!isExternalBuild).toString();
+  final fromPage = extractCurrentPageFromUrl(window.location.toString());
 
   return {
     ideKey: ideLaunched,

packages/devtools_app/lib/src/config_specific/url/_url_stub.dart

@@ -3,3 +3,13 @@
 // in the LICENSE file.
 
 Map<String, String> loadQueryParams() => {};
+
+/// Gets the URL from the browser.
+///
+/// Returns null for non-web platforms.
+String? getWebUrl() => null;
+
+/// Performs a web redirect using window.location.replace().
+///
+/// No-op for non-web platforms.
+void webRedirect(String url) {}

packages/devtools_app/lib/src/config_specific/url/_url_web.dart

@@ -9,3 +9,9 @@ import '../../primitives/utils.dart';
 Map<String, String> loadQueryParams() {
   return devToolsQueryParams(window.location.toString());
 }
+
+String? getWebUrl() => window.location.toString();
+
+void webRedirect(String url) {
+  window.location.replace(url);
+}

packages/devtools_app/lib/src/config_specific/url_strategy/noop.dart

@@ -0,0 +1,3 @@
+void usePathUrlStrategy() {
+  // No-op. URL Strategies are only supported for web.
+}

packages/devtools_app/lib/src/config_specific/url_strategy/url_strategy.dart

@@ -0,0 +1 @@
+export 'noop.dart' if (dart.library.html) 'web.dart';

packages/devtools_app/lib/src/config_specific/url_strategy/web.dart

@@ -0,0 +1,5 @@
+import 'package:flutter_web_plugins/flutter_web_plugins.dart';
+
+void usePathUrlStrategy() {
+  setUrlStrategy(PathUrlStrategy());
+}

packages/devtools_app/lib/src/primitives/url_utils.dart

@@ -0,0 +1,50 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/// Extracts the current DevTools page from the given [url].
+String extractCurrentPageFromUrl(String url) {
+  // The url can be in one of two forms:
+  // - /page?uri=xxx
+  // - /?page=xxx&uri=yyy (original formats IDEs may use)
+  // Use the path in preference to &page= as it's the one DevTools is updating
+  final uri = Uri.parse(url);
+  return uri.path == '/'
+      ? uri.queryParameters['page'] ?? ''
+      : uri.path.substring(1);
+}
+
+/// Maps DevTools URLs in the original fragment format onto the equivalent URLs
+/// in the new URL format.
+///
+/// Returns `null` if [url] is not a legacy URL.
+String? mapLegacyUrl(String url) {
+  final uri = Uri.parse(url);
+  // Old formats include:
+  //   http://localhost:123/#/inspector?uri=ws://...
+  //   http://localhost:123/#/?page=inspector&uri=ws://...
+  final isRootRequest = uri.path == '/' || uri.path.endsWith('/devtools/');
+  if (isRootRequest && uri.fragment.isNotEmpty) {
+    final basePath = uri.path;
+    // Convert the URL by removing the fragment separator.
+    final newUrl = url
+        // Handle localhost:123/#/inspector?uri=xxx
+        .replaceFirst('/#/', '/')
+        // Handle localhost:123/#?page=inspector&uri=xxx
+        .replaceFirst('/#', '');
+
+    // Move page names from the querystring into the path.
+    var newUri = Uri.parse(newUrl);
+    final page = newUri.queryParameters['page'];
+    if (newUri.path == basePath && page != null) {
+      final newParams = {...newUri.queryParameters}..remove('page');
+      newUri = newUri.replace(
+        path: '$basePath$page',
+        queryParameters: newParams,
+      );
+    }
+    return newUri.toString();
+  }
+
+  return null;
+}

packages/devtools_app/lib/src/shared/routing.dart

@@ -139,7 +139,7 @@ class DevToolsRouterDelegate extends RouterDelegate<DevToolsRouteConfiguration>
   /// If page and args would be the same, does nothing.
   /// Existing arguments (for example &uri=) will be preserved unless
   /// overwritten by [argUpdates].
-  void navigateIfNotCurrent(String page, [Map<String, String>? argUpdates]) {
+  void navigateIfNotCurrent(String page, [Map<String, String?>? argUpdates]) {
     final pageChanged = page != currentConfiguration!.page;
     final argsChanged = _changesArgs(argUpdates);
     if (!pageChanged && !argsChanged) {
@@ -211,7 +211,7 @@ class DevToolsRouterDelegate extends RouterDelegate<DevToolsRouteConfiguration>
 
   /// Checks whether applying [changes] over the current routes args will result
   /// in any changes.
-  bool _changesArgs(Map<String, String>? changes) => !mapEquals(
+  bool _changesArgs(Map<String, String?>? changes) => !mapEquals(
         {...currentConfiguration!.args, ...?changes},
         {...currentConfiguration!.args},
       );

packages/devtools_app/lib/src/shared/scaffold.dart

@@ -215,21 +215,27 @@ class DevToolsScaffoldState extends State<DevToolsScaffold>
         // Clear error count when navigating to a screen.
         serviceManager.errorBadgeManager.clearErrors(screen.screenId);
 
-        // If the tab index is 0 and the current route has no page ID (eg. we're
-        // at the URL /?uri= with no page ID), those are equivalent pages but
-        // navigateIfNotCurrent does not know that and will try to navigate, so
-        // skip that here.
-        final routerDelegate = DevToolsRouterDelegate.of(context);
-        if (_tabController!.index == 0 &&
-            (routerDelegate.currentConfiguration!.page.isEmpty)) {
-          return;
-        }
-
         // Update routing with the change.
+        final routerDelegate = DevToolsRouterDelegate.of(context);
         routerDelegate.navigateIfNotCurrent(screen.screenId);
       }
     });
 
+    // If we had no explicit page, we want to write one into the URL but
+    // without triggering a navigation. Since we can't nagivate during a build
+    // we have to wrap this in `Future.microtask`.
+    if (widget.page == null && _currentScreen is! SimpleScreen) {
+      WidgetsBinding.instance.addPostFrameCallback((_) {
+        final routerDelegate = DevToolsRouterDelegate.of(context);
+        Router.neglect(context, () {
+          routerDelegate.navigateIfNotCurrent(
+            _currentScreen.screenId,
+            routerDelegate.currentConfiguration?.args,
+          );
+        });
+      });
+    }
+
     // Broadcast the initial page.
     frameworkController.notifyPageChange(
       PageChangeEvent(_currentScreen.screenId, widget.embed),

packages/devtools_app/test/url_utils_test.dart

@@ -0,0 +1,80 @@
+// Copyright 2019 The Chromium Authors. 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:devtools_app/src/primitives/url_utils.dart';
+import 'package:flutter_test/flutter_test.dart';
+
+void main() {
+  group('url utils', () {
+    group('extractCurrentPageFromUrl', () {
+      test('parses the current page from the path', () {
+        final page =
+            extractCurrentPageFromUrl('http://localhost:9000/inspector?uri=x');
+        expect(page, 'inspector');
+      });
+
+      test('parses the current page from the query string', () {
+        final page = extractCurrentPageFromUrl(
+          'http://localhost:9000/?uri=x&page=inspector&theme=dark',
+        );
+        expect(page, 'inspector');
+      });
+
+      test(
+          'parses the current page from the path even if query string is populated',
+          () {
+        final page = extractCurrentPageFromUrl(
+          'http://localhost:9000/memory?uri=x&page=inspector&theme=dark',
+        );
+        expect(page, 'memory');
+      });
+    });
+
+    group('mapLegacyUrl', () {
+      for (final prefix in [
+        'http://localhost:123',
+        'http://localhost:123/authToken=/devtools'
+      ]) {
+        group(' with $prefix prefix', () {
+          test('does not map new-style URLs', () {
+            expect(mapLegacyUrl('$prefix'), isNull);
+            expect(mapLegacyUrl('$prefix/'), isNull);
+            expect(mapLegacyUrl('$prefix/foo?uri=ws://foo'), isNull);
+            expect(mapLegacyUrl('$prefix?uri=ws://foo'), isNull);
+            expect(mapLegacyUrl('$prefix/?uri=ws://foo'), isNull);
+            expect(mapLegacyUrl('$prefix/?uri=ws://foo#'), isNull);
+          });
+
+          test('maps legacy URIs with page names in path', () {
+            expect(
+              mapLegacyUrl('$prefix/#/inspector?foo=bar'),
+              '$prefix/inspector?foo=bar',
+            );
+          });
+
+          test('maps legacy URIs with page names in querystring', () {
+            expect(
+              mapLegacyUrl('$prefix/#/?page=inspector&foo=bar'),
+              '$prefix/inspector?foo=bar',
+            );
+          });
+
+          test('maps legacy URIs with no page names', () {
+            expect(
+              mapLegacyUrl('$prefix/#/?foo=bar'),
+              '$prefix/?foo=bar',
+            );
+          });
+        });
+      }
+    });
+  });
+}
+
+const dartSdkUrl =
+    'org-dartlang-sdk:///third_party/dart/sdk/lib/async/zone.dart';
+const flutterUrl =
+    'file:///path/to/flutter/packages/flutter/lib/src/widgets/binding.dart';
+const flutterUrlFromNonFlutterDir =
+    'file:///path/to/non-flutter/packages/flutter/lib/src/widgets/binding.dart';

packages/devtools_app/web/index.html

@@ -10,6 +10,8 @@
 <head>
     <meta charset="utf-8">
     <meta http-equiv="X-UA-Compatible" content="IE=edge">
+    <!-- Note: This tag is replaced when served through DDS! -->
+    <base href="/">
 
     <title></title>
     <link href="favicon.png" rel="icon" sizes="64x64">
@@ -41,18 +43,6 @@
         if (!supportsES6Classes()) {
             window.location.href = '/unsupported-browser.html';
         }
-
-        // Handle URLs that pass all variables directly on a querystring without
-        // the fragment (for ex. VS Code while it has some encoding bugs preventing
-        // building the correct URLs using fragments
-        // https://github.com/microsoft/vscode/issues/85930).
-        if (window.location.search && window.location.search.length > 1) {
-          // Ensure each component is encoded, because if the URI contains / slashes
-          // Flutter will split on them and try to push multiple routes.
-          const params = new URLSearchParams(unescape(window.location.search));
-          params.forEach(function(v, k) { params.set(k, encodeURIComponent(v)) });
-          window.location.replace(window.location.origin + '/#/?' + params.toString());
-        }
     </script>
 </head>