Set breakpoints by regex URL (#1078)

- Use `setBreakpointsByUrl` to set breakpoints using a regex based on the module path
- Chrome now handles re-establishing breakpoints upon refresh so remove the corresponding deprecated argument

Closes #1076

Author: grouma

dwds/CHANGELOG.md

@@ -13,6 +13,8 @@
 
 **Breaking Changes:**
 - Require access to the `.ddc_merged_metadata` file.
+- Remove deprecated parameter `restoreBreakpoints` as breakpoints are now
+  set by regex URL and Chrome automatically reestablishes them.
 
 ## 5.0.0
 

dwds/lib/dwds.dart

@@ -93,7 +93,6 @@ class Dwds {
     // TODO(annagrin): make expressionCompiler argument required
     // [issue 881](https://github.com/dart-lang/webdev/issues/881)
     ExpressionCompiler expressionCompiler,
-    @deprecated bool restoreBreakpoints,
   }) async {
     hostname ??= 'localhost';
     enableDebugging ??= true;
@@ -104,7 +103,6 @@ class Dwds {
     logWriter ??= (level, message) => print(message);
     verbose ??= false;
     globalLoadStrategy = loadStrategy;
-    restoreBreakpoints ??= false;
 
     DevTools devTools;
     String extensionUri;
@@ -148,7 +146,6 @@ class Dwds {
       logWriter,
       extensionBackend,
       urlEncoder,
-      restoreBreakpoints,
       useSseForDebugProxy,
       serveDevTools,
       expressionCompiler,

dwds/lib/src/debugging/debugger.dart

@@ -48,6 +48,7 @@ class Debugger extends Domain {
   final AssetReader _assetReader;
   final Modules _modules;
   final Locations _locations;
+  final String _root;
 
   Debugger._(
     this._remoteDebugger,
@@ -56,12 +57,12 @@ class Debugger extends Domain {
     this._assetReader,
     this._modules,
     this._locations,
-    String root,
+    this._root,
   )   : _breakpoints = _Breakpoints(
             locations: _locations,
             provider: provider,
             remoteDebugger: _remoteDebugger,
-            root: root),
+            root: _root),
         super(provider);
 
   /// The breakpoints we have set so far, indexable by either
@@ -269,18 +270,52 @@ class Debugger extends Domain {
     int column,
   }) async {
     checkIsolate('addBreakpoint', isolateId);
-
     final breakpoint = await _breakpoints.add(scriptId, line);
+    _notifyBreakpoint(breakpoint);
+    return breakpoint;
+  }
 
+  Future<ScriptRef> _updatedScriptRefFor(Breakpoint breakpoint) async {
+    var oldRef = (breakpoint.location as SourceLocation).script;
+    var dartUri = DartUri(oldRef.uri, _root);
+    return await inspector.scriptRefFor(dartUri.serverPath);
+  }
+
+  Future<void> reestablishBreakpoints(
+    Set<Breakpoint> previousBreakpoints,
+    Set<Breakpoint> disabledBreakpoints,
+  ) async {
+    // Previous breakpoints were never removed from Chrome since we use
+    // `setBreakpointByUrl`. We simply need to update the references.
+    for (var breakpoint in previousBreakpoints) {
+      var scriptRef = await _updatedScriptRefFor(breakpoint);
+      var updatedLocation = await _locations.locationForDart(
+          DartUri(scriptRef.uri, _root), _lineNumberFor(breakpoint));
+      var updatedBreakpoint = _breakpoints._dartBreakpoint(
+          scriptRef, updatedLocation, breakpoint.id);
+      _breakpoints._note(
+          bp: updatedBreakpoint,
+          jsId: _breakpoints._jsIdByDartId[updatedBreakpoint.id]);
+      _notifyBreakpoint(updatedBreakpoint);
+    }
+    // Disabled breakpoints were actually removed from Chrome so simply add
+    // them back.
+    for (var breakpoint in disabledBreakpoints) {
+      await addBreakpoint(
+          inspector.isolate.id,
+          (await _updatedScriptRefFor(breakpoint)).id,
+          _lineNumberFor(breakpoint));
+    }
+  }
+
+  void _notifyBreakpoint(Breakpoint breakpoint) {
     final event = Event(
       kind: EventKind.kBreakpointAdded,
       timestamp: DateTime.now().millisecondsSinceEpoch,
       isolate: inspector.isolateRef,
     );
     event.breakpoint = breakpoint;
     _streamNotify('Debug', event);
-
-    return breakpoint;
   }
 
   /// Remove a Dart breakpoint.
@@ -624,7 +659,7 @@ class Debugger extends Domain {
 }
 
 /// Returns the Dart line number for the provided breakpoint.
-int lineNumberFor(Breakpoint breakpoint) =>
+int _lineNumberFor(Breakpoint breakpoint) =>
     int.parse(breakpoint.id.split('#').last);
 
 /// Returns the breakpoint ID for the provided Dart script ID and Dart line
@@ -699,12 +734,13 @@ class _Breakpoints extends Domain {
   Future<String> _setJsBreakpoint(Location location) async {
     // Location is 0 based according to:
     // https://chromedevtools.github.io/devtools-protocol/tot/Debugger#type-Location
-    var response =
-        await remoteDebugger.sendCommand('Debugger.setBreakpoint', params: {
-      'location': {
-        'scriptId': location.jsLocation.scriptId,
-        'lineNumber': location.jsLocation.line - 1,
-      }
+
+    // The module can be loaded from a nested path and contain an ETAG suffix.
+    var urlRegex = '.*${location.modulePath}.*';
+    var response = await remoteDebugger
+        .sendCommand('Debugger.setBreakpointByUrl', params: {
+      'urlRegex': urlRegex,
+      'lineNumber': location.jsLocation.line - 1,
     });
     return response.result['breakpointId'] as String;
   }
@@ -729,8 +765,6 @@ class _Breakpoints extends Domain {
     return _bpByDartId.remove(dartId);
   }
 
-  String dartId(String jsId) => _dartIdByJsId[jsId];
-
   String jsId(String dartId) => _jsIdByDartId[dartId];
 }
 

dwds/lib/src/debugging/location.dart

@@ -21,16 +21,20 @@ class Location {
 
   final DartLocation dartLocation;
 
+  final String modulePath;
+
   /// An arbitrary integer value used to represent this location.
   final int tokenPos;
 
   Location._(
     this.jsLocation,
     this.dartLocation,
+    this.modulePath,
   ) : tokenPos = _startTokenId++;
 
   static Location from(
     String scriptId,
+    String modulePath,
     TargetLineEntry lineEntry,
     TargetEntry entry,
     DartUri dartUri,
@@ -41,8 +45,11 @@ class Location {
     var jsColumn = entry.column;
     // lineEntry data is 0 based according to:
     // https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k
-    return Location._(JsLocation.fromZeroBased(scriptId, jsLine, jsColumn),
-        DartLocation.fromZeroBased(dartUri, dartLine, dartColumn));
+    return Location._(
+      JsLocation.fromZeroBased(scriptId, jsLine, jsColumn),
+      DartLocation.fromZeroBased(dartUri, dartLine, dartColumn),
+      modulePath,
+    );
   }
 
   @override
@@ -259,6 +266,7 @@ class Locations {
           var dartUri = DartUri(path, _root);
           result.add(Location.from(
             scriptId,
+            modulePath,
             lineEntry,
             entry,
             dartUri,

dwds/lib/src/handlers/dev_handler.dart

@@ -63,7 +63,6 @@ class DevHandler {
   final StreamController<DebugConnection> extensionDebugConnections =
       StreamController<DebugConnection>();
   final UrlEncoder _urlEncoder;
-  final bool _restoreBreakpoints;
   final bool _useSseForDebugProxy;
   final bool _serveDevTools;
   final ExpressionCompiler _expressionCompiler;
@@ -87,7 +86,6 @@ class DevHandler {
       this._logWriter,
       this._extensionBackend,
       this._urlEncoder,
-      this._restoreBreakpoints,
       this._useSseForDebugProxy,
       this._serveDevTools,
       this._expressionCompiler,
@@ -197,7 +195,6 @@ class DevHandler {
       metadataProvider,
       appConnection,
       _logWriter,
-      _restoreBreakpoints,
       onResponse: (response) {
         if (_verbose) {
           if (response['error'] == null) return;
@@ -473,7 +470,6 @@ class DevHandler {
             metadataProvider,
             connection,
             _logWriter,
-            _restoreBreakpoints,
             onResponse: _verbose
                 ? (response) {
                     if (response['error'] == null) return;

dwds/lib/src/services/chrome_proxy_service.dart

@@ -80,8 +80,6 @@ class ChromeProxyService implements VmServiceInterface {
 
   StreamSubscription<ConsoleAPIEvent> _consoleSubscription;
 
-  /// If breakpoints should be restored upon a reload / hot restart.
-  final bool _restoreBreakpoints;
   final _disabledBreakpoints = <Breakpoint>{};
   final _previousBreakpoints = <Breakpoint>{};
 
@@ -96,7 +94,6 @@ class ChromeProxyService implements VmServiceInterface {
     this._metadataProvider,
     this._modules,
     this._locations,
-    this._restoreBreakpoints,
     this.executionContext,
     this._logWriter,
   ) {
@@ -119,7 +116,6 @@ class ChromeProxyService implements VmServiceInterface {
       MetadataProvider metadataProvider,
       AppConnection appConnection,
       LogWriter logWriter,
-      bool restoreBreakpoints,
       ExecutionContext executionContext,
       ExpressionCompiler expressionCompiler) async {
     final vm = VM(
@@ -137,17 +133,8 @@ class ChromeProxyService implements VmServiceInterface {
 
     var modules = Modules(metadataProvider, tabUrl);
     var locations = Locations(assetReader, modules, tabUrl);
-    var service = ChromeProxyService._(
-        vm,
-        tabUrl,
-        assetReader,
-        remoteDebugger,
-        metadataProvider,
-        modules,
-        locations,
-        restoreBreakpoints,
-        executionContext,
-        logWriter);
+    var service = ChromeProxyService._(vm, tabUrl, assetReader, remoteDebugger,
+        metadataProvider, modules, locations, executionContext, logWriter);
     unawaited(service.createIsolate(appConnection));
     await service.createEvaluator(expressionCompiler);
     return service;
@@ -192,14 +179,9 @@ class ChromeProxyService implements VmServiceInterface {
       executionContext,
     );
 
-    for (var breakpoint in _previousBreakpoints) {
-      var lineNumber = lineNumberFor(breakpoint);
-      var oldRef = (breakpoint.location as SourceLocation).script;
-      var dartUri = DartUri(oldRef.uri, uri);
-      var newRef = await _inspector.scriptRefFor(dartUri.serverPath);
-      await (await _debugger)
-          .addBreakpoint(_inspector.isolate.id, newRef.id, lineNumber);
-    }
+    await (await _debugger)
+        .reestablishBreakpoints(_previousBreakpoints, _disabledBreakpoints);
+    _disabledBreakpoints.clear();
 
     unawaited(appConnection.onStart.then((_) async {
       await (await _debugger).resumeFromStart();
@@ -257,12 +239,8 @@ class ChromeProxyService implements VmServiceInterface {
             isolate: _inspector.isolateRef));
     _vm.isolates.removeWhere((ref) => ref.id == isolate.id);
     _inspector = null;
-    if (_restoreBreakpoints) {
-      _previousBreakpoints.clear();
-      _previousBreakpoints
-        ..addAll(isolate.breakpoints)
-        ..addAll(_disabledBreakpoints);
-    }
+    _previousBreakpoints.clear();
+    _previousBreakpoints.addAll(isolate.breakpoints);
     _consoleSubscription.cancel();
     _consoleSubscription = null;
   }

dwds/lib/src/services/debug_service.dart

@@ -134,7 +134,6 @@ class DebugService {
       MetadataProvider metadataProvider,
       AppConnection appConnection,
       LogWriter logWriter,
-      bool restoreBreakpoints,
       {void Function(Map<String, dynamic>) onRequest,
       void Function(Map<String, dynamic>) onResponse,
       bool useSse,
@@ -148,7 +147,6 @@ class DebugService {
         metadataProvider,
         appConnection,
         logWriter,
-        restoreBreakpoints,
         executionContext,
         expressionCompiler);
     var authToken = _makeAuthToken();

dwds/test/debugger_test.dart

@@ -58,6 +58,7 @@ void main() async {
     var lineEntry = TargetLineEntry(92, [entry]);
     var location = Location.from(
       'foo.dart',
+      'foo.ddc.js',
       lineEntry,
       entry,
       DartUri('package:foo/foo.dart'),

dwds/test/fixtures/server.dart

@@ -106,8 +106,6 @@ class TestServer {
       hostname: hostname,
       verbose: true,
       urlEncoder: urlEncoder,
-      // ignore: deprecated_member_use_from_same_package
-      restoreBreakpoints: restoreBreakpoints,
       expressionCompiler: expressionCompiler,
     );
 

dwds/test/reload_test.dart

@@ -182,7 +182,8 @@ void main() {
       isolateId = vm.isolates.first.id;
       var isolate = await client.getIsolate(isolateId);
       expect(isolate.pauseEvent.kind, EventKind.kResume);
-      expect(isolate.breakpoints.isEmpty, isTrue);
+      // Previous breakpoint should still exist.
+      expect(isolate.breakpoints.isNotEmpty, isTrue);
     });
   });