Skip to content

/ servo

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

Attempt at refactoring and simplifying 'set cookies' operations on resource thread. #14526

Merged
merged 1 commit into from Dec 16, 2016
Merged

Attempt at refactoring and simplifying 'set cookies' operations on resource thread. #14526

merged 1 commit into from Dec 16, 2016
+35 −44

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Dec 9, 2016 •
edited by larsbergstrom

This change is Reviewable
@highfive
Copy link

highfive commented Dec 9, 2016

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/websocket.rs, components/script/dom/document.rs, components/script/webdriver_handlers.rs
  • @KiChjang: components/script/dom/websocket.rs, components/script/dom/document.rs, components/script/webdriver_handlers.rs, components/net/resource_thread.rs, components/net_traits/lib.rs, components/net_traits/lib.rs
@highfive
Copy link

highfive commented Dec 9, 2016

warning Warning warning

  • These commits modify net and script code, but no tests are modified. Please consider adding a test!
@frewsxcv
frewsxcv reviewed Dec 9, 2016
View changes
components/net_traits/lib.rs
ServoUrl,
#[serde(deserialize_with = "::hyper_serde::deserialize",
serialize_with = "::hyper_serde::serialize")]
Cookie,
CookieSource
),
/// Store cookies for a given originating URL
SetCookiesForUrl(ServoUrl, Vec<Serde<Cookie>>, CookieSource),

This comment has been minimized.

Sign in to view
@frewsxcv

frewsxcv Dec 9, 2016

Author Member

I think this Serde<...> abstraction can go away once we bump cookies-rs and use the serde-serialization feature
@bors-servo
Copy link
Contributor

bors-servo commented Dec 14, 2016

☔ The latest upstream changes (presumably #14570) made this pull request unmergeable. Please resolve the merge conflicts.
@jdm jdm assigned jdm and unassigned metajack Dec 15, 2016
@jdm jdm removed the S-awaiting-review label Dec 15, 2016
@jdm
jdm approved these changes Dec 15, 2016
View changes
Copy link
Member

jdm left a comment

This is good to merge after a rebase. Thanks for making this improvement!
@frewsxcv frewsxcv force-pushed the frewsxcv:cookies branch from 888a3d4 to 6915a1b Dec 15, 2016
@frewsxcv
Refactor and simplify 'set cookies' operations on resource thread.
Loading status checks…
77d2f9d
@frewsxcv frewsxcv force-pushed the frewsxcv:cookies branch from 6915a1b to 77d2f9d Dec 15, 2016
@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 15, 2016

@bors-servo r=jdm
@bors-servo
Copy link
Contributor

bors-servo commented Dec 15, 2016

📌 Commit 77d2f9d has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Dec 15, 2016

⌛ Testing commit 77d2f9d with merge bde8dce...
bors-servo added a commit that referenced this pull request Dec 15, 2016
@bors-servo
Auto merge of #14526 - frewsxcv:cookies, r=jdm
Loading status checks…
bde8dce 
Attempt at refactoring and simplifying 'set cookies' operations on resource thread.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14526)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 16, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit 77d2f9d into servo:master Dec 16, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@frewsxcv @highfive @bors-servo @jdm @metajack
You can’t perform that action at this time.