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 WebGLPaintTask to prevent creating extra IPC channels - Issu… #9234

Closed
wants to merge 2 commits into from

Conversation

@coder543
Copy link
Contributor

coder543 commented Jan 10, 2016

closes #9228

and this time I have a separate branch.

Review on Reviewable

@highfive
Copy link

highfive commented Jan 10, 2016

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

@KiChjang
Copy link
Member

KiChjang commented Jan 10, 2016

@KiChjang
Copy link
Member

KiChjang commented Jan 10, 2016

Fails tidy:

./components/canvas/webgl_paint_task.rs:233: trailing whitespace

NB: You can run ./mach test-tidy locally to check whether your source files are conforming to tidy rules.

@bors-servo
Copy link
Contributor

bors-servo commented Jan 10, 2016

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

@nox
Copy link
Member

nox commented Jan 10, 2016

I'm pretty sure @ecoal95 meant to initialise the IPC channel in the last .map(|_| …) call in that function. You are still creating the channel for nothing if the context couldn't be initialised.

@emilio
Copy link
Member

emilio commented Jan 10, 2016

Yep, @nox is correct, you still always create the IPC channel.

It should be done if the recv() receives an Ok value, that is,
in the last .map call.

@nox nox removed the S-awaiting-review label Jan 10, 2016
@emilio
Copy link
Member

emilio commented Jan 10, 2016

@coder543 Was this closed on purpose? You can rebase without creating a new PR, I'm happy to help you to do that if you want :P

@coder543
Copy link
Contributor Author

coder543 commented Jan 10, 2016

Yeah, I was up to the 3 or 4 commit mark since a commit last night renamed all Tasks to Threads, and after rebasing I realized I needed to make another commit just to handle some whitespace issues. Things were getting messy. I know there's a way to squash commits, but I've never done that before, so I just cloned master again and pasted in my changes. Now there is only one commit in a new pull request... sorry about the number of PRs this has caused.

@emilio
Copy link
Member

emilio commented Jan 10, 2016

@coder543 No problem! :-)

@nox
Copy link
Member

nox commented Jan 11, 2016

@coder543 Feel free to come on IRC, we can help you with Git.

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.

6 participants
You can’t perform that action at this time.