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

Replace Rc<RefCell<Request>> with Rc<Request> #9172

Merged
merged 1 commit into from Jan 8, 2016
Merged

Replace Rc<RefCell<Request>> with Rc<Request> #9172

merged 1 commit into from Jan 8, 2016

Conversation

@nikkisquared
Copy link
Contributor

nikkisquared commented Jan 6, 2016

To make using Request easier, I've changed it from an Rc<RefCell<T>> to just Rc<T>, and updated individual fields to RefCell<T> or Cell<T> as needed. Git seems to be telling me there's a conflict with request.rs, which I resolved locally, but I'm not sure what to do (if anything) to make it easier to merge.

Review on Reviewable
@jdm
Copy link
Member

jdm commented Jan 6, 2016

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 6, 2016

📌 Commit 57ef2ab has been approved by jdm
components/net/fetch/request.rs Outdated

let req = request.borrow();
let url = req.url_list.last().unwrap();
let url = request.current_url().clone();

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Jan 6, 2016

Member

Why is there a clone here?

This comment has been minimized.

Sign in to view
Copy link
@nikkisquared

nikkisquared Jan 6, 2016

Author Contributor

that was an oversight on my part, I've fixed it locally now!

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Jan 6, 2016

Member

There are other places where current_url() is followed by a clone(). Make sure you fix those as well.
@nikkisquared nikkisquared force-pushed the nikkisquared:rewrap_request branch from 358d6ca to 15249fa Jan 6, 2016
components/net/fetch/request.rs Outdated
let referer;
{
referer = http_request.referer.clone();
}

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Jan 6, 2016

Member

Why is this here? Isn't it just enough to do let referer = http_request.referer.clone()?

This comment has been minimized.

Sign in to view
Copy link
@nikkisquared

nikkisquared Jan 7, 2016

Author Contributor

That's weird, I swear I ran into this and fixed it. Must've been a similar legacy handling from elsewhere. I've updated it now- no clone is needed at all! Cloning was just to get around pointer lifetimes.
@nikkisquared nikkisquared force-pushed the nikkisquared:rewrap_request branch 2 times, most recently from 7673332 to ee6b8bf Jan 7, 2016
@KiChjang
Copy link
Member

KiChjang commented Jan 7, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2016

