Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructuring to start supporting breakpoints and sourcemaps #377

Merged
merged 13 commits into from
May 8, 2019
Merged

Restructuring to start supporting breakpoints and sourcemaps #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8f3845b
Breakpoints work
alan-knight Apr 4, 2019
dc5f6ed
one more comment
alan-knight Apr 4, 2019
fff7eef
revert analysis options
alan-knight Apr 4, 2019
4116201
WIP - lots of things related to breakpoints and sourcemaps
alan-knight Apr 12, 2019
7f6a70d
mid-edit
alan-knight Apr 15, 2019
3b7b9a7
Emergency state save
alan-knight Apr 18, 2019
e24ff48
Debugging tests
alan-knight Apr 26, 2019
48b046d
still wiping
alan-knight May 7, 2019
9c397ba
Remove PR 376 code
alan-knight May 7, 2019
203027a
Restructuring to start supporting breakpoints and sourcemaps
alan-knight May 7, 2019
e072bdb
Format test file
alan-knight May 7, 2019
9c99ab5
Don't reload - causes other failures
alan-knight May 7, 2019
d973788
Delete reload to satisfy linter
alan-knight May 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 32 additions & 17 deletions dwds/lib/src/chrome_proxy_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import 'package:pub_semver/pub_semver.dart' as semver;
import 'package:vm_service_lib/vm_service_lib.dart';
import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';

import 'debugger.dart';
import 'helpers.dart';

/// A proxy from the chrome debug protocol to the dart vm service protocol.
Expand Down Expand Up @@ -38,6 +39,9 @@ class ChromeProxyService implements VmServiceInterface {
/// This may be null during a hot restart or page refresh.
Isolate _isolate;

/// Provides debugger-related functionality.
Debugger debugger;

/// Fields that are specific to the current [_isolate].
///
/// These need to get cleared whenever [destroyIsolate] is called.
Expand Down Expand Up @@ -71,7 +75,6 @@ class ChromeProxyService implements VmServiceInterface {
'$appInstanceId');
}
var tabConnection = await appTab.connect();
await tabConnection.debugger.enable();
await tabConnection.runtime.enable();

// TODO: What about `architectureBits`, `targetCPU`, `hostCPU` and `pid`?
Expand All @@ -81,10 +84,16 @@ class ChromeProxyService implements VmServiceInterface {
..startTime = DateTime.now().millisecondsSinceEpoch
..version = Platform.version;
var service = ChromeProxyService._(vm, appTab, tabConnection, assetHandler);
await service._initialize();
await service.createIsolate();
return service;
}

Future<Null> _initialize() async {
debugger = Debugger(this);
await debugger.initialize();
}

