Skip to content

/ servo

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

Support for byte range requests on file urls #22024

Merged
merged 2 commits into from Oct 31, 2018
Merged

Support for byte range requests on file urls #22024

merged 2 commits into from Oct 31, 2018
+394 −3

Conversation

@ferjm
Copy link
Member

ferjm commented Oct 26, 2018 •
edited by SimonSapin

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors

This is required to allow seeking on media behind file urls

This change is Reviewable
@highfive
Copy link

highfive commented Oct 26, 2018

Heads up! This PR modifies the following files:
@highfive
Copy link

highfive commented Oct 26, 2018

warning Warning warning

  • These commits modify net code, but no tests are modified. Please consider adding a test!
@ferjm ferjm mentioned this pull request Oct 26, 2018
Merged
3 of 3 tasks complete
@ferjm
Copy link
Member Author

ferjm commented Oct 26, 2018

r? @jdm
@highfive highfive assigned jdm and unassigned asajeffrey Oct 26, 2018
@jdm
jdm requested changes Oct 30, 2018
View changes
components/net/fetch/methods.rs Show resolved Hide resolved
components/net/fetch/methods.rs Outdated
body.extend_from_slice(&buffer);
let offset = usize::min({
if let Some(end) = end {
let remaining_bytes = end as usize - body.len();

This comment has been minimized.

Sign in to view
@jdm

jdm Oct 30, 2018

Member

Don't we need to include start in this calculation? Otherwise end could be much larger than body.len() even if the range is small, and we could end up with a lot of unnecessary data in the buffer.

This comment has been minimized.

Sign in to view
@ferjm

ferjm Oct 30, 2018 •
edited

Author Member

Hmm... I believe we don't need to include start here. remaining_bytes is used to identify the last chunk of data and its size, which will likely be different to FILE_CHUNK_SIZE.

But your comment made me realize that end could be larger than the file size, and so we need to narrow it down in that case.

This comment has been minimized.

Sign in to view
@jdm

jdm Oct 30, 2018

Member

Imagine FILE_CHUNK_SIZE = 10, the total file is 1000 bytes long, and the range is [500, 599]. There will be 9 iterations of reading FILE_CHUNK_SIZE bytes and appending them to body. On the 10th iteration, remaining_bytes will be 509 (599-90), which is greater than FILE_CHUNK_SIZE. We will then append FILE_CHUNK_SIZE bytes to body (10), which exceeds the actual range we want, and then continue appending more data until we either run out of file content, or the end - body.len() calculation is less than FILE_CHUNK_SIZE.

This comment has been minimized.

Sign in to view
@ferjm

ferjm Oct 31, 2018

Author Member

Of course, you are absolutely right. Thanks for the detailed explanation.
@jdm jdm mentioned this pull request Oct 30, 2018
Closed
@ferjm ferjm force-pushed the ferjm:range.request.file.urls branch from 96914cf to 96b5d10 Oct 31, 2018
@ferjm ferjm force-pushed the ferjm:range.request.file.urls branch from 96b5d10 to fe6e476 Oct 31, 2018
@jdm
Copy link
Member

jdm commented Oct 31, 2018

Test-tidy is unhappy.
ferjm added 2 commits Oct 26, 2018
@ferjm
Support for byte range requests on file urls
0f73544
@ferjm
Tests for byte range requests on file urls
Loading status checks…
594f17a
@ferjm ferjm force-pushed the ferjm:range.request.file.urls branch from fe6e476 to 594f17a Oct 31, 2018
@ferjm
Copy link
Member Author

ferjm commented Oct 31, 2018

Fixed and added some tests
@jdm
Copy link
Member

jdm commented Oct 31, 2018

@bors-servo r+
Thanks for adding those tests!
@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2018

📌 Commit 594f17a has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2018

⌛ Testing commit 594f17a with merge 4c5a3c0...
bors-servo added a commit that referenced this pull request Oct 31, 2018
@bors-servo
Auto merge of #22024 - ferjm:range.request.file.urls, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
4c5a3c0 
Support for byte range requests on file urls

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

This is required to allow seeking on media behind file urls

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22024)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2018

☀️ Test successful - android, android-mac, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, status-taskcluster, windows-msvc-dev
Approved by: jdm
Pushing 4c5a3c0 to master...
@bors-servo bors-servo merged commit 594f17a into servo:master Oct 31, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@ferjm ferjm mentioned this pull request Nov 26, 2018
Open
@ferjm ferjm added this to Done in Media playback Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
Media playback
  
Done
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@ferjm @highfive @jdm @bors-servo @asajeffrey
You can’t perform that action at this time.