📌 Commit ee6b8bf has been approved by KiChjang
components/net/fetch/request.rs Outdated
request.clone()
} else {
Rc::new(RefCell::new(request.borrow().clone()))
request.clone()

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Jan 7, 2016

Member

Wait, sorry, this conditional check isn't even needed at all if both branches result in request.clone().

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Jan 7, 2016

Member

According to the spec, this really should be a copy of request. So Rc::try_unwrap(request).ok().clone() would be the way here.

This comment has been minimized.

Sign in to view
Copy link
@nikkisquared

nikkisquared Jan 7, 2016

Author Contributor

guh, I thought I got that one before, too! I've reworked that whole bit, the spec requires the if statement which I added a todo about
@nikkisquared nikkisquared force-pushed the nikkisquared:rewrap_request branch from ee6b8bf to ca94d77 Jan 7, 2016
components/net/fetch/request.rs Outdated
credentials_flag: bool,
authentication_fetch_flag: bool) -> Response {

// Step 1
// should this ever not be done?
let http_request = request.clone()

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Jan 7, 2016

Member

After reading the spec, what you had before made sense, but not on the else branch. It should have been something like Rc::new(Rc::try_unwrap(request).ok().clone()) instead.
Also, missing semicolon.
@KiChjang
Copy link
Member

KiChjang commented Jan 7, 2016

@bors-servo delegate+
r=me post-nit
@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2016

✌️ @nikkisquared can now approve this pull request
@KiChjang KiChjang self-assigned this Jan 7, 2016
@nikkisquared nikkisquared force-pushed the nikkisquared:rewrap_request branch from ca94d77 to afa8762 Jan 7, 2016
@jdm
Copy link
Member

jdm commented Jan 7, 2016

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2016

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

bors-servo commented Jan 7, 2016

⌛ Testing commit afa8762 with merge 9e653b5...
bors-servo added a commit that referenced this pull request Jan 7, 2016
@bors-servo
Auto merge of #9172 - nikkisquared:rewrap_request, r=jdm
9e653b5 
Replace Rc<RefCell<Request>> with Rc<Request>

To make using Request easier, I've changed it from an Rc<RefCell<T>> to just Rc<T>, and updated individual fields to RefCell<T> or Cell<T> as needed. Git seems to be telling me there's a conflict with request.rs, which I resolved locally, but I'm not sure what to do (if anything) to make it easier to merge.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9172)
<!-- Reviewable:end -->
@jdm
Copy link
Member

jdm commented Jan 7, 2016

@bors-servo: r+
Solid.
@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2016

📌 Commit e09e3db has been approved by jdm
@nikkisquared nikkisquared mentioned this pull request Jan 7, 2016
Merged
@nikkisquared
changed Rc<RefCell<Request>> into Rc<Request>
Loading status checks…
67f4df5
@nikkisquared nikkisquared force-pushed the nikkisquared:rewrap_request branch from e09e3db to 67f4df5 Jan 7, 2016
@jdm
Copy link
Member

jdm commented Jan 7, 2016

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 7, 2016

📌 Commit 67f4df5 has been approved by jdm
nikkisquared added a commit to nikkisquared/servo that referenced this pull request Jan 7, 2016
@nikkisquared
updated lines blocked by servo#9172
ad9ee1b
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2016

⌛ Testing commit 67f4df5 with merge f0cf894...
bors-servo added a commit that referenced this pull request Jan 8, 2016
@bors-servo
Auto merge of #9172 - nikkisquared:rewrap_request, r=jdm
f0cf894 
Replace Rc<RefCell<Request>> with Rc<Request>

To make using Request easier, I've changed it from an `Rc<RefCell<T>>` to just `Rc<T>`, and updated individual fields to `RefCell<T>` or `Cell<T>` as needed. Git seems to be telling me there's a conflict with request.rs, which I resolved locally, but I'm not sure what to do (if anything) to make it easier to merge.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9172)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2016

💔 Test failed - linux-rel
@KiChjang
Copy link
Member

KiChjang commented Jan 8, 2016

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

bors-servo commented Jan 8, 2016

⌛ Testing commit 67f4df5 with merge 5cf2520...
bors-servo added a commit that referenced this pull request Jan 8, 2016
@bors-servo
Auto merge of #9172 - nikkisquared:rewrap_request, r=jdm
Loading status checks…
5cf2520 
Replace Rc<RefCell<Request>> with Rc<Request>

To make using Request easier, I've changed it from an `Rc<RefCell<T>>` to just `Rc<T>`, and updated individual fields to `RefCell<T>` or `Cell<T>` as needed. Git seems to be telling me there's a conflict with request.rs, which I resolved locally, but I'm not sure what to do (if anything) to make it easier to merge.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9172)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 67f4df5 into servo:master Jan 8, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nikkisquared nikkisquared deleted the nikkisquared:rewrap_request branch Jan 8, 2016
bors-servo added a commit that referenced this pull request Jan 8, 2016
@bors-servo
Auto merge of #9195 - nikkisquared:rewrap_response, r=jdm
Loading status checks…
b8186a2 
Replace Rc<RefCell<Response>> with Rc<Response>

Similar to why I'm doing #9172, I'm changing `Rc<RefCell<Response>>` into `Rc<Response>` and wrapping fields in `RefCell<T>` as needed. So far I've only needed to change `url_list`, but I'm sure I'll need to update more fields in the future.

There's two lines that don't compile that I'm not going to bother trying to fix until #9172 is approved, because getting them to work now will just require more changes after merging with the changes #9172 makes to them.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9195)
<!-- Reviewable:end -->
@jdm jdm removed the S-awaiting-merge label May 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@KiChjang KiChjang

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@nikkisquared @jdm @bors-servo @KiChjang @Ms2ger @highfive
You can’t perform that action at this time.