-
Notifications
You must be signed in to change notification settings - Fork 995
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
Android range requests #5956
Open
steinjak
wants to merge
4
commits into
ionic-team:main
Choose a base branch
from
steinjak:android-range-requests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Android range requests #5956
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
8480271
Use unique instances for request headers
steinjak 1bb78ea
Skip in range response according to req header
steinjak dacbe8e
Merge branch 'ionic-team:main' into android-range-requests
steinjak 0d2ae7c
Renamed response header getter for clarity
steinjak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Am just driving by while hunting out my own problem... I wonder if a better way to do this is to make the copy only when it's about to be modified here.
I.e., instead of creating a copy every access, do it when the
tempResponseHeaders
variable is assigned at line 208:Other than that though, this code definitely makes good sense to me.
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 see what you mean (a getter should not create new objects), but this will re-introduce the problem, as there are other invocations of this method as well - all passing the returned instance into the constructor of
WebResourceResponse
(which is where the value oftempResponseHeaders
also ends up). Note that it's not the headers set on lines 225/226 that cause trouble here, but theContent-Length
header, set somewhere else downstream from this code.We could of course move the cloning to all call points of the getter, but that leaves a trap for the next person coming around and wanting to use this method, probably not knowing about this issue, so we should probably not do it without making the map read-only (through Collections.unmodifiableMap(...), ie.
this.responseHeaders = Collections.unmodifiableMap(new LinkedHashMap<String, String>(tempResponseHeaders))
on line 102). The downside to this approach, though, is that it triggers runtime exceptions - and I can't find the automated tests that will trigger these when developing, leaving it to the developer to thoroughly test all cases before submitting a patch...Maybe it would be better to rename
WebViewLocalServer.PathHandler.getResponseHeaders()
toWebViewLocalServer.PathHandler.buildDefaultResponseHeaders()
to clarify? I don't think I can see a case where the set of response headers need to be shared between responses, only cases where there is a base set of headers to be included in every response.What do you think, @peitschie ?