add a debugger UI #80

devoncarew · 2019-01-02T19:36:16Z

This adds a debugging UI to devtools. It:

displays the call stack, variables, breakpoints, and the app's scripts
allows the user to set and clear breakpoints
supports resuming execution when paused and stepping in/out/over
supports async stepping and displaying async frames
uses codemirror to display the app source
displays console output in-line in the page
calls toString() on objects in the variables view on mouse hover
has tests for much of the above

This PR is ready for review. We will want to (perhaps out of band of the PR), decide whether we do want a debugger UI in devtools, whether we should have it enabled now or only accessible through a compile time flag or similar. Happy to take feedback about how this fits into the product here or offline.

@jacob314 @kenzieschmoll

jacob314 · 2019-01-02T20:50:02Z

lib/debugger/debugger.css

+/* found in the LICENSE file. */
+
+.debugger-menu {
+    width: 280px;


nit: does this need to be hard coded to 280px?

Well, currently. It's the width of the left sections in the debugger UI. Our options are hard-coding a width, letting it float based on a percentage of the screen width, or building a user-positionable splitter control.

Of the 3 I prefer a hard-coded width. It ensures that the contents are still readable in more narrow windows, and doesn't get too wide for wider windows.

When designing this ui, I think we can assume a browser width of ~1300px (https://www.websitedimensions.com/).

Long term we'll likely want some user positionable controls, but likely not an immediate concern.

jacob314 · 2019-01-02T20:50:59Z

lib/debugger/debugger.dart

+    final PButton resumeButton = new PButton(null)
+      ..primary()
+      ..small()
+      ..element.style.minWidth = '90px'


is there a reason this style shouldn't be in the css?

No, I'll move this css.

jacob314 · 2019-01-02T20:56:55Z

lib/debugger/debugger.dart

+    stepIn.click(() => debuggerState.stepIn());
+    stepOut.click(() => debuggerState.stepOut());
+
+    final Map<String, dynamic> options = <String, dynamic>{


nit: consider omitting they types on the RHS unless they are adding value. we aren't forcing the flutter types everywhere style for this code anymore

👍 will do; I started this PR before the style change

FWIW I think it's nicer to omit types on the left. If the generic arguments are needed (they could be skipped altogether here) put them on the literal only.

It would be nice if we had a dart quick fix that stripped out all types that are inferred.

We'll want to hew closely to the flutter/flutter style in this repo.

jacob314 · 2019-01-02T20:57:37Z

lib/debugger/debugger.dart

+    final CodeMirror codeMirror =
+        new CodeMirror.fromElement(sourceArea.element, options: options);
+    codeMirror.setReadOnly(true);
+    // ignore: always_specify_types


this ignore: is not needed anymore as we switched the lint.

jacob314 · 2019-01-02T20:58:36Z

lib/debugger/debugger.dart

+          final Frame frame = frames.first;
+
+          final Frame newFrame = new Frame();
+          newFrame.type = frame.type;


nit: cascades are nice for this case. also consider adding clone constructor to Frame.

👍 re: cascades

The ctor is defined in a package: we're importing, but I'll open an issue to improve the options for constructors for these classes.

jacob314 · 2019-01-02T20:59:56Z

lib/debugger/debugger.dart

+    deviceStatus.element.text = '';
+
+    scriptsView.clearScripts();
+    _scriptCountDiv.text = '0';


I see setting the state of _scriptCountDiv a lot of places. Consider adding a helper that syncs its value to the current script count.

changed to listening to script changes, and having the listener update the count

jacob314 · 2019-01-02T21:02:34Z

lib/debugger/debugger.dart

+        scriptPrefix =
+            scriptPrefix.substring(0, scriptPrefix.lastIndexOf('/') + 1);
+      }
+    } else if (scriptPrefix.contains('/example/')) {


is this hard coding of /example/ needed? Seems a bit surprising. Generally it would be helpful to move this logic for determining the root directory to a shared util as I also need it for the inspector to clarify what directories are considered the local project for highlighting only user created widgets.

This is tough as we don't have a fool-proof way to determine what the root of a project is w/ just the info from the service protocol.

For flutter apps, they generally (always?) run as package:my_app urls (the first case above), so I think we'll be able to determine app code vs other code fairly reliably.

For cli apps, they generally run as file: urls, and have a bit more ambiguity about what is app code. In the logic above, I'm trying to walkup the url a bit. and use package layout conventions to identify the app directory.

I'll trim the logic for example/ above as its less common than lib/ or bin/.

jacob314 · 2019-01-02T21:03:53Z

lib/debugger/debugger.dart

+  }
+}
+
+class Pos {


nit: Pos --> Position?

That conflicts with an import, but I can find a better name than Pos.

SourcePosition

jacob314 · 2019-01-02T21:04:45Z

lib/debugger/debugger.dart

+    _items = new SelectableList<Breakpoint>()
+      ..flex()
+      ..clazz('menu-item-bottom-border')
+      ..element.style.overflowY = 'scroll';


nit: specify overflowY with CSS file?

jacob314 · 2019-01-02T21:05:06Z

lib/debugger/debugger.dart

+    _items.setRenderer((Breakpoint breakpoint) {
+      final dynamic location = breakpoint.location;
+
+      final CoreElement element = li(text: '', c: 'list-item');


can text parameter be omitted as it is empty?

jacob314 · 2019-01-02T21:06:06Z

lib/debugger/debugger.dart

+
+  Stream<Breakpoint> get onDoubleClick => _items.onDoubleClick;
+
+  final CoreElement _breakpointsCountDiv;


consider moving all fields up closer to the top of the class.

jacob314 · 2019-01-02T21:06:52Z

lib/debugger/debugger.dart

+      String uri1 = ref1.uri;
+      String uri2 = ref2.uri;
+
+      if (uri1.startsWith('dart:_')) {


add a helper method to cleanup dart:_ uris

jacob314 · 2019-01-02T21:07:47Z

lib/debugger/debugger.dart

+    _items = new SelectableList<Frame>()
+      ..flex()
+      ..clazz('menu-item-bottom-border')
+      ..element.style.overflowY = 'scroll';


looks like all the menus allow scroll. maybe add that to the css file?

jacob314 · 2019-01-02T21:09:33Z

lib/debugger/debugger.dart

+}
+
+class ScriptAndPos {
+  ScriptAndPos(this.uri, {this.position});


nit: add @required to position argument assuming it is required.

jacob314 · 2019-01-02T21:10:15Z

lib/debugger/debugger.dart

+  ScriptRef getRef(dynamic location) {
+    if (location is SourceLocation) {
+      return location.script;
+    } else if (location is UnresolvedSourceLocation) {


nit :
no need for either else as each if clause ends in a return.

jacob314 · 2019-01-02T21:11:03Z

lib/debugger/debugger.dart

+
+  int getPos(dynamic location) {
+    if (location is SourceLocation) {
+      return location.tokenPos ?? 0;


nit: make location.tokenPos be made non-nullable instead of having a check here?

We get this info directly from the service protocol - in some cases tokenPos can be null.

jacob314 · 2019-01-02T21:11:43Z

lib/debugger/debugger.dart

+  ConsoleArea() {
+    final Map<String, dynamic> options = <String, dynamic>{
+      'mode': 'text/plain',
+      //'lineNumbers': true,


remove commented line or add it as an option to ConsoleArea.

jacob314 · 2019-01-02T21:13:08Z

lib/debugger/debugger.dart

+    _editor.scrollIntoView(lastLineIndex, lastLine.length);
+  }
+
+  // for testing


Nit: use https://pub.dartlang.org/documentation/meta/latest/meta/visibleForTesting-constant.html

jacob314

This looks great! Lgtm with comments.

kenzieschmoll · 2019-01-02T21:19:54Z

example/test.dart

+    description = 'items: $count';
+
+    final List<int> numbers = new List.generate(10, (index) => index * index);
+    // ignore: unused_local_variable


nit: unnecessary ignore

kenzieschmoll · 2019-01-02T21:30:36Z

lib/debugger/debugger.dart

+
+      if (value is Sentinel) {
+        return value.valueAsString;
+      } else if (value is TypeArgumentsRef) {


nit: else's not needed since every if statement returns.

kenzieschmoll · 2019-01-02T21:33:10Z

lib/debugger/debugger.dart

+    if (result is Instance) {
+      final Instance obj = result;
+      return obj.valueAsString;
+    } else {


nit: else not needed since the if statement returns.

I like the parallelism here since there are just two branches.

kenzieschmoll · 2019-01-02T21:34:51Z

lib/debugger/debugger.dart

+  Future<Success> stepOut() =>
+      service.resume(isolateRef.id, step: StepOption.kOut);
+
+  // Used for testing.


nit: use visibleForTesting

kenzieschmoll · 2019-01-02T21:35:08Z

lib/debugger/debugger.dart

+    return service.addBreakpoint(isolateRef.id, scriptId, line);
+  }
+
+  // Used for testing.


nit: visibleForTesting

devoncarew · 2019-01-02T22:38:27Z

Thanks for the comments! I'm going to get the tests working, work through the comments, and then ping this PR.

devoncarew · 2019-01-02T22:48:39Z

The tests look like they have race conditions when run on travis unfortunately.

Uncaught, null, 18, 0, object null
  package:webkit_inspection_protocol/src/runtime.dart 23:7  WipRuntime.evaluate
  ===== asynchronous gap ===========================
  dart:async                                                _AsyncAwaitCompleter.completeError
  package:webkit_inspection_protocol/src/runtime.dart       WipRuntime.evaluate
  ===== asynchronous gap ===========================
  dart:async                                                _asyncThenWrapperHelper
  package:webkit_inspection_protocol/src/runtime.dart       WipRuntime.evaluate
  test/integration_test.dart 569:38                         BrowserTabInstance._getAppChannelObject
  test/integration_test.dart 558:22                         BrowserTabInstance.getBrowserChannel
  ===== asynchronous gap ===========================
  dart:async                                                _asyncThenWrapperHelper
  test/integration_test.dart                                BrowserTabInstance.getBrowserChannel
  test/integration_test.dart 377:23                         DevtoolsManager.start
  ===== asynchronous gap ===========================
  dart:async                                                _asyncThenWrapperHelper
  test/integration_test.dart                                DevtoolsManager.start
  test/integration_test.dart 117:17                         debuggingTests.<fn>
  ===== asynchronous gap ===========================
  dart:async                                                _asyncThenWrapperHelper
  test/integration_test.dart                                debuggingTests.<fn>

Perhaps just adding a wait or a condition at the right place?

natebosch · 2019-01-02T23:18:28Z

lib/debugger/debugger.dart

+    stepIn.click(() => debuggerState.stepIn());
+    stepOut.click(() => debuggerState.stepOut());
+
+    final Map<String, dynamic> options = <String, dynamic>{


FWIW I think it's nicer to omit types on the left. If the generic arguments are needed (they could be skipped altogether here) put them on the literal only.

natebosch · 2019-01-02T23:23:32Z

lib/debugger/debugger.dart

+}
+
+class DebuggerState {
+  DebuggerState();


this would likely make a good lint...

natebosch · 2019-01-02T23:25:45Z

lib/debugger/debugger.dart

+
+  List<Breakpoint> get breakpoints => _breakpoints.value;
+
+  void setVmService(VmService service) {


how would the user of this API know to use this instead of the setter directly?

How bout making the field private with a public set and get to attach the extra behavior here.

This is an internal class, so not intended to be driven from outside the library.

I like the explicit setFoo here, as this does do a bit of work. +1 to making the field private; this method is the only way that people outside of the class should manipulate the value.

natebosch · 2019-01-02T23:27:58Z

lib/debugger/debugger.dart

+    commonScriptPrefix = scriptPrefix;
+  }
+
+  String getShortScriptName(String uri) {


[nit] How about shortScriptName

https://www.dartlang.org/guides/language/effective-dart/design#avoid-starting-a-method-name-with-get

Not sure that applies, as this method takes a param?

From the guide: "Even if the member does need to be a method because it takes arguments or otherwise isn’t a good fit for a getter, you should still avoid get."

natebosch · 2019-01-02T23:30:31Z

test/example/test.dart

+
+void main() {
+  const Duration delay = Duration(seconds: 4);
+  Function work;


this is only assigned once, should it be written as a function instead of assigning a lambda?

https://www.dartlang.org/guides/language/effective-dart/usage#do-use-a-function-declaration-to-bind-a-function-to-a-name

I use this script for manual testing of the debugger, so it's got some oddities. I can look at using a class instead of top-level functions in order to get the same behavior I need.

The function does not need to be top level, it can be defined within main. From what I can see you can use exactly what you have if you:

Remove variable declaration

Remove the = at the definition point

Remove the semicolon following the last }

Oh, when you were testing did you specifically need a variable that held a closure? If that's the case maybe add a comment?

jacob314 · 2019-01-03T01:53:11Z

test/example/test.dart

@@ -0,0 +1,83 @@
+// Copyright 2018 The Chromium Authors. All rights reserved.


nit: for all new files update the year to 2019.

devoncarew

I made a pass through the comments, and w/ the updated build from a separate PR, believe I have the tests running on the bots.

devoncarew · 2019-01-03T03:11:56Z

lib/debugger/debugger.css

+/* found in the LICENSE file. */
+
+.debugger-menu {
+    width: 280px;


Well, currently. It's the width of the left sections in the debugger UI. Our options are hard-coding a width, letting it float based on a percentage of the screen width, or building a user-positionable splitter control.

Of the 3 I prefer a hard-coded width. It ensures that the contents are still readable in more narrow windows, and doesn't get too wide for wider windows.

When designing this ui, I think we can assume a browser width of ~1300px (https://www.websitedimensions.com/).

Long term we'll likely want some user positionable controls, but likely not an immediate concern.

devoncarew · 2019-01-03T03:15:26Z

lib/debugger/debugger.dart

+    final PButton resumeButton = new PButton(null)
+      ..primary()
+      ..small()
+      ..element.style.minWidth = '90px'


No, I'll move this css.

devoncarew · 2019-01-03T03:16:02Z

lib/debugger/debugger.dart

+    stepIn.click(() => debuggerState.stepIn());
+    stepOut.click(() => debuggerState.stepOut());
+
+    final Map<String, dynamic> options = <String, dynamic>{


We'll want to hew closely to the flutter/flutter style in this repo.

devoncarew · 2019-01-03T03:17:17Z

lib/debugger/debugger.dart

+          final Frame frame = frames.first;
+
+          final Frame newFrame = new Frame();
+          newFrame.type = frame.type;


👍 re: cascades

The ctor is defined in a package: we're importing, but I'll open an issue to improve the options for constructors for these classes.

devoncarew · 2019-01-03T03:37:47Z

lib/debugger/debugger.dart

+
+      if (value is Sentinel) {
+        return value.valueAsString;
+      } else if (value is TypeArgumentsRef) {


devoncarew · 2019-01-03T17:10:52Z

lib/debugger/debugger.dart

+
+  int getPos(dynamic location) {
+    if (location is SourceLocation) {
+      return location.tokenPos ?? 0;


We get this info directly from the service protocol - in some cases tokenPos can be null.

devoncarew · 2019-01-03T17:10:57Z

lib/debugger/debugger.dart

+  ConsoleArea() {
+    final Map<String, dynamic> options = <String, dynamic>{
+      'mode': 'text/plain',
+      //'lineNumbers': true,


devoncarew · 2019-01-03T17:11:15Z

lib/debugger/debugger.dart

+    _editor.scrollIntoView(lastLineIndex, lastLine.length);
+  }
+
+  // for testing


devoncarew · 2019-01-03T17:11:38Z

test/example/test.dart

@@ -0,0 +1,83 @@
+// Copyright 2018 The Chromium Authors. All rights reserved.


devoncarew · 2019-01-03T17:19:16Z

test/example/test.dart

+
+void main() {
+  const Duration delay = Duration(seconds: 4);
+  Function work;


I use this script for manual testing of the debugger, so it's got some oddities. I can look at using a class instead of top-level functions in order to get the same behavior I need.

DanTup · 2019-02-19T12:01:35Z

pubspec.yaml

@@ -4,13 +4,15 @@ version: 0.0.1
 homepage: https://github.com/flutter/devtools

 environment:
-  sdk: '>=2.0.0-dev <3.0.0'
+  sdk: '>=2.1.0 <3.0.0'


FWIW - this constraint prevents the SDK included in Flutter v1.0 from being able to activate this package (it shipped with 2.1.0-dev.9.4.flutter-f9ebf21297 and a -dev suffix makes that "less than" 2.1.0).

I suspect we're not concerned but since I noticed it when adding DC integration tests and it's a little counter-intuitive (we consider Flutter v1.0 shipped with Dart 2.1, but semver doesn't see it that way!) I thought it worth noting.

Since we're effectively just using this constraint in the pub global activated server app, I think we can change the lower bound to Flutter's.

devoncarew added 20 commits October 16, 2018 14:47

prototype debugger

2c96b95

merge to master

93c1650

update styles

8994f94

update to the latest vm_service_lib

45e9088

merge to master

3c4356c

Merge branch 'master' into debugger

fb0c86f

use the new getScripts() API

2e92538

debugger updates

ead827b

Merge branch 'master' into debugger

6e44fed

Merge branch 'master' into debugger

4e09836

merge to master

8a7a97c

implement async stepping; separate css

1dc1ec2

handle breaking on exceoptions

02fd363

remove a todo

840ae8b

initial debugger tests

16acce7

Merge branch 'master' into debugger

ab548cb

implement tests for debugging functionality

56e4b82

add a console; display object toString() result

e46514d

Merge branch 'master' into debugger

c921015

typo

a465acd

jacob314 reviewed Jan 2, 2019

View reviewed changes

allow non-flutter selected isolates

9c834b0

jacob314 reviewed Jan 2, 2019

View reviewed changes

kenzieschmoll mentioned this pull request Jan 2, 2019

De-dupe isolate logic in service_manager #75

Merged

jacob314 approved these changes Jan 2, 2019

View reviewed changes

kenzieschmoll reviewed Jan 2, 2019

View reviewed changes

devoncarew added 2 commits January 2, 2019 14:23

move the debugger example

f70d645

revert back to an older sdk

dbd4c44

kenzieschmoll approved these changes Jan 2, 2019

View reviewed changes

Merge branch 'master' into debugger

dc61e05

natebosch reviewed Jan 2, 2019

View reviewed changes

jacob314 reviewed Jan 3, 2019

View reviewed changes

review comments

6332647

devoncarew commented Jan 3, 2019

View reviewed changes

devoncarew merged commit be2b798 into flutter:master Jan 3, 2019

DanTup reviewed Feb 19, 2019

View reviewed changes

DanTup mentioned this pull request Feb 19, 2019

Relax SDK constraint to include Flutter stable (v1.0) #307

Merged

kenzieschmoll mentioned this pull request Dec 16, 2019

NaN toInt error in memory chart #1487

Closed


		Stream<Breakpoint> get onDoubleClick => _items.onDoubleClick;

		final CoreElement _breakpointsCountDiv;


		List<Breakpoint> get breakpoints => _breakpoints.value;

		void setVmService(VmService service) {

		@@ -0,0 +1,83 @@
		// Copyright 2018 The Chromium Authors. All rights reserved.

add a debugger UI #80

add a debugger UI #80

Uh oh!

Conversation

devoncarew commented Jan 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devoncarew Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devoncarew Jan 3, 2019 •

edited

Loading