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

read cookie_jar, hsts_list, auth_cache, and local_data from file if profile_dir option is present #10800

Merged
merged 1 commit into from Apr 26, 2016

Conversation

@DDEFISHER
Copy link
Contributor

DDEFISHER commented Apr 21, 2016

Last step in Persistent sessions student project

"check for the presence of the profile directory command-line option in the ResourceThread constructor and look for files that will contain serialized versions of the previous steps. If they exist, populate the appropriate fields with deserialized versions of the file contents."

Also populated local_data in StorageManager constructor

I am not sure if the handling of decoding and encoding the Option Tm type was done in the cleanest way here.


This change is Reviewable

@highfive
Copy link

highfive commented Apr 21, 2016

Heads up! This PR modifies the following files:

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

highfive commented Apr 21, 2016

warning Warning warning

  • These commits modify net code, but no tests are modified. Please consider adding a test!
@jdm jdm assigned jdm and unassigned Manishearth Apr 21, 2016
@DDEFISHER
Copy link
Contributor Author

DDEFISHER commented Apr 22, 2016

fixed tidy errors

@Manishearth
Copy link
Member

Manishearth commented Apr 22, 2016

The encoding can be done in a much cleaner way by using RsCookie inside Cookie and just sticking #[derive(Encodable,Decodable)] on top of it. Implementations of Encodable/Decodable are rarely manually written.

Actually, I think it might be better to just make cookie::Cookie serializable by sticking that derive annotation on it here

@Manishearth
Copy link
Member

Manishearth commented Apr 22, 2016

Also, I'm going to be busy the next few days due to exams. r? @jdm

(or anyone else)

@DDEFISHER
Copy link
Contributor Author

DDEFISHER commented Apr 22, 2016

Sounds good I will make a pull request to add the derive annotation to Cookie and Tm

@frewsxcv frewsxcv assigned frewsxcv and unassigned jdm Apr 22, 2016
@frewsxcv
Copy link
Member

frewsxcv commented Apr 22, 2016

Looks great, thanks!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Apr 22, 2016

📌 Commit 1a35d75 has been approved by frewsxcv

@Manishearth
Copy link
Member

Manishearth commented Apr 22, 2016

@bors-servo r-

Not till the manual impls go away 😄

@DDEFISHER
Copy link
Contributor Author

DDEFISHER commented Apr 23, 2016

Does the fact that components/net is using version 2.0 of cookie:Cookie present a problem to adding the derive to the Cookie struct in cookie:Cookie since it is on version 2.3 ?

@frewsxcv
Copy link
Member

frewsxcv commented Apr 23, 2016

I don't see any reason why we couldn't upgrade to version 2.3. Just need to make the changes upstream to cookie-rs, then run ./mach cargo-update -p cookie

@frewsxcv
Copy link
Member

frewsxcv commented Apr 25, 2016

A new version of the cookie crate has been pushed.

@DDEFISHER
Copy link
Contributor Author

DDEFISHER commented Apr 25, 2016

Thanks for the heads up!

@frewsxcv
Copy link
Member

frewsxcv commented Apr 25, 2016

Ping me when you get around to updating this (GitHub does send notifications for new pushes). Thanks!

@DDEFISHER
Copy link
Contributor Author

DDEFISHER commented Apr 25, 2016

looks like I will have to rebase

@frewsxcv
Copy link
Member

frewsxcv commented Apr 25, 2016

thread '<main>' panicked at 'a package was referenced twice in the lockfile', src/cargo/core/resolver/encode.rs:51
@DDEFISHER
Copy link
Contributor Author

DDEFISHER commented Apr 25, 2016

Sorry I am fixing this now

@frewsxcv
Copy link
Member

frewsxcv commented Apr 25, 2016

Thanks again!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2016

📌 Commit d6c06c2 has been approved by frewsxcv

@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2016

Testing commit d6c06c2 with merge abd1e5a...

bors-servo added a commit that referenced this pull request Apr 25, 2016
read cookie_jar, hsts_list, auth_cache, and local_data from file if profile_dir option is present

Last step in Persistent sessions student project

"check for the presence of the profile directory command-line option in the ResourceThread constructor and look for files that will contain serialized versions of the previous steps. If they exist, populate the appropriate fields with deserialized versions of the file contents."

Also populated local_data in StorageManager constructor

I am not sure if the handling of decoding and encoding the Option Tm type was done in the cleanest way here.

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

bors-servo commented Apr 25, 2016

💔 Test failed - linux-dev

@KiChjang
Copy link
Member

KiChjang commented Apr 25, 2016

thread '<main>' panicked at 'a package was referenced twice in the lockfile', src/cargo/core/resolver/encode.rs:51
@frewsxcv
Copy link
Member

frewsxcv commented Apr 25, 2016

Looks like cef and gonk lockfiles also need updating.

…rofile_dir option is present
@frewsxcv
Copy link
Member

frewsxcv commented Apr 26, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Apr 26, 2016

📌 Commit d9c32b2 has been approved by frewsxcv

@bors-servo
Copy link
Contributor

bors-servo commented Apr 26, 2016

Testing commit d9c32b2 with merge 7a582b4...

bors-servo added a commit that referenced this pull request Apr 26, 2016
read cookie_jar, hsts_list, auth_cache, and local_data from file if profile_dir option is present

Last step in Persistent sessions student project

"check for the presence of the profile directory command-line option in the ResourceThread constructor and look for files that will contain serialized versions of the previous steps. If they exist, populate the appropriate fields with deserialized versions of the file contents."

Also populated local_data in StorageManager constructor

I am not sure if the handling of decoding and encoding the Option Tm type was done in the cleanest way here.

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

bors-servo commented Apr 26, 2016

@bors-servo bors-servo merged commit d9c32b2 into servo:master Apr 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
srm09 added a commit to srm09/servo that referenced this pull request Apr 26, 2016
srm09 added a commit to srm09/servo that referenced this pull request Apr 29, 2016
srm09 added a commit to srm09/servo that referenced this pull request Apr 29, 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

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