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

common, response: add window state to window rect response #109

Merged
merged 1 commit into from
Aug 7, 2017
Merged

common, response: add window state to window rect response #109

merged 1 commit into from
Aug 7, 2017

Conversation

andreastt
Copy link
Contributor

@andreastt andreastt commented Aug 7, 2017

The WebDriver specification recently introduced an additional "state"
field for the window rect.


This change is Reviewable

@jgraham
Copy link
Member

jgraham commented Aug 7, 2017

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


src/common.rs, line 227 at r1 (raw file):

///
/// [`normal`]: #variant.Normal
#[derive(RustcEncodable, Debug)]

Why RustcEncodable?


Comments from Reviewable

The WebDriver specification recently introduced an additional "state"
field for the window rect.
@andreastt
Copy link
Contributor Author

Why RustcEncodable?

This isn’t needed. Dropped.

@jgraham
Copy link
Member

jgraham commented Aug 7, 2017

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


Comments from Reviewable

@jgraham jgraham merged commit 2b54d3d into mozilla:master Aug 7, 2017
@whimboo
Copy link
Collaborator

whimboo commented Aug 7, 2017

@andreastt, this change makes geckodriver fail to compile on central now. Is there a bug which covers updating geckodriver?

@andreastt
Copy link
Contributor Author

@whimboo geckodriver doesn’t fail to compile on m-c because geckodriver is pinned to 0.28.0. This introduces some API changes which needs to be addressed next time it upgrades it dependencies…

@whimboo
Copy link
Collaborator

whimboo commented Aug 8, 2017

Ok, bad wording on my side. Due to my work for proxy and other related changes I have to work with the current webdriver-rust code. As such I cannot use the version from crates.io. It would be great if you can do this upgrade on m-c.

@andreastt
Copy link
Contributor Author

Sure thing! I’ve submitted some patches for this in https://bugzilla.mozilla.org/show_bug.cgi?id=1388365.

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