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

Add WebDriver support for getting elements by selector. #5969

Merged
merged 1 commit into from May 11, 2015

Conversation

@jgraham
Copy link
Contributor

jgraham commented May 6, 2015

Also adds example support for getting the name and text properties of the elements.

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented May 6, 2015

Critic review: https://critic.hoppipolla.co.uk/r/4927

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jdm
Copy link
Member

jdm commented May 6, 2015

-S-awaiting-review +S-needs-code-changes


Reviewed files:

  • components/script/script_task.rs @ r1
  • components/script/webdriver_handlers.rs @ r1
  • components/webdriver_server/lib.rs @ r1
  • components/webdriver_traits/lib.rs @ r1

components/script/webdriver_handlers.rs, line 28 [r1] (raw file):
nit: no need for : JSRef<Node>.


components/script/webdriver_handlers.rs, line 42 [r1] (raw file):
Let's make get_unique_id always initialize the unique id if it isn't set yet, instead.


components/script/webdriver_handlers.rs, line 105 [r1] (raw file):
unwrap_or("".to_owned())


components/script/webdriver_handlers.rs, line 115 [r1] (raw file):
nit: no need for : JSRef<Element>


components/webdriver_server/lib.rs, line 162 [r1] (raw file):
Let's move the command into a separate binding; this is a pretty long line.


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


components/webdriver_server/lib.rs, line 186 [r1] (raw file):
If you use into_iter() you don't need x.clone().


components/webdriver_server/lib.rs, line 201 [r1] (raw file):
Long line, separate binding.


components/webdriver_server/lib.rs, line 215 [r1] (raw file):
Long line; separate binding.


Comments from the review on Reviewable.io

@jgraham
Copy link
Contributor Author

jgraham commented May 7, 2015

components/script/webdriver_handlers.rs, line 105 [r1] (raw file):
It's not supposed to be possible to select non-Elements through the API, but I guess this can't hurt.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented May 7, 2015

-S-awaiting-review +S-needs-squash


Reviewed files:

  • components/script/dom/node.rs @ r2
  • components/script/webdriver_handlers.rs @ r2
  • components/webdriver_server/lib.rs @ r2

Comments from the review on Reviewable.io

Also adds example support for getting the name and text properties of the elements.
@jgraham jgraham force-pushed the jgraham:webdriver_select_css branch from ae11e1e to 9e44206 May 8, 2015
@jdm
Copy link
Member

jdm commented May 11, 2015

@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2015

📌 Commit 9e44206 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2015

Testing commit 9e44206 with merge 8df8249...

bors-servo pushed a commit that referenced this pull request May 11, 2015
Also adds example support for getting the name and text properties of the elements.

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

bors-servo commented May 11, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit 9e44206 into servo:master May 11, 2015
2 checks passed
2 checks passed
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.