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

Webdriver - GetWindowSize/IsEnabled/IsSelected #8661

Merged

Conversation

craftytrickster
Copy link
Contributor

Added handlers for #8623:
GetWindowSize
IsEnabled(WebElement)
IsSelected(WebElement)

I am not sure how to actually verify my webdriver code works, if someone can give advice I would make the necessary changes.

Thanks!

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 24, 2015
@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @glennw (or someone else) soon.

@craftytrickster craftytrickster changed the title Webdriver - GetWindowSize/IsEnabled/IsSelected(WebElement) Webdriver - GetWindowSize/IsEnabled/IsSelected Nov 24, 2015
@jdm
Copy link
Member

jdm commented Nov 24, 2015

r? @jgraham

@jgraham
Copy link
Contributor

jgraham commented Nov 24, 2015

Looks mostly good. Unfortunately there isn't a easy way to test webdriver stuff because there doesn't exist a testsuite for the spec; I usually poke at it manually using the simple client I wrote for wptrunner. You can do this too, if you want; something like:

cd $SERVO_ROOT
source python/_virtualenv/bin/activate
./mach run [-d|-r] --webdriver 4444 &
cd tests/wpt/harness/wptrunner/executors
[i]python
import webdriver
session = webdriver.Session("127.0.0.1", 4444); session.start(); session.url = "about:blank";

and then look in webdriver.py to see more of the api that you can use. It doesn't fully implement the webdriver spec but it is rather simple to extend if you need. Sorry for the bad answer.

In any case I'll run this through its paces after the next round of fixups.


Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


components/script/webdriver_handlers.rs, line 251 [r1] (raw file):
Indentation


components/script/webdriver_handlers.rs, line 259 [r1] (raw file):
Indentation


components/script/webdriver_handlers.rs, line 274 [r1] (raw file):
Indentation


components/webdriver_server/lib.rs, line 366 [r1] (raw file):
This should in fact be a WindowSizeResponse, which means you don't need to construct an interior JSON object, just set the width and height properties in the constructor.


Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 24, 2015
@craftytrickster
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


components/script/webdriver_handlers.rs, line 286 [r1] (raw file):
according to the spec, if it is not an input or option element, I should return false by default, not Err as I am doing here. I will add the extra step to check if it is a regular html element before the else { Err } branch.


Comments from the review on Reviewable.io

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 24, 2015
@craftytrickster
Copy link
Contributor Author

Thanks @jgraham. I made the code changes you suggested. When I have time later I will look at the python test method you mentioned. I guess no repo is safe from the evil clutches of dynamically typed languages, not even Servo. 😄

@jgraham
Copy link
Contributor

jgraham commented Nov 29, 2015

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@jgraham
Copy link
Contributor

jgraham commented Nov 29, 2015

This seems to work fine. Before merging, would it be possible to get something more like an email address in the author field of the commit?

@craftytrickster
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


components/script/webdriver_handlers.rs, line 286 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@craftytrickster
Copy link
Contributor Author

Just added email details to author, should be good to go.

@jgraham
Copy link
Contributor

jgraham commented Nov 29, 2015

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 8d45107 has been approved by jgraham

@bors-servo
Copy link
Contributor

⌛ Testing commit 8d45107 with merge a877a56...

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 29, 2015
bors-servo pushed a commit that referenced this pull request Nov 29, 2015
…=jgraham

Webdriver - GetWindowSize/IsEnabled/IsSelected

Added handlers for #8623:
GetWindowSize
IsEnabled(WebElement)
IsSelected(WebElement)

I am not sure how to actually verify my webdriver code works, if someone can give advice I would make the necessary changes.

Thanks!

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8661)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit 8d45107 into servo:master Nov 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants