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

Added WebDriver DeleteCookies Function #23006

Merged
merged 1 commit into from Apr 19, 2019

Conversation

Projects
None yet
7 participants
@aditj
Copy link
Contributor

commented Mar 9, 2019

This change adds DeleteCookies function of the webdriver crate to the servo webdriver server.
See spec


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix part of #8623 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Mar 9, 2019

Heads up! This PR modifies the following files:

@aditj aditj force-pushed the aditj:aditj-patch-7 branch 11 times, most recently from 3b7c38c to 841e71a Mar 9, 2019

@jdm
Copy link
Member

left a comment

Good start! We need to make some changes to fully match the specification, however.

@@ -236,6 +236,10 @@ impl ResourceChannelManager {
http_state,
),
},
CoreResourceMsg::DeleteCookies() => {
http_state.cookie_jar.write().unwrap().clear_storage();

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

According to https://w3c.github.io/webdriver/#dfn-delete-cookies (which is invoked from https://w3c.github.io/webdriver/#dfn-delete-all-cookies) this can delete more cookies than we want. We will want a message that includes the list of cookies to delete and the URL associated with the deletion request.

This comment has been minimized.

Copy link
@aditj

aditj Mar 14, 2019

Author Contributor

According to https://w3c.github.io/webdriver/#dfn-delete-cookies (which is invoked from https://w3c.github.io/webdriver/#dfn-delete-all-cookies) this can delete more cookies than we want. We will want a message that includes the list of cookies to delete and the URL associated with the deletion request.

I think we dont need to give a list of cookies and the URL associated since the spec we need to delete all the cookies pertaining to a particular document. (I dont find the need of a URL)

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

You're right about one part; https://w3c.github.io/webdriver/#dfn-associated-cookies means that we do not need to create a list of cookies to delete ahead of time. However, https://w3c.github.io/webdriver/#dfn-delete-cookies means that we some way of indexing into the cookie storage (see how CookieStorage::cookies_for_url does that).

This comment has been minimized.

Copy link
@jdm

jdm Mar 25, 2019

Member

We still need to accept a URL in this message and use that to clear only the cookies for that document.

@@ -970,6 +970,13 @@ impl Handler {
}
}

fn handle_delete_cookies(&self) -> WebDriverResult<WebDriverResponse> {
let (sender,_) = ipc::channel().unwrap();

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

We should wait for the response from the receiver that's created here and return any error that occurs.

@@ -83,7 +83,9 @@ impl CookieStorage {
Ok(None)
}
}

