-
Notifications
You must be signed in to change notification settings - Fork 83
Send debug info from injected client to the debug extension #1772
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
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.
Thanks Elliott! I left a couple of comments.
@@ -86,6 +86,7 @@ class Dwds { | |||
bool launchDevToolsInNewWindow = true, | |||
SdkConfigurationProvider? sdkConfigurationProvider, | |||
bool emitDebugEvents = true, | |||
bool isInternalBuild = false, |
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.
Is something passing this value, or is is it going to be added in future CLs?
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.
Not yet, this will get passed by flutter_tools, ddr etc when they start dwds
messageHandler: (DebugInfo debugInfo) async { | ||
final currentTab = await _getTab(); | ||
final currentUrl = currentTab?.url ?? ''; | ||
final appUrl = debugInfo.appUrl ?? ''; |
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.
Is it possible to add tests to make sure all the data in the debug info is received here?
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 can't think of a good way to test this until we are actually using the debug info to trigger a debug session. We don't have access to the extension icon to see if it has been changed (that is why we have this onFakeClick
method in the MV2 extension, and we can't intercept runtime messages in tests because they are for extension communication only.
Once more of the functionality is added (and we are triggering a debug session) the existence of these values can be tested.
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.
Oh but I can add tests for the injected client until then, will update those!
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 was able to add a test for the internal build property on the window object. Not sure how to test the CustomEvent
unfortunately. Ideally webdriver would have a way of registering an event listener on the page document, so that we could load up the dart app and listen for the dart-app-ready
event and confirm that the properties are what we expect, but unfortunately webdriver doesn't support event listeners like 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.
Thanks Elliott! We chatter offline about adding test in subsequent PRs.
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.
LGTM!
In the current MV2 Dart Debug Extension, we send a command to evaluate the debug information on the window object:
webdev/dwds/debug_extension/web/background.dart
Lines 454 to 461 in cb06447
This will no longer be permitted with manifest V3. Instead, we are having the injected client send that information to the extension in the custom-event used to notify the extension that the app is ready.
We are also including in that debug information whether or not it is an internal (google3) build.