/// Creates a new [_isolate].
///
/// Only one isolate at a time is supported, but they should be cleaned up
Expand Down Expand Up @@ -133,12 +142,12 @@ class ChromeProxyService implements VmServiceInterface {
_vm.isolates.add(isolateRef);
_isolate = isolate;

_streamNotify(
streamNotify(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this was changed to public. Likewise with scriptRefs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because Debugger needs to call them and it's in a separate library. We could instead make all the sub-components part files, but I'm not sure that's generally considered a good technique.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could move them to an internal library that could be used from both places, but that might require a bit more re-arrangement, e.g. moving _streamControllers to the other library as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Debugger isn't using them just yet but will in the near future. Keeping them public is probably preferable to a bunch of part files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw this is also why we only expose the VmServiceInterface now publicly instead of this class, so these don't leak outside the package (not without a src import)

'Isolate',
Event()
..kind = EventKind.kIsolateStart
..isolate = isolateRef);
_streamNotify(
streamNotify(
'Isolate',
Event()
..kind = EventKind.kIsolateRunnable
Expand All @@ -148,7 +157,7 @@ class ChromeProxyService implements VmServiceInterface {
// isolate, but devtools doesn't recognize extensions after a page refresh
// otherwise.
for (var extensionRpc in isolate.extensionRPCs) {
_streamNotify(
streamNotify(
'Isolate',
Event()
..kind = EventKind.kServiceExtensionAdded
Expand All @@ -162,7 +171,7 @@ class ChromeProxyService implements VmServiceInterface {
/// Clears out [_isolate] and all related cached information.
void destroyIsolate() {
if (_isolate == null) return;
_streamNotify(
streamNotify(
'Isolate',
Event()
..kind = EventKind.kIsolateExit
Expand All @@ -180,8 +189,8 @@ class ChromeProxyService implements VmServiceInterface {

@override
Future<Breakpoint> addBreakpoint(String isolateId, String scriptId, int line,
{int column}) {
throw UnimplementedError();
{int column}) async {
return debugger.addBreakpoint(isolateId, scriptId, line, column: column);
}

@override
Expand Down Expand Up @@ -501,7 +510,7 @@ function($argsString) {

@override
Future<ScriptList> getScripts(String isolateId) async {
var scripts = await _getScripts(isolateId);
var scripts = await scriptRefs(isolateId);
return ScriptList()..scripts = scripts;
}

Expand All @@ -518,7 +527,11 @@ function($argsString) {
..source = script;
}

Future<List<ScriptRef>> _getScripts(String isolateId) async {
/// Internal method to list all scripts in the isolate.
///
/// This is used as the underlying mechanism
/// for [getScripts].
Future<List<ScriptRef>> scriptRefs(String isolateId) async {
var isolate = _getIsolate(isolateId);
var scripts = <ScriptRef>[];
for (var lib in isolate.libraries) {
Expand All @@ -527,6 +540,8 @@ function($argsString) {
if (lib.id.startsWith('dart:') || lib.id.endsWith('.bootstrap')) continue;
scripts.addAll((await _getLibrary(isolateId, lib.id)).scripts);
}
// TODO(alanknight): Is this the same as the values in _scriptRefs after
// constructing all the libraries? Make that clearer.
return scripts;
}

Expand Down Expand Up @@ -613,7 +628,7 @@ function($argsString) {

@override
Future<ReloadReport> reloadSources(String isolateId,
{bool force, bool pause, String rootLibUri, String packagesUri}) {
{bool force, bool pause, String rootLibUri, String packagesUri}) async {
throw UnimplementedError();
}

Expand Down Expand Up @@ -673,7 +688,7 @@ function($argsString) {
@override
Future<Success> setVMName(String name) async {
_vm.name = name;
_streamNotify(
streamNotify(
'VM',
Event()
..kind = EventKind.kVMUpdate
Expand Down Expand Up @@ -761,11 +776,11 @@ function($argsString) {
} else {
event.kind = EventKind.kPauseInterrupted;
}
_streamNotify('Debug', event);
streamNotify('Debug', event);
});
resumeSubscription = tabConnection.debugger.onResumed.listen((e) {
if (_isolate == null) return;
_streamNotify(
streamNotify(
'Debug',
Event()
..kind = EventKind.kResume
Expand Down Expand Up @@ -808,15 +823,15 @@ function($argsString) {
case 'dart.developer.registerExtension':
var service = event.args[1].value as String;
_isolate.extensionRPCs.add(service);
_streamNotify(
streamNotify(
'Isolate',
Event()
..kind = EventKind.kServiceExtensionAdded
..extensionRPC = service
..isolate = isolateRef);
break;
case 'dart.developer.postEvent':
_streamNotify(
streamNotify(
'Extension',
Event()
..kind = EventKind.kExtension
Expand All @@ -836,7 +851,7 @@ function($argsString) {
// up the library for a given instance to create it though.
// https://github.com/dart-lang/sdk/issues/36771.
..classRef = ClassRef();
_streamNotify(
streamNotify(
'Debug',
Event()
..kind = EventKind.kInspect
Expand All @@ -852,7 +867,7 @@ function($argsString) {

/// Adds [event] to the stream with [streamId] if there is anybody listening
/// on that stream.
void _streamNotify(String streamId, Event event) {
void streamNotify(String streamId, Event event) {
var controller = _streamControllers[streamId];
if (controller == null) return;
controller.add(event);
Expand Down
23 changes: 23 additions & 0 deletions dwds/lib/src/debugger.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import 'dart:async';

import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';
import 'package:vm_service_lib/vm_service_lib.dart';

import 'chrome_proxy_service.dart';

class Debugger {
Debugger(this.mainProxy);

ChromeProxyService mainProxy;

WipDebugger get chromeDebugger => mainProxy.tabConnection.debugger;

Future<Null> initialize() async {
await chromeDebugger.enable();
}

Future<Breakpoint> addBreakpoint(String isolateId, String scriptId, int line,
{int column}) async {
throw UnimplementedError();
}
}
45 changes: 32 additions & 13 deletions dwds/test/chrome_proxy_service_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,38 @@ void main() {
await context.tearDown();
});

test('addBreakPoint', () {
expect(() => service.addBreakpoint(null, null, null),
throwsUnimplementedError);
});
group('breakpoints', () {
VM vm;
Isolate isolate;
ScriptList scripts;
ScriptRef mainScript;

test('addBreakpointAtEntry', () {
expect(() => service.addBreakpointAtEntry(null, null),
throwsUnimplementedError);
});
setUp(() async {
vm = await service.getVM();
isolate = await service.getIsolate(vm.isolates.first.id) as Isolate;
scripts = await service.getScripts(isolate.id);
mainScript =
scripts.scripts.firstWhere((each) => each.uri.contains('main.dart'));
});

test('addBreakpointWithScriptUri', () {
expect(() => service.addBreakpointWithScriptUri(null, null, null),
throwsUnimplementedError);
test('addBreakPoint', () async {
// TODO: Reloading causes other tests to fail. Investigate.
// await reload();
expect(() => service.addBreakpoint(isolate.id, mainScript.id, 19),
throwsUnimplementedError);
var breakpoints = isolate.breakpoints;
expect(breakpoints, isEmpty);
});

test('addBreakpointAtEntry', () {
expect(() => service.addBreakpointAtEntry(null, null),
throwsUnimplementedError);
});

test('addBreakpointWithScriptUri', () {
expect(() => service.addBreakpointWithScriptUri(null, null, null),
throwsUnimplementedError);
});
});

group('callServiceExtension', () {
Expand Down Expand Up @@ -358,15 +377,15 @@ void main() {
pauseCompleter.complete();
});
var resumeCompleter = Completer();
var resumseSub = tabConnection.debugger.onResumed.listen((_) {
var resumeSub = tabConnection.debugger.onResumed.listen((_) {
resumeCompleter.complete();
});
expect(await service.pause(isolateId), const TypeMatcher<Success>());
await pauseCompleter.future;
expect(await service.resume(isolateId), const TypeMatcher<Success>());
await resumeCompleter.future;
await pauseSub.cancel();
await resumseSub.cancel();
await resumeSub.cancel();
});

test('registerService', () async {
Expand Down