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

Allow to not depend on offscreen_gl_context #1009

Merged
merged 1 commit into from
Mar 28, 2017

Conversation

sotaroikeda
Copy link
Contributor

@sotaroikeda sotaroikeda commented Mar 24, 2017

It is created based on #773.

cc @emilio, @kvark, @glennw


This change is Reviewable

@sotaroikeda
Copy link
Contributor Author

A priority became higher because of Bug 1349967[1]

[1]
https://bugzilla.mozilla.org/show_bug.cgi?id=1349967

@nical
Copy link
Contributor

nical commented Mar 24, 2017

I would prefer it if instead of adding conditional compilation all over the code we would just contain that inside of WebGLContextWrapper (and have a WebGLContextWrapper with mostly empty functions if the webrender feature isn't enabled.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is OK. I do agree with @nical that we should explore the option to cut out less stuff by providing dummies/stubs.

@@ -77,6 +78,19 @@ pub fn should_record_msg(msg: &ApiMsg) -> bool {
&ApiMsg::TickScrollingBounce |
&ApiMsg::WebGLCommand(..) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should just apply the cfg to this item only

@sotaroikeda
Copy link
Contributor Author

Updated a patch as to reduce cfg.

@sotaroikeda
Copy link
Contributor Author

@kvark, can you review the pull request again?

@emilio
Copy link
Member

emilio commented Mar 27, 2017

I guess another alternative for this would be to depend on offscreen_gl_context, but without the x11 feature (which is enabled by default).

This would make it just use the not_implemented platform by default, which would mean that you wouldn't need any cfg at all I think.

@kvark
Copy link
Member

kvark commented Mar 27, 2017

Oh interesting, @emilio !
@sotaroikeda would you want to try that idea out? Should be simpler.

@sotaroikeda
Copy link
Contributor Author

sotaroikeda commented Mar 28, 2017

It was my first proposal, but now I prefer current pull request's approach since it could make crate dependency of gecko simpler. The current approach could remove several crates from /third_party/rust of gecko.

@nical
Copy link
Contributor

nical commented Mar 28, 2017

I like the idea of avoiding unnecessary dependencies in gecko's repository.

@emilio
Copy link
Member

emilio commented Mar 28, 2017

Sounds good, then this looks good to me, does anybody else have any concern with the current approach? If not, feel free to r=me.

@kvark
Copy link
Member

kvark commented Mar 28, 2017

Sounds like a consensus to me :)
@bors-servo r=emilio

@bors-servo
Copy link
Contributor

📌 Commit 78af0cd has been approved by emilio

@bors-servo
Copy link
Contributor

⌛ Testing commit 78af0cd with merge 5c2a9ff...

bors-servo pushed a commit that referenced this pull request Mar 28, 2017
Allow to not depend on offscreen_gl_context

It is created based on #773.

cc @emilio, @kvark, @glennw

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1009)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: emilio
Pushing 5c2a9ff to master...

@bors-servo bors-servo merged commit 78af0cd into servo:master Mar 28, 2017
@sotaroikeda sotaroikeda deleted the feat-webgl branch March 30, 2017 01:26
moz-v2v-gh pushed a commit to mozilla/gecko-projects that referenced this pull request Mar 30, 2017
This is no longer needed after servo/webrender#1009

MozReview-Commit-ID: GEHGRK0TGXY
Manishearth pushed a commit to Manishearth/gecko-dev that referenced this pull request Apr 5, 2017
This is no longer needed after servo/webrender#1009

MozReview-Commit-ID: GEHGRK0TGXY
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
This is no longer needed after servo/webrender#1009

MozReview-Commit-ID: GEHGRK0TGXY

UltraBlame original commit: a5222d76fff2c8a7bda16baca390b795424e2fd2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
This is no longer needed after servo/webrender#1009

MozReview-Commit-ID: GEHGRK0TGXY

UltraBlame original commit: a5222d76fff2c8a7bda16baca390b795424e2fd2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
This is no longer needed after servo/webrender#1009

MozReview-Commit-ID: GEHGRK0TGXY

UltraBlame original commit: a5222d76fff2c8a7bda16baca390b795424e2fd2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants