Skip to content
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

Update Blob::{new, new_inherited} to take Strings #11768

Merged
merged 1 commit into from Jun 17, 2016

Conversation

@achals
Copy link
Contributor

achals commented Jun 17, 2016


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11762.
  • There are tests for these changes OR
  • These changes do not require tests because no logic changes, only interface changes.

This change is Reviewable

@highfive
Copy link

highfive commented Jun 17, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/websocket.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/file.rs, components/script/dom/blob.rs, components/script/dom/testbinding.rs
@highfive
Copy link

highfive commented Jun 17, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@@ -156,7 +156,7 @@ impl Blob {
};

let slice = DataSlice::from_bytes(bytes);
Ok(Blob::new(global, BlobImpl::new_from_slice(slice), &blobPropertyBag.get_typestring()))
Ok(Blob::new(global, BlobImpl::new_from_slice(slice), blobPropertyBag.get_typestring().to_owned()))

This comment has been minimized.

@Ms2ger

Ms2ger Jun 17, 2016

Contributor

You should be able to remove this to_owned call

@@ -252,7 +252,7 @@ impl BlobMethods for Blob {
let global = self.global();
let bytes = self.get_slice_or_empty().bytes.clone();
let slice = DataSlice::new(bytes, start, end);
Blob::new(global.r(), BlobImpl::new_from_slice(slice), &relativeContentType)
Blob::new(global.r(), BlobImpl::new_from_slice(slice), relativeContentType.to_string())

This comment has been minimized.

@Ms2ger

Ms2ger Jun 17, 2016

Contributor

It would be better to use .into() here

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 17, 2016

Nice job, just two small comments.

@jdm jdm assigned Ms2ger and unassigned metajack Jun 17, 2016
@achals
Copy link
Contributor Author

achals commented Jun 17, 2016

Neato, updating.

@achals achals force-pushed the achals:blob-string branch from 0ca24d1 to af325a9 Jun 17, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 17, 2016

Thank you!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jun 17, 2016

📌 Commit af325a9 has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Jun 17, 2016

Testing commit af325a9 with merge 9208b8e...

bors-servo added a commit that referenced this pull request Jun 17, 2016
Update Blob::{new, new_inherited} to take Strings

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #11762.

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because no logic changes, only interface 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11768)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 17, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Jun 17, 2016

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform3d-translatez-001.htm
  └   → /css-transforms-1_dev/html/transform3d-translatez-001.htm 902d90a8d198625335e738587fdfe81dcc90392d
/css-transforms-1_dev/html/reference/transform3d-translatez-ref.htm b14318ccfd8a59b7b249d41521e178d617678823
Testing 902d90a8d198625335e738587fdfe81dcc90392d == b14318ccfd8a59b7b249d41521e178d617678823
/css-transforms-1_dev/html/transform3d-translatez-001.htm 902d90a8d198625335e738587fdfe81dcc90392d
/css-transforms-1_dev/html/reference/transform3d-translatez-notref.htm 902d90a8d198625335e738587fdfe81dcc90392d
Testing 902d90a8d198625335e738587fdfe81dcc90392d != 902d90a8d198625335e738587fdfe81dcc90392d
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 17, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 17, 2016

Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel, mac-rel-css...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 17, 2016

@bors-servo bors-servo merged commit af325a9 into servo:master Jun 17, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@achals achals deleted the achals:blob-string branch Jun 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.