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

Refactor WebGL implementation (reopens #5769) #5863

Merged
merged 1 commit into from Apr 27, 2015
Merged

Conversation

@emilio
Copy link
Member

emilio commented Apr 27, 2015

GitHub doesn't allow me to reopen #5769, so I created this.

Sorry about the merge fail, my bad :/

cc/ @jdm @dmarcos


This PR uses customized GL context creation code, right now only working under Linux, so I expect the clearcolor test to fail on other platforms.

It addresses some other problems:

  • Propagates context creation error to the top, returning null if not found.
  • Uses GLContextAttributes, which will allow us to write WebGLContextAttributes easily.
  • Doesn't allow a 2d context and a WebGL context coexist.
  • Panics when resizing the context to larger dimensions (to be fixed soon, but better than blindly allowing it).
    Removes some unused dependencies

Review on Reviewable

@highfive
Copy link

highfive commented Apr 27, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

@highfive
Copy link

highfive commented Apr 27, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Apr 27, 2015

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

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.

@jdm
Copy link
Member

jdm commented Apr 27, 2015

Still needs a rebase, unfortunately.

@emilio
Copy link
Member Author

emilio commented Apr 27, 2015

Yep, I was just doing it.

The previous issue was that I failed to merge some old commits when the first implementation wasn't in master yet.

Should be mergeable now, sorry.

@jdm jdm removed the S-needs-rebase label Apr 27, 2015
@jdm
Copy link
Member

jdm commented Apr 27, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2015

📌 Commit f337d1d has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2015

Testing commit f337d1d with merge 534ec0a...

bors-servo pushed a commit that referenced this pull request Apr 27, 2015
GitHub doesn't allow me to reopen #5769, so I created this.

Sorry about the merge fail, my bad :/

cc/ @jdm @dmarcos

---

This PR uses customized GL context creation code, right now only working under Linux, so I expect the clearcolor test to fail on other platforms.

It addresses some other problems:

* Propagates context creation error to the top, returning null if not found.
* Uses GLContextAttributes, which will allow us to write WebGLContextAttributes easily.
* Doesn't allow a 2d context and a WebGL context coexist.
* Panics when resizing the context to larger dimensions (to be fixed soon, but better than blindly allowing it).
    Removes some unused dependencies

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

bors-servo commented Apr 27, 2015

💔 Test failed - mac2

@emilio
Copy link
Member Author

emilio commented Apr 27, 2015

Rewinded to old cocoa version. I kept winapi version since it was duplicated in master.

@jdm
Copy link
Member

jdm commented Apr 27, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2015

📌 Commit 79a5dae has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2015

Testing commit 79a5dae with merge 9f2ad93...

bors-servo pushed a commit that referenced this pull request Apr 27, 2015
GitHub doesn't allow me to reopen #5769, so I created this.

Sorry about the merge fail, my bad :/

cc/ @jdm @dmarcos

---

This PR uses customized GL context creation code, right now only working under Linux, so I expect the clearcolor test to fail on other platforms.

It addresses some other problems:

* Propagates context creation error to the top, returning null if not found.
* Uses GLContextAttributes, which will allow us to write WebGLContextAttributes easily.
* Doesn't allow a 2d context and a WebGL context coexist.
* Panics when resizing the context to larger dimensions (to be fixed soon, but better than blindly allowing it).
    Removes some unused dependencies

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

bors-servo commented Apr 27, 2015

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

@bors-servo bors-servo merged commit 79a5dae into servo:master Apr 27, 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.