Skip to content

/ servo

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

Glutin update #7096

Merged
merged 1 commit into from Aug 22, 2015
Merged

Glutin update #7096

merged 1 commit into from Aug 22, 2015
+253 −22

Conversation

@paulrouget
Copy link
Contributor

paulrouget commented Aug 8, 2015

Review on Reviewable
@paulrouget paulrouget force-pushed the paulrouget:glutin-update branch from 2af9fab to 3b9d23c Aug 8, 2015
This was referenced Aug 8, 2015
Closed
Closed
@paulrouget paulrouget force-pushed the paulrouget:glutin-update branch 2 times, most recently from 78356d8 to fbcf7fa Aug 11, 2015
ports/glutin/window.rs Outdated
@@ -98,7 +98,10 @@ impl Window {
.with_parent(parent)
.build()
.unwrap();
unsafe { glutin_window.make_current() };
match unsafe { glutin_window.make_current() } {

This comment has been minimized.

Sign in to view
@glennw

glennw Aug 14, 2015

Member

This could be ...make_current().expect("Failed to make context current!");

This comment has been minimized.

Sign in to view
@paulrouget

paulrouget Aug 17, 2015

Author Contributor

I can't use expect:

/Users/paul/github/servo/ports/glutin/window.rs:102:47: 102:88 error: use of unstable library feature 'result_expect': newly introduced (see issue #27277)
/Users/paul/github/servo/ports/glutin/window.rs:102         unsafe { glutin_window.make_current().expect("Failed to make context current!") }
                                                                                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/paul/github/servo/ports/glutin/window.rs:102:47: 102:88 help: add #![feature(result_expect)] to the crate attributes to enable

This comment has been minimized.

Sign in to view
@nox

nox Aug 18, 2015

Member

Just enable the feature.
ports/glutin/window.rs Outdated
@@ -519,7 +522,7 @@ impl WindowMethods for Window {
}

fn present(&self) {
self.window.swap_buffers()
self.window.swap_buffers().is_ok();

This comment has been minimized.

Sign in to view
@glennw

glennw Aug 14, 2015

Member

I think just .ok() is generally preferred for this.

This comment has been minimized.

Sign in to view
@nox

nox Aug 18, 2015

Member

The intent of that code isn't obvious, shouldn't we instead do "let _ = …"?

This comment has been minimized.

Sign in to view
@nox

nox Aug 18, 2015

Member 
13:50 <Ms2ger> I prefer .unwrap()
13:50 <nox> Ms2ger: Yeah me too but I wasn't sure if that call is supposed to fail silently or not.
@paulrouget paulrouget force-pushed the paulrouget:glutin-update branch 3 times, most recently from 755d3a2 to 84b66f5 Aug 17, 2015
@paulrouget
Copy link
Contributor Author

paulrouget commented Aug 19, 2015

Addressed Nox' comments.
@nox
Copy link
Member

nox commented Aug 20, 2015

@bors-servo r=glennw r+

Thanks for your contribution.

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io
@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2015

📌 Commit 84b66f5 has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2015

📌 Commit 84b66f5 has been approved by glennw
@nox nox self-assigned this Aug 20, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Aug 20, 2015

⌛ Testing commit 84b66f5 with merge 441a054...
bors-servo pushed a commit that referenced this pull request Aug 20, 2015
bors-servo
Auto merge of #7096 - paulrouget:glutin-update, r=glennw
441a054 
Glutin update

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

bors-servo commented Aug 20, 2015

💔 Test failed - mac2
@paulrouget
Copy link
Contributor Author

paulrouget commented Aug 20, 2015

This still requires an update of Cargo.toml, and it depends on servo/glutin#44
@paulrouget paulrouget force-pushed the paulrouget:glutin-update branch from 84b66f5 to 8f2e1fc Aug 22, 2015
@paulrouget
Copy link
Contributor Author

paulrouget commented Aug 22, 2015

The glutin servo branch has been updated.

I ran cargo update -p glutin, and ended up with this new Cargo.lock. Does it look right?
@nox
Copy link
Member

nox commented Aug 22, 2015

@paulrouget You must use ./mach update-cargo, AFAIK.
@glennw
Copy link
Member

glennw commented Aug 22, 2015

@paulrouget Yep, if you run ./mach update-cargo -p glutin like @nox mentioned, this will update the cargo.lock files for all ports. I have only quickly scanned the changed cargo.lock file, but it looks about right - lots of new dependencies from the upstream glutin :)
@paulrouget
Update glutin window port after glutin update
Loading status checks…
0209bf1
@paulrouget paulrouget force-pushed the paulrouget:glutin-update branch from 8f2e1fc to 0209bf1 Aug 22, 2015
@paulrouget
Copy link
Contributor Author

paulrouget commented Aug 22, 2015

Updated with mach update-cargo -p glutin.
@SimonSapin
Copy link
Member

SimonSapin commented Aug 22, 2015

@bors-servo r=glennw
@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2015

📌 Commit 0209bf1 has been approved by glennw
@bors-servo
Copy link
Contributor

bors-servo commented Aug 22, 2015

⌛ Testing commit 0209bf1 with merge 12c44d6...
bors-servo pushed a commit that referenced this pull request Aug 22, 2015
bors-servo
Auto merge of #7096 - paulrouget:glutin-update, r=glennw
Loading status checks…
12c44d6 
Glutin update

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

bors-servo commented Aug 22, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2, mac3
@bors-servo bors-servo merged commit 0209bf1 into servo:master Aug 22, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Aug 22, 2015
Merged
@mbrubeck mbrubeck mentioned this pull request Aug 25, 2015
Merged
bors-servo pushed a commit that referenced this pull request Aug 25, 2015
bors-servo
Auto merge of #7368 - mbrubeck:closed, r=jdm
ea04b01 
Quit when the glutin window closes

Fixes a regression from #7096.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7368)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this pull request Aug 26, 2015
bors-servo
Auto merge of #7368 - mbrubeck:closed, r=jdm
Loading status checks…
5f9097c 
Quit when the glutin window closes

Fixes a regression from #7096.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7368)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@paulrouget @nox @bors-servo @glennw @SimonSapin @highfive
You can’t perform that action at this time.