-
Notifications
You must be signed in to change notification settings - Fork 83
Batch Debugger.ScriptParsed events sent from the debug extension #1628
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Batch Debugger.ScriptParsed events sent from the debug extension #1628
Conversation
@@ -61,7 +62,10 @@ Tab _mostRecentDartTab; | |||
DebuggerTrigger _debuggerTrigger; | |||
|
|||
class DebugSession { | |||
final SocketClient socketClient; | |||
// How often to send batched events. |
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.
Nit: private fields should be listed after public ones
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.
Done, thanks!
dwds/test/devtools_test.dart
Outdated
// TODO(grouma): switch back to `fixture.webdriver.title` when | ||
// https://github.com/flutter/devtools/issues/2045 is fixed. | ||
expect(await context.webDriver.pageSource, contains('Flutter')); | ||
expect(await context.webDriver.title, contains('Dart DevTools')); |
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.
This changes seem unrelated to this PR / the tests are still disabled. I think these changes should be made in a separate PR as part of re-enabling the tests.
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.
Agreed, will revert. I am concerned about those tests being disabled for too long though - we might break launching devtools inadvertently... We can use the same waiting for the devtools to launch technique as used in the debug_extension_tests to make the tests not flaky.
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.
Done!
@@ -3,7 +3,9 @@ | |||
- Update `package:vm_service` to 8.3.0. | |||
- Convert JavaScript stack traces in uncaught exceptions to Dart stack traces. | |||
- Fix failure to set breakpoints on windows with a base change in index.html. | |||
|
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.
One more nit: Looks like the DWDS change is actually just moving batched_stream.dart
into shared utilities. Let's update CHANGELOG to reflect that
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.
done!
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.
One requested change then LGTM
Update change log to reflect moving `batched_stream.dart` into shared utilities.
Batch Debugger.ScriptParsed events sent from the debug extension to extension backend.
Debugger.ScriptParsed
and send the batches every second to the server.Debugger.ScriptParsed
eventsNote: We introduced sending
Debugger.ScriptParsed
events to fix the issue #1557, this PR is a follow up making the solution faster.Closes: #1599
Related: #1621
Related: #1613