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

Split paint task messages from ScriptMsg #8598

Merged
merged 1 commit into from Nov 26, 2015
Merged

Conversation

@g-k
Copy link
Contributor

g-k commented Nov 19, 2015

Refs: #8592

Review on Reviewable

@jdm
Copy link
Member

jdm commented Nov 19, 2015

@KiChjang Want to take a swing at reviewing this?

@KiChjang
Copy link
Member

KiChjang commented Nov 19, 2015

Sure!

@KiChjang
Copy link
Member

KiChjang commented Nov 19, 2015

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.


components/compositing/constellation.rs, line 628 [r1] (raw file):
We might want to have some ways of distinguishing between Failure messages that are fired from the script. Two ways I can think of: pass an additional string parameter to handle_failure_msg or move the debug!macro up here and to the branch handling ScriptMsg::Failure above.


Comments from the review on Reviewable.io

@KiChjang
Copy link
Member

KiChjang commented Nov 19, 2015

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Nov 19, 2015

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

@KiChjang
Copy link
Member

KiChjang commented Nov 22, 2015

I see that you have made a new commit to address the previous issues @g-k , but you'll need to rebase this branch onto master before it can be merged, and also squash your commits. See https://github.com/servo/servo/wiki/Github-&-Critic-PR-handling-101 for a detailed explanation (ignore the part about critic, it's an old review system which we currently do not use anymore).

Essentially what needs to happen is:

git remote add mozilla git@github.com:servo/servo.git
git fetch mozilla
git rebase mozilla/master

This rebases your branch onto servo's master. Now, onto squashing commits:

git rebase -i HEAD~2

Delete the second line's "pick" and replace it with "f". Finally force-push your branch:

git push -f
@g-k g-k force-pushed the g-k:split-scriptmsg branch from 960cb94 to be00e4b Nov 22, 2015
@g-k
Copy link
Contributor Author

g-k commented Nov 22, 2015

Thanks @KiChjang! I rebased and autosquashed but have a build failing with

error: the trait `serde::ser::Serialize` is not implemented for the type `std::sync::mpsc::Sender<msg::constellation_msg::PaintMsg>`

and

error: mismatched types:
 expected `msg::constellation_msg::ConstellationChan<msg::constellation_msg::PaintMsg>`,
    found `std::sync::mpsc::Sender<msg::constellation_msg::PaintMsg>`
(expected struct `msg::constellation_msg::ConstellationChan`,
    found struct `std::sync::mpsc::Sender`) [E0308]

errors. Should painter_chan have type msg::constellation_msg::ConstellationChan<msg::constellation_msg::PaintMsg> or std::sync::mpsc::Sender<msg::constellation_msg::PaintMsg>?

@g-k g-k force-pushed the g-k:split-scriptmsg branch 2 times, most recently from 462bf6a to 022d6d3 Nov 22, 2015
@g-k
Copy link
Contributor Author

g-k commented Nov 22, 2015

Thanks @eefriedman! don't know how I missed that.

@g-k
Copy link
Contributor Author

g-k commented Nov 25, 2015

Rebased.

@jdm jdm removed the S-needs-rebase label Nov 25, 2015
@KiChjang
Copy link
Member

KiChjang commented Nov 25, 2015

Almost there! Once you address the comments, ask @jdm to r+.


Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


components/compositing/constellation.rs, line 285 [r2] (raw file):
nit: Do we still need this comment here?


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Nov 25, 2015

@bors-servo: delegate=KiChjang

@bors-servo
Copy link
Contributor

bors-servo commented Nov 25, 2015

✌️ @KiChjang can now approve this pull request

Refs: #8592
@g-k g-k force-pushed the g-k:split-scriptmsg branch from 022d6d3 to 7668fd0 Nov 25, 2015
@g-k
Copy link
Contributor Author

g-k commented Nov 25, 2015

Review status: 3 of 4 files reviewed at latest revision, 1 unresolved discussion.


components/compositing/constellation.rs, line 285 [r2] (raw file):
I don't think so. Removed!


Comments from the review on Reviewable.io

@KiChjang
Copy link
Member

KiChjang commented Nov 25, 2015

@bors-servo r+


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Nov 25, 2015

📌 Commit 7668fd0 has been approved by KiChjang

@bors-servo
Copy link
Contributor

bors-servo commented Nov 25, 2015

Testing commit 7668fd0 with merge 5e7306b...

bors-servo added a commit that referenced this pull request Nov 25, 2015
Split paint task messages from ScriptMsg

Refs: #8592

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

bors-servo commented Nov 25, 2015

💔 Test failed - linux-rel

@jdm
Copy link
Member

jdm commented Nov 25, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 25, 2015

Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

bors-servo commented Nov 26, 2015

@bors-servo bors-servo merged commit 7668fd0 into servo:master Nov 26, 2015
3 checks passed
3 checks passed
code-review/reviewable Review complete: all files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@KiChjang
Copy link
Member

KiChjang commented Nov 26, 2015

\o/

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

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