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

Create an HTTP global state object #10175

Closed
jdm opened this issue Mar 24, 2016 · 6 comments
Closed

Create an HTTP global state object #10175

jdm opened this issue Mar 24, 2016 · 6 comments
Labels
A-network C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor. I-refactor No impact; the issue is one of maintainability or tidiness. Proposed solution requires refactoring.

Comments

@jdm
Copy link
Member

jdm commented Mar 24, 2016

We pass around stuff like the cookie storage, the HSTS list, the HTTP authentication cache, etc. in http_loader.rs. This also means that the unit tests have a lot of repeated initialization code that creates these objects even when they're not used. I propose we wrap them up in a struct and give it a argument-less constructor to make reading the code easier and require fewer changes when adding additional global HTTP state.

Code: components/net/http_loader.rs, components/net/resource_thread.rs, tests/unit/net/http_loader.rs

@jdm jdm added E-less-complex Straightforward. Recommended for a new contributor. I-refactor No impact; the issue is one of maintainability or tidiness. Proposed solution requires refactoring. A-network labels Mar 24, 2016
@notriddle
Copy link
Contributor

You would still be passing around that one object, right?

@jdm
Copy link
Member Author

jdm commented Mar 24, 2016

Yes.

@cbrewster
Copy link
Contributor

Should this state object include things like LoadData, or only things that can be initialized without any parameters?

ex. LoadData is setup in a test like:

let mut load_data = LoadData::new(LoadContext::Browsing, url.clone(), None);
load_data.data = None;
load_data.method = Method::Get;

@jdm
Copy link
Member Author

jdm commented Mar 24, 2016

LoadData is a per-request object, so it would not be included.

@cbrewster
Copy link
Contributor

@jdm ok makes sense, currently I have:

hsts_list
cookie_jar
auth_cache

included in the http global state object, should anything else be included/excluded?

@KiChjang KiChjang added the C-assigned There is someone working on resolving the issue label Mar 25, 2016
bors-servo pushed a commit that referenced this issue 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 -->
@cbrewster
Copy link
Contributor

Issue resolved in PR #10188.

@Ms2ger Ms2ger closed this as completed Mar 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor. I-refactor No impact; the issue is one of maintainability or tidiness. Proposed solution requires refactoring.
Projects
None yet
Development

No branches or pull requests

5 participants