-
Notifications
You must be signed in to change notification settings - Fork 87
Add support for variables in stack frames #479
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.
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
Conversation
dwds/lib/src/domain.dart
Outdated
|
|
||
| /// A constructor for the AppInspector to call which doesn't set | ||
| /// [_appInspectorProvider] as it's not used by the AppInspector. | ||
| Domain.forInspector(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow this. Why is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Domain takes an AppInspectorProvider. The AppInspectorProvider has to return the instance. When we're in the constructor for AppInspector we can't reference this, so we can't call super with an AppInspectorProvider. The only workaround I found was to give it a constructor that didn't need that.
| case 'object': | ||
| // TODO: Actual toString() | ||
| var toString = 'Placeholder for toString() result'; | ||
| var truncated = toString.substring(0, math.min(100, toString.length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious where this 100 value is coming from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's completely arbitrary. Added a TODO to make that consistent with what the VM does.
| /// A library for WebKit mirror objects and support code. These probably should | ||
| /// get migrated into webkit_inspection_protocol over time. | ||
| import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not depend on this package for this abstraction? This will become problematic as we make use of the Chrome Debug Extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs that now because it can return RemoteObject from that package for its values. My thought was more to fold this into that package than to re-implement what it does.
Why is it problematic if we use the extension? Is the extension going to use an incompatible protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. We'll likely reuse the abstractions provided by webkit_inspection_protocol but change out the actual communication layer.
Sorry for the size, this has several different things in it