Replace use of gleam in webgl with sparkle. #24176

jdm · 2019-09-10T18:58:54Z

Rely on https://github.com/servo/sparkle for the GL bindings required to implement Servo's WebGL stack. This does not touch the GL usage in the compositor or webrender, which continue to rely on https://github.com/servo/gleam. This means that any breaking changes to the public GL bindings interface (such as required by #24111) only require updating rust-offscreen-rendering-context and rust-webvr, rather than many other crates that are out of our direct control.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
There are tests for these changes

This change is

highfive · 2019-09-10T18:58:59Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/webgl_extensions/extensions.rs, components/script/Cargo.toml
@KiChjang: components/script/dom/webgl_extensions/extensions.rs, components/script/Cargo.toml

highfive · 2019-09-10T18:59:01Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2019-09-10T18:59:34Z

@bors-servo try

Replace use of gleam in webgl with sparkle. Rely on https://github.com/servo/sparkle for the GL bindings required to implement Servo's WebGL stack. This does not touch the GL usage in the compositor or webrender, which continue to rely on https://github.com/servo/gleam. This means that any breaking changes to the public GL bindings interface (such as required by #24111) only require updating rust-offscreen-rendering-context and rust-webvr, rather than many other crates that are out of our direct control. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24176)

bors-servo · 2019-09-10T18:59:39Z

⌛ Trying commit 0fb0c00 with merge c6b3829...

bors-servo · 2019-09-10T19:27:33Z

💔 Test failed - linux-rel-css

jdm · 2019-09-10T19:51:38Z

@bors-servo try

bors-servo · 2019-09-10T19:51:43Z

⌛ Trying commit 963c7c4 with merge 52877ef...

Replace use of gleam in webgl with sparkle. Rely on https://github.com/servo/sparkle for the GL bindings required to implement Servo's WebGL stack. This does not touch the GL usage in the compositor or webrender, which continue to rely on https://github.com/servo/gleam. This means that any breaking changes to the public GL bindings interface (such as required by #24111) only require updating rust-offscreen-rendering-context and rust-webvr, rather than many other crates that are out of our direct control. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24176)

bors-servo · 2019-09-10T19:52:06Z

💔 Test failed - linux-rel-css

jdm · 2019-09-10T20:18:35Z

@bors-servo try

bors-servo · 2019-09-10T20:18:40Z

⌛ Trying commit eeb5d34 with merge 25ac322...

Replace use of gleam in webgl with sparkle. Rely on https://github.com/servo/sparkle for the GL bindings required to implement Servo's WebGL stack. This does not touch the GL usage in the compositor or webrender, which continue to rely on https://github.com/servo/gleam. This means that any breaking changes to the public GL bindings interface (such as required by #24111) only require updating rust-offscreen-rendering-context and rust-webvr, rather than many other crates that are out of our direct control. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24176)

asajeffrey

Looks good so far! You can r=me once the dependencies have merged.

asajeffrey · 2019-09-10T20:22:14Z

components/servo/lib.rs

+            let gl_type = match window.gl().get_type() {
+                gl::GlType::Gl => sparkle::gl::GlType::Gl,
+                gl::GlType::Gles => sparkle::gl::GlType::Gles,
+            };


Can we re-export GlType to avoid this type conversion?

Re-export where? Have gleam depend on sparkle? Have sparkle depend on gleam? Neither solution sounds like an improvement to me.

Ah, we're getting the GlType from glutin, oh that makes sense.

bors-servo · 2019-09-10T20:47:19Z

💔 Test failed - linux-rel-css

CYBAI · 2019-09-12T09:57:39Z

@bors-servo retry

Intermittent reftest failures due to font selection choices #23290

bors-servo · 2019-09-12T10:01:38Z

⌛ Testing commit 1cefae7 with merge ea24d14...

Replace use of gleam in webgl with sparkle. Rely on https://github.com/servo/sparkle for the GL bindings required to implement Servo's WebGL stack. This does not touch the GL usage in the compositor or webrender, which continue to rely on https://github.com/servo/gleam. This means that any breaking changes to the public GL bindings interface (such as required by #24111) only require updating rust-offscreen-rendering-context and rust-webvr, rather than many other crates that are out of our direct control. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24176)

bors-servo · 2019-09-12T12:03:28Z

💔 Test failed - status-taskcluster

jdm · 2019-09-12T12:41:51Z

@bors-servo retry

cygwin heap exhaustion

Replace use of gleam in webgl with sparkle. Rely on https://github.com/servo/sparkle for the GL bindings required to implement Servo's WebGL stack. This does not touch the GL usage in the compositor or webrender, which continue to rely on https://github.com/servo/gleam. This means that any breaking changes to the public GL bindings interface (such as required by #24111) only require updating rust-offscreen-rendering-context and rust-webvr, rather than many other crates that are out of our direct control. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24176)

bors-servo · 2019-09-12T14:40:34Z

⌛ Testing commit 1cefae7 with merge 897d030...

bors-servo · 2019-09-12T17:22:10Z

💔 Test failed - status-taskcluster

jdm · 2019-09-12T17:47:55Z

@bors-servo retry
#23290

bors-servo · 2019-09-12T20:08:56Z

