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

Move code to exit servo after writing a screenshot out of composite_specific_target. #7875

Merged
merged 1 commit into from Oct 8, 2015

Conversation

@jgraham
Copy link
Contributor

jgraham commented Oct 5, 2015

The structure of this function was confusing, so move some parts out into the
caller where they seem like a more natural fit and add documentation of the
functions

Review on Reviewable

@jgraham
Copy link
Contributor Author

jgraham commented Oct 5, 2015

@glennw does something like this seem sensible? I didn't fully test it yet…

@@ -478,7 +478,8 @@ impl<Window: WindowMethods> IOCompositor<Window> {
}

(Msg::CreatePng(reply), ShutdownState::NotShuttingDown) => {
let img = self.composite_specific_target(CompositeTarget::WindowAndPng);
let res = self.composite_specific_target(CompositeTarget::WindowAndPng);
let img = res.ok().and_then(|x| x);

This comment has been minimized.

Copy link
@jdm

jdm Oct 5, 2015

Member

The and_then seems redundant.

This comment has been minimized.

Copy link
@jgraham

jgraham Oct 5, 2015

Author Contributor

It unwraps a level of Option i.e. Some(Some(img)) becomes Some(img)

This comment has been minimized.

Copy link
@jgraham

jgraham Oct 5, 2015

Author Contributor

But I guess I'm looking for unwrap_or. The Result and Option APIs are huge :(

@glennw
Copy link
Member

glennw commented Oct 7, 2015

@jgraham It looks reasonable to me - did you want to do any more testing / changes on it, or is it ready?

…pecific_target.

The structure of this function was confusing, so move some parts out into the
caller where they seem like a more natural fit and add documentation of the
functions
@jgraham jgraham force-pushed the jgraham:composite_no_exit branch from 923ddf9 to 64b74f5 Oct 7, 2015
@jgraham
Copy link
Contributor Author

jgraham commented Oct 7, 2015

I think it's ready for review.

@glennw
Copy link
Member

glennw commented Oct 7, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

📌 Commit 64b74f5 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

Testing commit 64b74f5 with merge f917037...

bors-servo pushed a commit that referenced this pull request Oct 7, 2015
Move code to exit servo after writing a screenshot out of composite_specific_target.

The structure of this function was confusing, so move some parts out into the
caller where they seem like a more natural fit and add documentation of the
functions

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

bors-servo commented Oct 7, 2015

💔 Test failed - mac-dev-ref-unit

@jdm
Copy link
Member

jdm commented Oct 7, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

Previous build results for android, gonk, linux-dev, linux-rel, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only mac-dev-ref-unit...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

💔 Test failed - mac-dev-ref-unit

@jdm
Copy link
Member

jdm commented Oct 7, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

Previous build results for android, gonk, linux-dev, linux-rel, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only mac-dev-ref-unit...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

💔 Test failed - mac-dev-ref-unit

@jdm
Copy link
Member

jdm commented Oct 7, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

Previous build results for android, gonk, linux-dev, linux-rel, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only mac-dev-ref-unit...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

💔 Test failed - mac-dev-ref-unit

@jgraham
Copy link
Contributor Author

jgraham commented Oct 7, 2015

Well, it's the same test that fails each time…

@jdm
Copy link
Member

jdm commented Oct 7, 2015

@bors-servo: retry

  • #7730
    It's ok, it fails over and over in other PRs too.
@jgraham
Copy link
Contributor Author

jgraham commented Oct 7, 2015

Ah, the other kind of OK

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2015

Testing commit 64b74f5 with merge e7f73fd...

bors-servo pushed a commit that referenced this pull request Oct 7, 2015
Move code to exit servo after writing a screenshot out of composite_specific_target.

The structure of this function was confusing, so move some parts out into the
caller where they seem like a more natural fit and add documentation of the
functions

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

bors-servo commented Oct 8, 2015

@bors-servo bors-servo merged commit 64b74f5 into servo:master Oct 8, 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

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