pub fn clear_storage(&mut self) {
self.cookies_map.clear();

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

According to https://w3c.github.io/webdriver/#dfn-delete-cookies we should set the cookie expiry time to the past, rather than deleting the actual stored cookie.

This comment has been minimized.

Copy link
@aditj

aditj Mar 14, 2019

Author Contributor

Okay I'll try to implement that :)

This comment has been minimized.

Copy link
@aditj

aditj Mar 14, 2019

Author Contributor

@jdm a little help , what should I assign the value of cookie's expiry_time variable to?

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

Try now().to_timespec() - Duration::seconds(1)?

This comment has been minimized.

Copy link
@aditj

aditj Mar 15, 2019

Author Contributor

I dont think we need it in timespec since the type of expiry_time is Tm.

@@ -1840,6 +1840,9 @@ impl ScriptThread {
WebDriverScriptCommand::AddCookie(params, reply) => {
webdriver_handlers::handle_add_cookie(&*documents, pipeline_id, params, reply)
},
WebDriverScriptCommand::DeleteCookies(reply) => {
webdriver_handlers::handle_delete_cookies(&*documents,reply)

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

We should pass pipeline_id in order to choose a particular document.

This comment has been minimized.

Copy link
@aditj

aditj Mar 14, 2019

Author Contributor

Okay I'll delete cookies of a particular document.
But I think this discussion is worth a read.

@@ -356,6 +356,17 @@ pub fn handle_add_cookie(
.unwrap();
}

pub fn handle_delete_cookies(documents: &Documents,reply: IpcSender<Result<(),()>>) {
for (id, document) in documents.iter() {

This comment has been minimized.

Copy link
@jdm

jdm Mar 14, 2019

Member

We will want to:

  1. choose a particular document
  2. get all of its cookies
  3. send a message deleting those cookies for this document's URL

This comment has been minimized.

Copy link
@aditj

aditj Mar 16, 2019

Author Contributor

@jdm is the algorithm compliant now?

@jdm jdm assigned jdm and unassigned emilio Mar 14, 2019

@aditj aditj force-pushed the aditj:aditj-patch-7 branch from 841e71a to 328dbd6 Mar 14, 2019

@aditj aditj force-pushed the aditj:aditj-patch-7 branch 5 times, most recently from 289142a to a3b1ab7 Mar 14, 2019

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2019

Error syncing changes upstream. Logs saved in error-snapshot-1552691546047.

@aditj aditj force-pushed the aditj:aditj-patch-7 branch from a3b1ab7 to 4f34cb5 Mar 15, 2019

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2019

Error syncing changes upstream. Logs saved in error-snapshot-1552691852886.

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Unfortunately something went wrong with the last rebase (or merge), so we will need to figure out how to extract the actual changes from this PR's list of commits.

@aditj aditj force-pushed the aditj:aditj-patch-7 branch from c35438b to 8aa20ba Apr 12, 2019

@aditj aditj force-pushed the aditj:aditj-patch-7 branch 2 times, most recently from 1589f5b to 2f70220 Apr 12, 2019

@CYBAI

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

If it's hard to rebase, I'd suggest

  1. cherry-pick the commit for this PR into a new branch based on latest master
  2. rename aditj-patch-7 to a temp name
  3. rename the new branch to aditj-patch-7

Then you should be able to have a clean branch 🤔 After confirming the new branch works fine, then you can remove the old branch which you renamed at step 2.

@jdm jdm removed the S-awaiting-review label Apr 16, 2019

@aditj

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

If it's hard to rebase, I'd suggest

  1. cherry-pick the commit for this PR into a new branch based on latest master
  2. rename aditj-patch-7 to a temp name
  3. rename the new branch to aditj-patch-7

Then you should be able to have a clean branch After confirming the new branch works fine, then you can remove the old branch which you renamed at step 2.

Its a merge commit,cherry-pick isn't working :(

@aditj aditj force-pushed the aditj:aditj-patch-7 branch from 2f70220 to d0024fb Apr 17, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@aditj In that case, I recommend taking the output of https://patch-diff.githubusercontent.com/raw/servo/servo/pull/23006.diff, saving it, then running git reset --hard 04c93c511b021a1e76512f997992667540657c4e, followed by git apply 23006.diff, committing the changes, then force pushing your branch.

@aditj

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@aditj In that case, I recommend taking the output of https://patch-diff.githubusercontent.com/raw/servo/servo/pull/23006.diff, saving it, then running git reset --hard 04c93c511b021a1e76512f997992667540657c4e, followed by git apply 23006.diff, committing the changes, then force pushing your branch.

Thanks @jdm ! I didnt know such a tool existed 😄 , I'll try it!

@aditj

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@jdm I (and git) can't find that commit id , where did you get it from?

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@aditj That was the most recent commit from master. If you git fetch from servo/servo, you should be able to use it.

@aditj aditj force-pushed the aditj:aditj-patch-7 branch from d0024fb to 64961cc Apr 19, 2019

@aditj

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@aditj That was the most recent commit from master. If you git fetch from servo/servo, you should be able to use it.

I think it looks clean now :D
Thanks a lot @jdm and @CYBAI !

@CYBAI CYBAI removed the S-needs-rebase label Apr 19, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

📌 Commit 64961cc has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

⌛️ Testing commit 64961cc with merge 53b752b...

bors-servo added a commit that referenced this pull request Apr 19, 2019

Auto merge of #23006 - aditj:aditj-patch-7, r=jdm
Added WebDriver DeleteCookies Function

<!-- Please describe your changes on the following line: -->
This change adds DeleteCookies function of the webdriver crate to the servo webdriver server.
See [spec](https://w3c.github.io/webdriver/#delete-all-cookies)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix part of #8623 (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/23006)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

@bors-servo bors-servo merged commit 64961cc into servo:master Apr 19, 2019

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request 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
You can’t perform that action at this time.