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

Fix step 31 of the Request constructor #12845

Closed
nox opened this issue Aug 13, 2016 · 4 comments
Closed

Fix step 31 of the Request constructor #12845

nox opened this issue Aug 13, 2016 · 4 comments

Comments

@nox
Copy link
Member

@nox nox commented Aug 13, 2016

Step 31 says:

Fill r's Headers object with headers. Rethrow any exceptions.

But the code just does:

r.Headers().fill(Some(HeadersOrByteStringSequenceSequence::Headers(headers_copy)));

The compiler even rightly complains:

warning: unused result which must be used, #[warn(unused_must_use)] on by default
   --> /Users/nox/src/servo/components/script/dom/request.rs:342:9
    |
342 |         r.Headers().fill(Some(HeadersOrByteStringSequenceSequence::Headers(headers_copy)));
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The code for step 31 should be wrapped in try!().

@highfive
Copy link

@highfive highfive commented Aug 13, 2016

Please make a comment here if you intend to work on this issue. Thank you!

@nox
Copy link
Member Author

@nox nox commented Aug 13, 2016

Cc @jeenalee.

@jeenalee
Copy link
Contributor

@jeenalee jeenalee commented Aug 13, 2016

I'd like to work on this. Thank you for filing this issue!

@nox nox added the C-assigned label Aug 13, 2016
@nox
Copy link
Member Author

@nox nox commented Aug 13, 2016

Sure. :)

@jeenalee jeenalee mentioned this issue Aug 13, 2016
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Aug 13, 2016
Fix step 31 of the Request constructor.

<!-- Please describe your changes on the following line: -->
This PR fixes the step 31 of the Request constructor to fill Request's Headers object and rethrow any exceptions. Additionally, it removes unnecessary line breaks, comments, and redundant function.

---
<!-- 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 #12845 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because this PR does not change the behavior of the Request constructor.

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

In addition to fixing step 31 of the Request constructor, this
commit also:
- remove `get_current_url` function, and use `net_request::request`'s
built-in `current_url` method
- clean up unnecessary line breaks and comments

<!-- 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/12851)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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