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 screenshot method #29

Merged
merged 1 commit into from
Jun 5, 2018
Merged

Add screenshot method #29

merged 1 commit into from
Jun 5, 2018

Conversation

r-darwish
Copy link

No description provided.

@jonhoo
Copy link
Owner

jonhoo commented May 29, 2018

Hi there! This looks like a good addition, thanks!
Some minor things I'd like to see fixed up before merging, but I think overall it's pretty close :)

src/error.rs Outdated
@@ -106,6 +106,9 @@ pub enum CmdError {

/// A function was invoked with an invalid argument.
InvalidArgument(String, String),

/// Base64 decode error
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be more specific here. Perhaps ImageDecodeError? And then with an appropriate comment.

src/error.rs Outdated
@@ -142,6 +145,7 @@ impl Error for CmdError {
CmdError::Json(..) => "webdriver returned incoherent response",
CmdError::NotW3C(..) => "webdriver returned non-conforming response",
CmdError::InvalidArgument(..) => "invalid argument provided",
CmdError::Base64DecodeError(..) => "Base64 decode error",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here; I'd prefer to see the message relate to image decoding.

src/lib.rs Outdated
.issue_wd_cmd(WebDriverCommand::TakeScreenshot)
.and_then(|(_, src)| {
if let Some(src) = src.as_string() {
return decode(src).map_err(error::CmdError::from);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer base64::decode here rather than the top-of-file use base64::decode.

src/lib.rs Outdated
@@ -1045,6 +1048,19 @@ impl Client {
self.current_url_().map(|(_, u)| u)
}

/// Get a PNG screenshot of the current page.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this "Get a PNG-encoded screenshot of the current page."?

@syepes
Copy link

syepes commented May 30, 2018

+1

@r-darwish
Copy link
Author

Will do. I'll get to fixing it in the upcoming days.

@jonhoo jonhoo mentioned this pull request Jun 4, 2018
@r-darwish
Copy link
Author

Fixed

@jonhoo
Copy link
Owner

jonhoo commented Jun 5, 2018

Perfect, thank you!

@jonhoo jonhoo merged commit 3aca82b into jonhoo:master Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants