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

Add Http Global State Object #10188

Merged
merged 1 commit into from Mar 26, 2016
Merged

Conversation

@cbrewster
Copy link
Member

cbrewster commented Mar 25, 2016

This adds a new HttpState object which holds common http state(#10175). This reduces the amount of work that is required to add extra things to the Http state.

The HttpState object currently holds:

hsts_list: Arc::new(RwLock::new(HSTSList::new())),
cookie_jar: Arc::new(RwLock::new(CookieStorage::new())),
auth_cache: Arc::new(RwLock::new(HashMap::new())),

This change is Reviewable

@jdm jdm assigned jdm and unassigned nox Mar 25, 2016
@jdm
Copy link
Member

jdm commented Mar 25, 2016

Thanks for doing this! These changes propagate the new type to places where I don't think it really adds clarity, so the majority of my comments are pointing out places where I find the older code clearer.
-S-awaiting-review +S-needs-code-changes


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 12 unresolved discussions.


components/net/http_loader.rs, line 81 [r1] (raw file):
This doesn't need to be an Arc.


components/net/http_loader.rs, line 366 [r1] (raw file):
Let's revert this change. It makes more sense to pass a reference to the specific field we care about.


components/net/http_loader.rs, line 375 [r1] (raw file):
Let's revert this change.


components/net/http_loader.rs, line 391 [r1] (raw file):
Revert.


components/net/http_loader.rs, line 403 [r1] (raw file):
Revert.


components/net/http_loader.rs, line 520 [r1] (raw file):
Revert.


components/net/http_loader.rs, line 530 [r1] (raw file):
Revert.


components/net/http_loader.rs, line 563 [r1] (raw file):
Revert.


components/net/http_loader.rs, line 597 [r1] (raw file):
Revert.


components/net/resource_thread.rs, line 278 [r1] (raw file):
I think I'd still prefer to keep these fields and make a new HttpState instance when calling http_loader::factory instead.


components/net/websocket_loader.rs, line 30 [r1] (raw file):
Let's revert this.


components/net/websocket_loader.rs, line 65 [r1] (raw file):
Revert.


Comments from the review on Reviewable.io

@cbrewster
Copy link
Member Author

cbrewster commented Mar 25, 2016

No problem! Almost have all the changes made.


Review status: all files reviewed at latest revision, 12 unresolved discussions.


Comments from the review on Reviewable.io

@cbrewster
Copy link
Member Author

cbrewster commented Mar 25, 2016

Review status: 1 of 3 files reviewed at latest revision, 12 unresolved discussions.


components/net/http_loader.rs, line 81 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Mar 25, 2016

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


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


components/net/http_loader.rs, line 81 [r2] (raw file):
I think taking an HttpState argument here still makes sense.


components/net/http_loader.rs, line 152 [r2] (raw file):
No need for Arc here.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Mar 25, 2016

Please squash all commits as well when making the final change!
-S-awaiting-review +S-needs-code-changes +S-needs-squash


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/net/http_loader.rs, line 154 [r3] (raw file):
No need for Arc on any HttpState argument.


Comments from the review on Reviewable.io

@cbrewster
Copy link
Member Author

cbrewster commented Mar 25, 2016

On that last change, the tests often pass the HttpState object into the load function and then use the HttpState object again within an assert. Would it be best to pass a ref of the object to the load function so the object is not moved?

Replace hsts_list, auth_cache, and cookie_jar with http_state

Reverted a few changes

Moved http_state back to factory
Removed unnecessary Arc

Removed Arc for http_state
@cbrewster cbrewster force-pushed the cbrewster:http-global-state branch from 0309e49 to b09570b Mar 25, 2016
@cbrewster
Copy link
Member Author

cbrewster commented Mar 25, 2016

Squashed and removed Arc for HttpState

@jdm
Copy link
Member

jdm commented Mar 26, 2016

@bors-servo: r+
Thanks for doing this cleanup!


Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Mar 26, 2016

📌 Commit b09570b has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Mar 26, 2016

Testing commit b09570b with merge d82f97a...

bors-servo added a commit that referenced this pull request Mar 26, 2016
Add Http Global State Object

This adds a new HttpState object which holds common http state(#10175). This reduces the amount of work that is required to add extra things to the Http state.

The HttpState object currently holds:
```
hsts_list: Arc::new(RwLock::new(HSTSList::new())),
cookie_jar: Arc::new(RwLock::new(CookieStorage::new())),
auth_cache: Arc::new(RwLock::new(HashMap::new())),
```

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

bors-servo commented Mar 26, 2016

@bors-servo bors-servo merged commit b09570b into servo:master Mar 26, 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
@cbrewster cbrewster deleted the cbrewster:http-global-state branch Mar 26, 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.

None yet

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