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 delete session command. #11049

Merged
merged 1 commit into from May 10, 2016

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented May 6, 2016

Delete session is needed by the web platform test webdriver/navigation.py.


This change is Reviewable

@highfive
Copy link

highfive commented May 6, 2016

Heads up! This PR modifies the following files:

  • @jgraham: components/webdriver_server/lib.rs
@jgraham
Copy link
Contributor

jgraham commented May 6, 2016

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/webdriver_server/lib.rs, line 305 [r1] (raw file):
Does exiting race sending the response here?


Comments from Reviewable

@asajeffrey
Copy link
Member Author

asajeffrey commented May 6, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/webdriver_server/lib.rs, line 305 [r1] (raw file):
Indeed it does. I wasn't sure if we should be exiting now, or if there'd be a later exit command. Servo doesn;'t really support sessions...


Comments from Reviewable

@jgraham
Copy link
Contributor

jgraham commented May 6, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/webdriver_server/lib.rs, line 305 [r1] (raw file):
So I think what you want to do is add the Exit message in the fn delete_session in WebDriverHandler at the bottom of the file. That at least makes it my problem if there is a race ;)


Comments from Reviewable

@asajeffrey asajeffrey force-pushed the asajeffrey:webdriver-delete-session branch from 5967046 to 9005337 May 10, 2016
@highfive
Copy link

highfive commented May 10, 2016

New code was committed to pull request.

@asajeffrey
Copy link
Member Author

asajeffrey commented May 10, 2016

Rebased and responded to review comment.

Previously, highfive wrote…

New code was committed to pull request.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


components/webdriver_server/lib.rs, line 305 [r1] (raw file):

Previously, jgraham wrote…

So I think what you want to do is add the Exit message in the fn delete_session in WebDriverHandler at the bottom of the file. That at least makes it my problem if there is a race ;)


Done.


Comments from Reviewable

@jgraham
Copy link
Contributor

jgraham commented May 10, 2016

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/webdriver_server/lib.rs, line 320 [r2] (raw file):

    fn handle_get(&self, parameters: &GetParameters) -> WebDriverResult<WebDriverResponse> {
        let url = Url::parse(&parameters.url[..]).or(Url::parse("about:blank")).unwrap();

I think I have steered the working group towards making this an error. Just need to update the spec/tests.


Comments from Reviewable

@asajeffrey
Copy link
Member Author

asajeffrey commented May 10, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/webdriver_server/lib.rs, line 320 [r2] (raw file):

Previously, jgraham wrote…

I think I have steered the working group towards making this an error. Just need to update the spec/tests.


OK, we can hold off on making this change until the spec has settled down.


Comments from Reviewable

@asajeffrey asajeffrey force-pushed the asajeffrey:webdriver-delete-session branch from 9005337 to bbf9eca May 10, 2016
@highfive
Copy link

highfive commented May 10, 2016

New code was committed to pull request.

@jgraham
Copy link
Contributor

jgraham commented May 10, 2016

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


Comments from Reviewable

@jgraham
Copy link
Contributor

jgraham commented May 10, 2016

@bors-servo
Copy link
Contributor

bors-servo commented May 10, 2016

📌 Commit bbf9eca has been approved by jgraham

@bors-servo
Copy link
Contributor

bors-servo commented May 10, 2016

Testing commit bbf9eca with merge 0c673ef...

bors-servo added a commit that referenced this pull request May 10, 2016
Added webdriver delete session command.

Delete session is needed by the web platform test `webdriver/navigation.py`.

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

bors-servo commented May 10, 2016

@bors-servo bors-servo merged commit bbf9eca into servo:master May 10, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
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
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.