-
Notifications
You must be signed in to change notification settings - Fork 324
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
implemented har feature #7970
implemented har feature #7970
Conversation
@kenzieschmoll @exaby73 |
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 for the contribution! I've got mostly minor comments so far.
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
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 for addressing the comments!
}; | ||
}).toList(); | ||
|
||
// Assemble the final HAR object |
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.
Ubernit: indent
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.
sorry didn't get you
packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart
Outdated
Show resolved
Hide resolved
static const onContentLoad = -1; | ||
static const onLoad = -1; | ||
static const httpVersion = 'HTTP/1.1'; | ||
static const responseHttpVersion = 'http/2.0'; |
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.
Should this be hardcoded?
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 field doesn't seem to be available in the 'DartIOHttpRequestData'
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 put some placeholder like "Unknown" instead? Maybe @brianquinlan knows if this information is available somewhere.
@kenzieschmoll as I observed the offline functionality works in these ways :
I have implemented the 1st one (changes yet to be pushed as of now). |
@bkonyi can you re-check the comments and mark them as resolved if they are addressed |
Sorry for the delay, I'll take a look. |
All of my comments are basically addressed and this LGTM overall. I'll let @kenzieschmoll give final say on the PR. Thanks for your contribution! |
Looking forward to contributing more in future, after this one is 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.
I apologize, I had a pending review that I never hit submit on. Here are my recommendations.
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/har_network_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/har_network_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/har_network_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/shared/http/http_request_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/har_data_entry.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/har_data_entry.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/har_data_entry.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/har_data_entry.dart
Outdated
Show resolved
Hide resolved
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 once all tests pass. Thanks for the feature!
Thank you for the review! I'll ensure all tests pass. Appreciate the feedback and support! |
packages/devtools_app/lib/src/shared/diagnostics/inspector_service.dart
Outdated
Show resolved
Hide resolved
@kenzieschmoll All the tests have passed can this be merged now? Or Anything else remaining? |
This PR addresses below issue -
#3806
Please add a note to
packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md
if your change requires release notes. Otherwise, add the 'release-notes-not-required' label to the PR.Pre-launch Checklist
///
).If you need help, consider asking for help on Discord.