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

write cookie_jar, hsts_list, auth_cache to file if profile_dir option is present #10661

Merged
merged 1 commit into from Apr 21, 2016

Conversation

@DDEFISHER
Copy link
Contributor

DDEFISHER commented Apr 17, 2016

For Persistent sessions student project

"if the profile directory command-line option is present when the ResourceThread is instructed to exit, serialize the data contained in cookie_storage, hsts_list, and the new HTTP authorization cache, and write the serialized data in separate files inside the profile directory."

and

"perform the same serialization on shutdown for local_data in storage_thread.rs, which represents the LocalStorage API."


This change is Reviewable

@highfive
Copy link

highfive commented Apr 17, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/net/resource_thread.rs, components/net/cookie_storage.rs, components/net/cookie.rs
@highfive
Copy link

highfive commented Apr 17, 2016

warning Warning warning

  • These commits modify net code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member

jdm commented Apr 17, 2016

Good start!
-S-awaiting-review +S-needs-code-changes


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


components/net/cookie_storage.rs, line 16 [r1] (raw file):
Let's add a u32 version field as well and start with 1.


components/net/cookie_storage.rs, line 19 [r1] (raw file):
We should be able to derive RustcEncodable on CookieStorage like HSTSList.


components/net/resource_thread.rs, line 222 [r1] (raw file):
There's a lot of duplicated code in these new functions. We can make this more general, like so:

fn write_json_to_file<T: Encodable>(data: &T, profile_dir: &str, filename: &str) {
    ...
}

components/net/resource_thread.rs, line 223 [r1] (raw file):
Let's make the final structure look like:

{
    "version": 1,
    "entries": {
        ...
    },
}

components/net/resource_thread.rs, line 230 [r1] (raw file):
Path::new(profile_dir).join("auth_cache") will properly support other operating systems like Windows.


components/net/resource_thread.rs, line 364 [r1] (raw file):
We should be derive RustcEncodable on AuthCacheEntry instead, like HSTSEntry.


Comments from Reviewable

@jdm jdm assigned jdm and unassigned mbrubeck Apr 17, 2016
@DDEFISHER
Copy link
Contributor Author

DDEFISHER commented Apr 18, 2016

Should I change the auth_cache type in ResourceManager from auth_cache: Arc<RwLock<HashMap<Url, AuthCacheEntry>>>, to auth_cache: AuthCache and then create AuthCache with the following fields?

AuthCache { version: u32, entries: Arc<RwLock<HashMap<Url, AuthCacheEntry>>>, }

@jdm
Copy link
Member

jdm commented Apr 18, 2016

Sure, that seems like a useful change.

@jdm
Copy link
Member

jdm commented Apr 19, 2016

Almost there!
-S-awaiting-review +S-needs-code-changes


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


components/net/cookie.rs, line 207 [r2] (raw file):
The string still says tm_mon.


components/net/resource_thread.rs, line 210 [r2] (raw file):
Why is the clone necessary here (and subsequent lines)?


components/net/resource_thread.rs, line 211 [r2] (raw file):
Let's add warn!("Error writing auth cache to disk")


components/net/resource_thread.rs, line 214 [r2] (raw file):
Let's add .json to the filenames here.


components/net/resource_thread.rs, line 215 [r2] (raw file):
Let's add a warn! here.


components/net/resource_thread.rs, line 219 [r2] (raw file):
Let's add a warn! here.


components/net/resource_thread.rs, line 234 [r2] (raw file):
We probably shouldn't silently write an empty payload if we have a problem encoding. Let's just return here.


components/net/resource_thread.rs, line 330 [r2] (raw file):
nit: space after :


Comments from Reviewable

@DDEFISHER
Copy link
Contributor Author

DDEFISHER commented Apr 19, 2016

Went ahead and added the part to serialize local data as well for the next step
"perform the same serialization on shutdown for local_data in storage_thread.rs, which represents the LocalStorage API."

@jdm
Copy link
Member

jdm commented Apr 20, 2016

@bors-servo: r+
Thanks!


Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2016

📌 Commit b12594a has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2016

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

…ofile_dir option is present
@jdm
Copy link
Member

jdm commented Apr 20, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2016

📌 Commit d4f63cd has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2016

Testing commit d4f63cd with merge 75d99ee...

bors-servo added a commit that referenced this pull request Apr 20, 2016
write cookie_jar, hsts_list, auth_cache to file if profile_dir option is present

For Persistent sessions student project

"if the profile directory command-line option is present when the ResourceThread is instructed to exit, serialize the data contained in cookie_storage, hsts_list, and the new HTTP authorization cache, and write the serialized data in separate files inside the profile directory."

and

"perform the same serialization on shutdown for local_data in storage_thread.rs, which represents the LocalStorage API."

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

bors-servo commented Apr 20, 2016

💔 Test failed - linux-rel

@jdm
Copy link
Member

jdm commented Apr 20, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2016

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

@bors-servo
Copy link
Contributor

bors-servo commented Apr 21, 2016

@bors-servo bors-servo merged commit d4f63cd into servo:master Apr 21, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
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.