Repository navigation
Initial implementation of network profiling page - #1508
Conversation
There was a problem hiding this comment.
I've seen lots of people make meaningful engineering contributions to Flutter apps within their first week, but this is still very impressive.
I left a couple of suggestions on style and structural improvements.
I also left some changes necessary to know the code will work through failure cases.
Also, can you please include a screenshot of this page?
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| /// This library contains code pulled from dart:io that is not platform specific. |
There was a problem hiding this comment.
If this works on web, it's a shame we can't just import it from dart:io.
There was a problem hiding this comment.
@ditman who is interested in answering just that sort of question. :)
There was a problem hiding this comment.
Yup, there's no platform specific code in a large portion of the dart:_http code :-(
There was a problem hiding this comment.
This is great, and I don't want to derail this PR, but maybe some of the base work here (Cookie, Http Headers...) may have a better home in package:http?
(This is something that can be extracted/refactored later)
| import '../network_controller.dart'; | ||
| import 'http_request_inspector.dart'; | ||
|
|
||
| class HttpRequestDataTableSource extends DataTableSource { |
There was a problem hiding this comment.
This class will need tests.
kenzieschmoll
left a comment
There was a problem hiding this comment.
done with initial round of comments.
Great to see this code! Can you add some screenshots to the PR description?
| Future<void> _forEachIsolate(Future Function(IsolateRef) callback) async { | ||
| final vm = await serviceManager.service.getVM(); | ||
| final futures = <Future>[]; | ||
| for (final isolate in vm.isolates) { |
There was a problem hiding this comment.
is vm.isolates subject to change while this for loop is executing?
There was a problem hiding this comment.
Theoretically a new isolate could be spawned while this loop is executing but I'm not sure it's worth handling that edge case unless it becomes a problem.
|
As a drive by comment, when adding or changing significant UI you probably want to add a few screenshots - it'll help reviewers get a sense for the UI changes. |
| requests: currentValues, outstanding: outstandingRequestsMap); | ||
| } | ||
|
|
||
| void _startPolling() { |
There was a problem hiding this comment.
can you also document how we make sure we stop polling?
jacob314
left a comment
There was a problem hiding this comment.
This is a an amazing start!
To test this code there are two reasonable options:
- write a combination of unittests tests against the controller and widget tests that exercise the the full ui.
- write all tests as widget tests against the full ui.
You can see the existing tests for examples of controller tests that connect the controller to a real dart application. We don't yet have widget tests that connect to a real dart application that I know of but that is something we should change soon.
|
Thanks everyone for the initial comments! You're going to keep me busy... :-) I've uploaded screenshots in the PR description. Let me know if there's something I missed that you'd like a screenshot of. |
|
I think I've covered the majority of the requested refactoring and theming work (still need to deal with the paddings). I'll get started on tests once people are happy with the general structure of the change. Please take another look when you have a chance. Also, flutter/flutter#47552 and flutter/flutter#48531 have landed so this change should work properly as of flutter/flutter@d907a26. |
more comments to timeout code in NetworkService
jacob314
left a comment
There was a problem hiding this comment.
Great work!
* Hide expand/collapse buttons when LayoutExplorer is in view.
Looking for initial comments and feedback. Happy to add tests when I'm up to it, if requested.
This isn't quite ready to land as it depends on the following PRs landing in the Flutter framework:
showCheckboxColumnparameter to DataTable and PaginatedDataTable flutter#47552Screenshots:
No requests recorded
Recording, waiting for first request
Recordings received, none selected
General request information and headers
Request timing information
Cookies tab (only response cookies shown here, but response cookies are also displayed). This tab is hidden when there are no cookies for a request
