Add Get Window Position and Set Window Position commands #37

Merged
merged 1 commit into from Sep 26, 2016

Projects

None yet

3 participants

@andreastt
Member
andreastt commented Jun 28, 2016 edited

Commands are added to the WebDriver specification in
w3c/webdriver#307.


This change is Reviewable

@andreastt andreastt changed the title from add Get Window Position and Set Window Position commands to Add Get Window Position and Set Window Position commands Jun 28, 2016
@andreastt andreastt referenced this pull request in mozilla/geckodriver Jun 29, 2016
Closed

Missing Set Window Position endpoint #113

@andreastt
Member
@jgraham
Collaborator
jgraham commented Jun 29, 2016

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


src/command.rs, line 573 [r1] (raw file):

impl Parameters for WindowPositionParameters {
    fn from_json(body: &Json) -> WebDriverResult<WindowPositionParameters> {
        let data = try_opt!(body.as_object(), ErrorStatus::UnknownError, "Message body was not an object");

I think these should be broken over multiple lines.


src/response.rs, line 22 [r1] (raw file):

impl WebDriverResponse {
    pub fn to_json_string(self) -> String {
        let s = match self {

Please revert. The () are unneeded.


src/response.rs, line 87 [r1] (raw file):

impl WindowPositionResponse {
    pub fn new(x: u64, y: u64) -> WindowPositionResponse {
        WindowPositionResponse { x: x, y: y }

Use multiple lines.


Comments from Reviewable

@andreastt
Member

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.


src/response.rs, line 87 [r1] (raw file):

Previously, jgraham wrote…

Use multiple lines.

I’ve put this on multiple lines. However, I checked that rustfmt uses this style, which is why I chose it.

Comments from Reviewable

@andreastt andreastt add Get Window Position and Set Window Position commands
Commands are added to the WebDriver specification in
w3c/webdriver#307.
6d0e23a
@andreastt
Member

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.


src/command.rs, line 573 [r1] (raw file):

Previously, jgraham wrote…

I think these should be broken over multiple lines.

Done.

src/response.rs, line 22 [r1] (raw file):

Previously, jgraham wrote…

Please revert. The () are unneeded.

Done.

Comments from Reviewable

@andreastt
Member

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.


src/response.rs, line 87 [r1] (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

I’ve put this on multiple lines. However, I checked that rustfmt uses this style, which is why I chose it.

Done.

Comments from Reviewable

@andreastt
Member

@jgraham Mind taking another look?

@AutomatedTester
Member

@jgraham ping

@jgraham
Collaborator
jgraham commented Sep 26, 2016

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


Comments from Reviewable

@jgraham jgraham merged commit d7f0485 into mozilla:master Sep 26, 2016

2 checks passed

code-review/reviewable 3 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment