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

webgl: Support texImage2D with a canvas as an argument #7682

Merged
merged 5 commits into from Sep 25, 2015

Conversation

@emilio
Copy link
Member

emilio commented Sep 19, 2015

This involved some refactoring of the 2d context code, which lead to some more test passed there.

Review on Reviewable

@emilio emilio force-pushed the emilio:webgl-texture2d branch from 3d03720 to 0380547 Sep 19, 2015
@emilio
Copy link
Member Author

emilio commented Sep 19, 2015

r? @jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 20, 2015

The latest upstream changes (presumably #7601) made this pull request unmergeable. Please resolve the merge conflicts.

@emilio emilio force-pushed the emilio:webgl-texture2d branch from 0380547 to 7301f43 Sep 20, 2015
@emilio
Copy link
Member Author

emilio commented Sep 20, 2015

Unbitrotted :P

@emilio
Copy link
Member Author

emilio commented Sep 22, 2015

Wow highfive really hates my rebases, it never removes the label...

cc/ @jdm

@jdm jdm removed the S-needs-rebase label Sep 22, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2015

The latest upstream changes (presumably #7710) made this pull request unmergeable. Please resolve the merge conflicts.

@emilio emilio force-pushed the emilio:webgl-texture2d branch from 7301f43 to 9f58ebf Sep 24, 2015
@@ -150,6 +150,17 @@ impl LayoutHTMLCanvasElementHelpers for LayoutJS<HTMLCanvasElement> {


impl HTMLCanvasElement {
pub fn ipc_renderer(&self) -> Option<IpcSender<CanvasMsg>> {
if let Some(context) = self.context.get() {

This comment has been minimized.

@pcwalton

pcwalton Sep 25, 2015

Contributor

Could use something like

self.context.map(|context| {
    match context {
        CanvasContext::Context2d(context) => context.root().r().ipc_renderer(),
        CanvasContext::WebGL(context) => context.root().r().ipc_randerer(),
    }
})

This comment has been minimized.

@emilio

emilio Sep 25, 2015

Author Member

Yes, sure :)

Refactored it also in LayoutHTMLCanvasElementHelpers

@jdm jdm removed the S-needs-rebase label Sep 25, 2015
@emilio emilio force-pushed the emilio:webgl-texture2d branch from 9f58ebf to 7442aeb Sep 25, 2015
Prefer `Option::map` instead.
@emilio emilio force-pushed the emilio:webgl-texture2d branch from 7442aeb to d12dbf9 Sep 25, 2015
@jdm
Copy link
Member

jdm commented Sep 25, 2015

-S-awaiting-review +S-needs-code-changes


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


components/script/dom/canvasrenderingcontext2d.rs, line 962 [r1] (raw file):
Why don't we move this into fetch_all_data instead?


Comments from the review on Reviewable.io

@emilio emilio force-pushed the emilio:webgl-texture2d branch from 4ce9141 to d12dbf9 Sep 25, 2015
@emilio
Copy link
Member Author

emilio commented Sep 25, 2015

Review status: 8 of 9 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


components/script/dom/canvasrenderingcontext2d.rs, line 962 [r1] (raw file):
Because that behaviour is specified for the 2d context API, but not explicitly for WebGL...

Both firefox and chrome seem to return a white bitmap when the canvas has no context, but it's unintuitive something like:

gl.texImage2d(..., canvas_without_context);
canvas_without_context.getContext('webgl'); // <- fails

In fact both allow it and the webgl context creation succeeds, I've done the same.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Sep 25, 2015

-S-awaiting-review +S-needs-code-changes


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


components/script/dom/canvasrenderingcontext2d.rs, line 962 [r1] (raw file):
Let's add tests?


Comments from the review on Reviewable.io

@emilio
Copy link
Member Author

emilio commented Sep 25, 2015

Review status: 8 of 11 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


components/script/dom/canvasrenderingcontext2d.rs, line 962 [r1] (raw file):
Done!


Comments from the review on Reviewable.io

@emilio emilio force-pushed the emilio:webgl-texture2d branch from f11199c to 17a9472 Sep 25, 2015
@jdm
Copy link
Member

jdm commented Sep 25, 2015

@bors-servo: r+
Thanks!


Reviewed 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2015

📌 Commit 17a9472 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2015

Testing commit 17a9472 with merge 0f8c1b4...

bors-servo pushed a commit that referenced this pull request Sep 25, 2015
webgl: Support texImage2D with a canvas as an argument

This involved some refactoring of the 2d context code, which lead to some more test passed there.

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

bors-servo commented Sep 25, 2015

@bors-servo bors-servo merged commit 17a9472 into servo:master Sep 25, 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.