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
[web] consolidate network code into httpFetch #39657
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.
LGTM
lib/web_ui/dev/test_platform.dart
Outdated
Future<shelf.Response> _testPayloadGenerator(shelf.Request request) async { | ||
if (!request.requestedUri.path.endsWith('/long_test_payload')) { | ||
return shelf.Response.notFound( | ||
'This request is not handled by the screenshot handler'); |
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.
copy-paste?
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.
Yep! Fixed.
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.
Looks great!
…120845) * b310be5ce Revert "Revert "Add support for double tap action from Apple Pencil 2 (#39267)" (#39607)" (flutter/engine#39637) * 7c24f2ff3 Roll Dart SDK from 5d17a336bdfe to a594e34e85b6 (1 revision) (flutter/engine#39656) * fa1b2dd40 Roll Skia from 21627ff455d0 to bb3d8185f067 (13 revisions) (flutter/engine#39661) * 02a379db1 [web] consolidate network code into httpFetch (flutter/engine#39657)
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.
🚀
@override | ||
String toString() => 'Failed to load asset at "$url" ($httpStatus)'; | ||
return (await response.payload.asByteBuffer()).asByteData(); | ||
} | ||
} | ||
|
||
/// An asset manager that gives fake empty responses for assets. |
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 the WebOnlyMockAssetManager
class (and in general all the new Mock classes) be moved somewhere under test
?
@visibleForTesting | ||
Future<HttpFetchResponse> testOnlyHttpPost(String url, String data) async { |
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 there any possibility that these test-only functions can be defined in test code only? That way you wouldn't need the @visibleForTesting
annotation.
|
||
/// Convenience function for making a fetch request and getting the data as a | ||
/// [ByteBuffer], when the default error handling mechanism is sufficient. | ||
Future<ByteBuffer> httpFetchByteBuffer(String url) async { |
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.
There's a fair amount of response.asUint8List
uses, maybe it's worth to have a httpFetchUint8List
method as well?
if (mockHttpFetchResponseFactory != null) { | ||
return mockHttpFetchResponseFactory!(url); | ||
} | ||
try { | ||
final _DomResponse domResponse = await _rawHttpGet(url); | ||
return HttpFetchResponseImpl._(url, domResponse); |
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.
Since _DomResponse is a @staticInterop
object, you may be able to simplify this a bunch by just making the mockHttpFetchResponseFactory
return a
js_util.jsify({
// Something that resembles a _DomResponse
}) as _DomResponse;
(Or even create a real _DomResponse object with its constructor)
That would allow you to simplify the objects returned by fetch quite a bit, and also:
try {
_DomResponse response;
if (mockHttpFetchResponseFactory != null) {
response = await mockHttpFetchResponseFactory!(url);
} else {
response = await _rawHttpGet(url);
}
return HttpFetchResponseImpl._(url, domResponse);
} catch (...) {
//...
}
test what happens when the mockHttpRequestFactory throws.
Consolidate all of web engine HTTP code into a single
httpFetch
function that provides a standard way for handling errors:httpFetch
and a few specialize convenience functions around it for making HTTP calls.httpFetch
(fonts, assets, network images,