💣 Failed to start rebuilding: Unknown error

bors-servo · 2019-09-12T20:09:00Z

⌛ Testing commit 1cefae7 with merge 89d0b02...

Replace use of gleam in webgl with sparkle. Rely on https://github.com/servo/sparkle for the GL bindings required to implement Servo's WebGL stack. This does not touch the GL usage in the compositor or webrender, which continue to rely on https://github.com/servo/gleam. This means that any breaking changes to the public GL bindings interface (such as required by #24111) only require updating rust-offscreen-rendering-context and rust-webvr, rather than many other crates that are out of our direct control. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24176)

bors-servo · 2019-09-12T20:57:15Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: asajeffrey
Pushing 89d0b02 to master...

kvark · 2019-09-18T12:50:29Z

Trying to understand the reasoning here. Is sparkle essentially a fork of gleam that will have more breaking changes? why couldn't you just manage it as a fork of gleam then?

jdm · 2019-09-18T12:58:37Z

Sparkle doesn't use traits, so changes to expose new methods don't cause semver major API changes. Additionally, sparkle only exposes methods for APIs thare are actually available for GL and GLES.

highfive assigned asajeffrey Sep 10, 2019

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 10, 2019

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 10, 2019

jdm force-pushed the no-gleam branch from 0fb0c00 to bc29930 Compare September 10, 2019 19:46

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 10, 2019

jdm force-pushed the no-gleam branch from bc29930 to 963c7c4 Compare September 10, 2019 19:50

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 10, 2019

jdm force-pushed the no-gleam branch from 963c7c4 to eeb5d34 Compare September 10, 2019 20:18

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 10, 2019

jdm added the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Sep 10, 2019

jdm mentioned this pull request Sep 10, 2019

webgl: Support vertex array objects on macOS. #24111

Merged

3 tasks

asajeffrey reviewed Sep 10, 2019

View reviewed changes

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 10, 2019

jdm force-pushed the no-gleam branch from eeb5d34 to f6ef89b Compare September 10, 2019 20:55

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 10, 2019

Replace use of gleam in webgl with sparkle.

1cefae7

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Sep 12, 2019

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 12, 2019

This was referenced Sep 12, 2019

Add missing Sync-related functions servo/gleam#204

Closed

Add missing sampler functions servo/gleam#203

Closed

Add missing is_query function servo/gleam#202

Closed

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Sep 12, 2019

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 12, 2019

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Sep 12, 2019

bors-servo merged commit 1cefae7 into servo:master Sep 12, 2019

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace use of gleam in webgl with sparkle. #24176

Replace use of gleam in webgl with sparkle. #24176

jdm commented Sep 10, 2019 •

edited by SimonSapin

highfive commented Sep 10, 2019

highfive commented Sep 10, 2019

jdm commented Sep 10, 2019

bors-servo commented Sep 10, 2019

bors-servo commented Sep 10, 2019

jdm commented Sep 10, 2019

bors-servo commented Sep 10, 2019

bors-servo commented Sep 10, 2019

jdm commented Sep 10, 2019

bors-servo commented Sep 10, 2019

asajeffrey left a comment

asajeffrey Sep 10, 2019

jdm Sep 10, 2019

asajeffrey Sep 10, 2019

bors-servo commented Sep 10, 2019

CYBAI commented Sep 12, 2019

bors-servo commented Sep 12, 2019

bors-servo commented Sep 12, 2019

jdm commented Sep 12, 2019

bors-servo commented Sep 12, 2019

bors-servo commented Sep 12, 2019

jdm commented Sep 12, 2019

bors-servo commented Sep 12, 2019

bors-servo commented Sep 12, 2019

bors-servo commented Sep 12, 2019

kvark commented Sep 18, 2019

jdm commented Sep 18, 2019

Replace use of gleam in webgl with sparkle. #24176

Replace use of gleam in webgl with sparkle. #24176

Conversation

jdm commented Sep 10, 2019 • edited by SimonSapin

highfive commented Sep 10, 2019

highfive commented Sep 10, 2019

jdm commented Sep 10, 2019

bors-servo commented Sep 10, 2019

bors-servo commented Sep 10, 2019

jdm commented Sep 10, 2019

bors-servo commented Sep 10, 2019

bors-servo commented Sep 10, 2019

jdm commented Sep 10, 2019

bors-servo commented Sep 10, 2019

asajeffrey left a comment

Choose a reason for hiding this comment

asajeffrey Sep 10, 2019

Choose a reason for hiding this comment

jdm Sep 10, 2019

Choose a reason for hiding this comment

asajeffrey Sep 10, 2019

Choose a reason for hiding this comment

bors-servo commented Sep 10, 2019

CYBAI commented Sep 12, 2019

bors-servo commented Sep 12, 2019

bors-servo commented Sep 12, 2019

jdm commented Sep 12, 2019

bors-servo commented Sep 12, 2019

bors-servo commented Sep 12, 2019

jdm commented Sep 12, 2019

bors-servo commented Sep 12, 2019

bors-servo commented Sep 12, 2019

bors-servo commented Sep 12, 2019

kvark commented Sep 18, 2019

jdm commented Sep 18, 2019

jdm commented Sep 10, 2019 •

edited by SimonSapin