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

Move HSTS/CookieStorage to Arc<RwLock> from Ipc #7654

Merged
merged 3 commits into from Sep 19, 2015

Conversation

@samfoo
Copy link
Contributor

samfoo commented Sep 17, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 18, 2015

The latest upstream changes (presumably #7447) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Sep 18, 2015

Looking pretty good!
-S-awaiting-review +S-needs-code-changes


Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.


components/net/http_loader.rs, line 326 [r1] (raw file):
nit: &Arc


components/net/http_loader.rs, line 335 [r1] (raw file):
nit: &Arc


components/net/http_loader.rs, line 351 [r1] (raw file):
nit: &Arc


components/net/http_loader.rs, line 363 [r1] (raw file):
nit: &Arc


components/net/http_loader.rs, line 504 [r1] (raw file):
I think I'd still prefer the request_must_be_secured abstraction instead of inlining this here.


components/net/resource_task.rs, line 166 [r1] (raw file):
Is this clone necessary?


components/net/resource_task.rs, line 167 [r1] (raw file):
This could use read instad, right?


components/net/resource_task.rs, line 208 [r1] (raw file):
Is this clone necessary?


tests/unit/net/hsts.rs, line 15 [r1] (raw file):
Why remove this test?


tests/unit/net/http_loader.rs, line 180 [r1] (raw file):
This can be read, right?


tests/unit/net/http_loader.rs, line 401 [r1] (raw file):
Why's this clone necessary?


tests/unit/net/http_loader.rs, line 434 [r1] (raw file):
Unnecessary clone?


tests/unit/net/http_loader.rs, line 484 [r1] (raw file):
Let's reuse url.


Comments from the review on Reviewable.io

@samfoo
Copy link
Contributor Author

samfoo commented Sep 19, 2015

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


components/net/resource_task.rs, line 167 [r1] (raw file):
Very confusingly, no. CookieStorage::cookies_for_url takes &mut self. I haven't investigated why.


tests/unit/net/hsts.rs, line 15 [r1] (raw file):
The resource manager is now just responsible for owning the HSTS list. Adding and removing entries is only done on the list, rather than through the manager. This test no longer is really testing anything.


tests/unit/net/http_loader.rs, line 180 [r1] (raw file):
Same as above.


tests/unit/net/http_loader.rs, line 401 [r1] (raw file):
Load consumes hsts_list by value. The assertion on the line below needs to use the list again. In this case, cloning an Arc shouldn't matter anyway, right? It's not really cloning the list, just the pointer?


tests/unit/net/http_loader.rs, line 434 [r1] (raw file):
Same as above.


Comments from the review on Reviewable.io

@samfoo samfoo force-pushed the samfoo:hsts-cookie-arc branch from 5b3aa36 to cd95319 Sep 19, 2015
@samfoo samfoo force-pushed the samfoo:hsts-cookie-arc branch from cd95319 to 6d20b38 Sep 19, 2015
@jdm
Copy link
Member

jdm commented Sep 19, 2015

-S-awaiting-review -S-needs-rebase +S-needs-code-changes


Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


components/net/resource_task.rs, line 167 [r1] (raw file):
Oh right, we update the last used time in the cookies that get retrieved.


tests/unit/net/http_loader.rs, line 401 [r1] (raw file):
load consumes the result of hsts_list.clone(), and hsts_list remains unconsumed.


Comments from the review on Reviewable.io

@samfoo
Copy link
Contributor Author

samfoo commented Sep 19, 2015

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


tests/unit/net/http_loader.rs, line 401 [r1] (raw file):
Whoops, you're right! I got a bit carried away there. Fixed.


Comments from the review on Reviewable.io

@samfoo samfoo force-pushed the samfoo:hsts-cookie-arc branch from 37a4155 to 6411192 Sep 19, 2015
@samfoo
Copy link
Contributor Author

samfoo commented Sep 19, 2015

Review status: 5 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


tests/unit/net/http_loader.rs, line 434 [r1] (raw file):
Removed, as above.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Sep 19, 2015

@bors-servo: r+


Reviewed 1 of 1 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2015

📌 Commit 6411192 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2015

Testing commit 6411192 with merge 7f2d819...

bors-servo pushed a commit that referenced this pull request Sep 19, 2015
Move HSTS/CookieStorage to Arc<RwLock> from Ipc

#7421

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

bors-servo commented Sep 19, 2015

💔 Test failed - mac-rel-wpt

@jdm
Copy link
Member

jdm commented Sep 19, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2015

Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css are reusable. Rebuilding only linux-rel, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Sep 19, 2015

@bors-servo bors-servo merged commit 6411192 into servo:master Sep 19, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@samfoo samfoo deleted the samfoo:hsts-cookie-arc branch Sep 19, 2015
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.

None yet

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