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

Implement Step 1 of Response API Clone Method #13895

Merged
merged 1 commit into from Oct 23, 2016
Merged

Implement Step 1 of Response API Clone Method #13895

merged 1 commit into from Oct 23, 2016

Conversation

@lautat
Copy link
Contributor

lautat commented Oct 22, 2016 •
edited by larsbergstrom

Implements first step of response API Clone method. Response Clone test expectations have been updated. Fixes issue #13888.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #13888.
  • There are tests for these changes

This change is Reviewable
@highfive
Copy link

highfive commented Oct 22, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @KiChjang (or someone else) soon.
@highfive
Copy link

highfive commented Oct 22, 2016

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/response.rs
  • @KiChjang: components/script/dom/response.rs
@KiChjang
KiChjang requested changes Oct 22, 2016
View changes
Copy link
Member

KiChjang left a comment

Looks good! Just 2 minor nits and r=me.
components/script/dom/response.rs Outdated
@@ -298,7 +298,9 @@ impl ResponseMethods for Response {
// https://fetch.spec.whatwg.org/#dom-response-clone
fn Clone(&self) -> Fallible<Root<Response>> {
// Step 1
// TODO: This step relies on body and stream, which are still unimplemented.
if self.is_locked() || self.BodyUsed() {

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Oct 22, 2016

Member

nit: This can just be self.body_used.get() instead of self.BodyUsed().
components/script/dom/response.rs Outdated
@@ -298,7 +298,9 @@ impl ResponseMethods for Response {
// https://fetch.spec.whatwg.org/#dom-response-clone
fn Clone(&self) -> Fallible<Root<Response>> {
// Step 1
// TODO: This step relies on body and stream, which are still unimplemented.
if self.is_locked() || self.BodyUsed() {
return Err(Error::Type("cannot clone a disturbed response".to_string()))

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Oct 22, 2016

Member

nit: Add a semi-colon at the end of this line.
@lautat
Implement response API Clone step 1
Loading status checks…
65a7747
@jdm
Copy link
Member

jdm commented Oct 23, 2016

@bors-servo: r=KiChjang
@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2016

📌 Commit 65a7747 has been approved by KiChjang
@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2016

⌛ Testing commit 65a7747 with merge 2772626...
bors-servo added a commit that referenced this pull request Oct 23, 2016
@bors-servo
Auto merge of #13895 - lautat:dom-response-clone-step-1, r=KiChjang
Loading status checks…
2772626 
Implement Step 1 of Response API Clone Method

<!-- Please describe your changes on the following line: -->
Implements first step of response API Clone method. Response Clone test expectations have been updated. Fixes issue #13888.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13888.

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/13895)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2016

💔 Test failed - mac-rel-css
@jdm
Copy link
Member

jdm commented Oct 23, 2016

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2016

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only mac-rel-css...
@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit 65a7747 into servo:master Oct 23, 2016
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
@lautat lautat deleted the lautat:dom-response-clone-step-1 branch Oct 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KiChjang KiChjang

Assignees

@KiChjang KiChjang

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Write Step 1 of the Response API `Clone()` method
5 participants
@lautat @highfive @jdm @bors-servo @KiChjang
You can’t perform that action at this time.