Skip to content
This repository has been archived by the owner on Apr 1, 2019. It is now read-only.

Add Get Window Position and Set Window Position commands #37

Merged
merged 1 commit into from
Sep 26, 2016
Merged

Add Get Window Position and Set Window Position commands #37

merged 1 commit into from
Sep 26, 2016

Conversation

andreastt
Copy link
Contributor

@andreastt andreastt commented Jun 28, 2016

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


This change is Reviewable

@andreastt andreastt changed the title add Get Window Position and Set Window Position commands Add Get Window Position and Set Window Position commands Jun 28, 2016
@andreastt
Copy link
Contributor Author

@jgraham r?

@jgraham
Copy link
Member

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
Copy link
Contributor Author

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

Commands are added to the WebDriver specification in
w3c/webdriver#307.
@andreastt
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@jgraham Mind taking another look?

@AutomatedTester
Copy link

@jgraham ping

@jgraham
Copy link
Member

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants