[webview_flutter_android] Add headers to NavigationRequest to tell if it isDownloadRequest from onDownloadStart#11502
[webview_flutter_android] Add headers to NavigationRequest to tell if it isDownloadRequest from onDownloadStart#11502wuzhengu wants to merge 2 commits intoflutter:mainfrom
Conversation
… it isDownloadRequest from onDownloadStart
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a headers field to the NavigationRequest class and introduces an isDownloadRequest property derived from the content-disposition header. The Android implementation is updated to populate these headers during download events. Feedback indicates that deriving isDownloadRequest from headers creates a fragile dependency and that the logic prepending 'attachment; ' to the content-disposition header may result in invalid header values; it is recommended to use an explicit boolean field instead.
| const NavigationRequest({required this.url, required this.isMainFrame, this.headers}); | ||
|
|
||
| /// The URL of the pending navigation request. | ||
| final String url; | ||
|
|
||
| /// Indicates whether the request was made in the web site's main frame or a subframe. | ||
| final bool isMainFrame; | ||
|
|
||
| /// Headers for the request. | ||
| final Map<String, String>? headers; | ||
|
|
||
| /// Indicates whether the request was made to download an attachment. | ||
| bool get isDownloadRequest => | ||
| headers?['content-disposition']?.toLowerCase().startsWith('attachment') ?? false; |
There was a problem hiding this comment.
Instead of deriving isDownloadRequest from a specific header value, it is more robust and maintainable to add an explicit boolean field to the NavigationRequest class.
The current implementation creates a "magic string" dependency between the platform interface and the implementations (e.g., requiring the Android implementation to ensure a specific header key and value exist). It also conflates request headers with response metadata. Adding a formal isDownloadRequest parameter to the constructor is a cleaner approach.
const NavigationRequest({
required this.url,
required this.isMainFrame,
this.headers,
this.isDownloadRequest = false,
});
/// The URL of the pending navigation request.
final String url;
/// Indicates whether the request was made in the web site's main frame or a subframe.
final bool isMainFrame;
/// Headers for the request.
final Map<String, String>? headers;
/// Indicates whether the request was made to download an attachment.
final bool isDownloadRequest;| if (!contentDisposition.toLowerCase().startsWith('attachment')) { | ||
| contentDisposition = 'attachment; $contentDisposition'.trim(); | ||
| } |
There was a problem hiding this comment.
This logic modifies the content-disposition header by prepending "attachment; ". This is problematic because it can result in an invalid or misleading header value if the original header already contains other directives (for example, if it was already "inline; filename=...", it becomes "attachment; inline; filename=...").
If an explicit boolean field is added to NavigationRequest (as suggested in the platform interface), this spoofing logic can be removed, and the actual headers from the server can be passed through unmodified.
|
The Gemini review is correct; this is not how we would implement this. Also, per https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#platform-support, the first step here would be investigating whether there is a way to support this on iOS, as we prefer not to add platform-specific features when the behavior is something that would apply to other platforms. If you would like to move forward with this, the next step would be to evaluate how this could work on iOS. |
2 participants
Add
headersandisDownloadRequesttoNavigationRequest，so we can easily tell if it was made to download an attachment fromonDownloadStart.List which issues are fixed by this PR. You must list at least one issue.
132738
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2