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

Support WebDriver takeScreenshot command. #5884

Merged
merged 1 commit into from May 7, 2015

Conversation

@jgraham
Copy link
Contributor

jgraham commented Apr 28, 2015

This adds support for compositing to a PNG without actually quiting
the browser.

Cargo bits need to be updated after the upstream changes to rust-png land.

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Apr 28, 2015

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

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.

@bors-servo
Copy link
Contributor

bors-servo commented Apr 29, 2015

The latest upstream changes (presumably #5895) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented May 1, 2015

Reviewed files:

  • components/compositing/compositor.rs @ r1
  • components/compositing/compositor_task.rs @ r1
  • components/compositing/constellation.rs @ r1
  • components/compositing/headless.rs @ r1
  • components/msg/Cargo.toml @ r1
  • components/msg/constellation_msg.rs @ r1
  • components/msg/lib.rs @ r1
  • components/script/script_task.rs @ r1
  • components/servo/Cargo.lock @ r1
  • components/webdriver_server/Cargo.toml @ r1
  • components/webdriver_server/lib.rs @ r1

components/compositing/compositor.rs, line 191 [r1] (raw file):
This option deserves documentation, so we should probably document the others and the overall type as well.


components/compositing/compositor.rs, line 1148 [r1] (raw file):
Would it make more sense to take an Option<Sender<png::Image>> argument instead of having all of these return None branches?


components/compositing/compositor.rs, line 1219 [r1] (raw file):
This can just be a free function, and I think that will be clearer too.


components/compositing/compositor_task.rs, line 221 [r1] (raw file):
This is very zen.


components/msg/constellation_msg.rs, line 238 [r1] (raw file):
/// for docs (and the one above


components/webdriver_server/lib.rs, line 11 [r1] (raw file):
I believe this is gone in the Rust upgrade that is coming down the pipes. I'd like to find a different solution.


components/webdriver_server/lib.rs, line 47 [r1] (raw file):
std:🧵:sleep_ms?


components/webdriver_server/lib.rs, line 161 [r1] (raw file):
for _ in &[0..iterations] {



Comments from the review on Reviewable.io

@jdm jdm removed the S-awaiting-review label May 1, 2015
@jdm
Copy link
Member

jdm commented May 1, 2015

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


Comments from the review on Reviewable.io

@jgraham
Copy link
Contributor Author

jgraham commented May 1, 2015

components/compositing/compositor.rs, line 1148 [r1] (raw file):
I sort of felt like this was a better separation of concerns because it doesn't bake sending the PNG over a channel into this function. But I can do it the other way if you prefer.


components/webdriver_server/lib.rs, line 11 [r1] (raw file):
Yeah it is; after the rustup I can use std:🧵:sleep_ms as you point out below. But afaict from the docs, that doesn't exist in the current version of rustc. Although, perhaps I should verify that rather than trusting them.



Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented May 1, 2015

Reviewable is confusing me but I have marked those two as addressed, based on your feedback.


Comments from the review on Reviewable.io

@jgraham jgraham force-pushed the jgraham:webdriver_screenshot branch from 0bb69db to 63f7485 May 5, 2015
@jdm
Copy link
Member

jdm commented May 6, 2015

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


Reviewed files:

  • components/compositing/compositor.rs @ r2
  • components/compositing/compositor_task.rs @ r2
  • components/compositing/constellation.rs @ r2
  • components/compositing/headless.rs @ r2
  • components/msg/constellation_msg.rs @ r2
  • components/script/script_task.rs @ r2
  • components/servo/Cargo.lock @ r2
  • components/webdriver_server/lib.rs @ r2

components/webdriver_server/lib.rs, line 166 [r2] (raw file):
interval?


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

The latest upstream changes (presumably #5753) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm jdm added the S-needs-rebase label May 6, 2015
@jgraham jgraham force-pushed the jgraham:webdriver_screenshot branch from 93acf65 to cc43efc May 6, 2015
@jdm
Copy link
Member

jdm commented May 6, 2015

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


Reviewed files:

  • components/compositing/compositor.rs @ r3
  • components/compositing/compositor_task.rs @ r3
  • components/compositing/constellation.rs @ r3
  • components/msg/constellation_msg.rs @ r3
  • components/script/script_task.rs @ r3
  • components/servo/Cargo.lock @ r3
  • components/webdriver_server/Cargo.toml @ r3
  • components/webdriver_server/lib.rs @ r3

Comments from the review on Reviewable.io

@jdm jdm added the S-needs-squash label May 6, 2015
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

The latest upstream changes (presumably #5361) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm jdm added the S-needs-rebase label May 6, 2015
@jgraham jgraham force-pushed the jgraham:webdriver_screenshot branch from cc43efc to 570aa4d May 6, 2015
@jdm
Copy link
Member

jdm commented May 6, 2015

@bors-servo: r+
-S-awaiting-review -S-needs-rebase -S-needs-squash


Reviewed files:

  • components/compositing/compositor.rs @ r4
  • components/compositing/compositor_task.rs @ r4
  • components/compositing/constellation.rs @ r4
  • components/compositing/headless.rs @ r4
  • components/msg/constellation_msg.rs @ r4
  • components/msg/lib.rs @ r4
  • components/script/script_task.rs @ r4
  • components/servo/Cargo.lock @ r4
  • components/webdriver_server/lib.rs @ r4

Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2015

📌 Commit 570aa4d has been approved by jdm

bors-servo pushed a commit that referenced this pull request May 7, 2015
This adds support for compositing to a PNG without actually quiting
the browser.

Cargo bits need to be updated after the upstream changes to rust-png land.

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

bors-servo commented May 7, 2015

Testing commit 570aa4d with merge 3989bb4...

@bors-servo
Copy link
Contributor

bors-servo commented May 7, 2015

💔 Test failed - mac1

@bors-servo
Copy link
Contributor

bors-servo commented May 7, 2015

The latest upstream changes (presumably #5968) made this pull request unmergeable. Please resolve the merge conflicts.

@jgraham jgraham force-pushed the jgraham:webdriver_screenshot branch from 570aa4d to 967edc3 May 7, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented May 7, 2015

@bors-servo: r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 7, 2015

📌 Commit 967edc3 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 7, 2015

Testing commit 967edc3 with merge 74ead3f...

bors-servo pushed a commit that referenced this pull request May 7, 2015
This adds support for compositing to a PNG without actually quiting
the browser.

Cargo bits need to be updated after the upstream changes to rust-png land.

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

bors-servo commented May 7, 2015

💔 Test failed - gonk

This adds support for compositing to a PNG without actually quiting
the browser.
@jgraham jgraham force-pushed the jgraham:webdriver_screenshot branch from 967edc3 to 5311d9a May 7, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented May 7, 2015

@bors-servo: r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 7, 2015

📌 Commit 5311d9a has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented May 7, 2015

Testing commit 5311d9a with merge 63ba1cb...

bors-servo pushed a commit that referenced this pull request May 7, 2015
This adds support for compositing to a PNG without actually quiting
the browser.

Cargo bits need to be updated after the upstream changes to rust-png land.

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

bors-servo commented May 7, 2015

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

@bors-servo bors-servo merged commit 5311d9a into servo:master May 7, 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

6 participants
You can’t perform that action at this time.