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

Merge generic funs to share them across all bindings (fixes #2684) #6223

Merged
merged 1 commit into from Jun 24, 2015

Conversation

@nox
Copy link
Member

nox commented May 30, 2015

Review on Reviewable

@highfive
Copy link

highfive commented May 30, 2015

warning Warning warning

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

hoppipolla-critic-bot commented May 30, 2015

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

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.

@nox
Copy link
Member Author

nox commented May 30, 2015

I am pretty sure GetProtoObject() can be shared too.

@jdm
Copy link
Member

jdm commented May 30, 2015

I'd like to hold off on merging these changes until after #6150.

@nox
Copy link
Member Author

nox commented May 30, 2015

Why? Don't you want to review again 175 files after I bitrot them? Agreed. :)

@metajack
Copy link
Contributor

metajack commented Jun 2, 2015

Assigning to @Ms2ger since @jdm will be out by the time #6150 lands.

@nox nox force-pushed the nox:merge-generic-functions branch from 8694a5a to acd3399 Jun 3, 2015
@nox
Copy link
Member Author

nox commented Jun 3, 2015

@Ms2ger Split as requested.

@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2015

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

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 20, 2015

Please rebase on master.

@nox nox force-pushed the nox:merge-generic-functions branch from acd3399 to 9f75356 Jun 20, 2015
@nox nox removed the S-needs-rebase label Jun 20, 2015
@nox
Copy link
Member Author

nox commented Jun 20, 2015

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 20, 2015

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


components/script/dom/bindings/codegen/CodegenRust.py, line 2699 [r2] (raw file):
We should be able to kill this now.


components/script/dom/bindings/codegen/CodegenRust.py, line 4586 [r2] (raw file):
Remove those too?


components/script/dom/bindings/conversions.rs, line 575 [r2] (raw file):
How does this not break proxy objects?


components/script/dom/bindings/conversions.rs, line 589 [r2] (raw file):
Why mut obj?


components/script/dom/bindings/utils.rs, line 694 [r2] (raw file):
There's a bunch of duplicated code in all those; could you share some more?


Comments from the review on Reviewable.io

@nox nox force-pushed the nox:merge-generic-functions branch from 9f75356 to 0dbcda4 Jun 21, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 22, 2015

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


components/script/dom/bindings/utils.rs, line 678 [r3] (raw file):
Perhaps get_this()


Comments from the review on Reviewable.io

@nox nox force-pushed the nox:merge-generic-functions branch from 0dbcda4 to ac426b5 Jun 22, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 22, 2015

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


Comments from the review on Reviewable.io

@michaelwu
Copy link
Contributor

michaelwu commented Jun 23, 2015

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


components/script/dom/bindings/utils.rs, line 691 [r3] (raw file):
I usually wouldn't expect this to work. The rooters for GC things (Rooted<T>) usually can't be moved, so returning them is generally a bad idea unless that's the only thing you're returning and you use the special constructor that allows for this - Rooted::new_with_addr. If the assertion at https://github.com/servo/rust-mozjs/blob/master/src/rust.rs#L286 doesn't catch it though.. it's probably ok. The callers might be moving it to the right place if it works.


Comments from the review on Reviewable.io

@nox nox force-pushed the nox:merge-generic-functions branch from ac426b5 to 792680c Jun 23, 2015
@nox
Copy link
Member Author

nox commented Jun 23, 2015

@Ms2ger Your turn again!


Review status: 4 of 5 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 24, 2015

-S-awaiting-review +S-needs-squash


Reviewed 1 of 1 files at r5.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@nox nox force-pushed the nox:merge-generic-functions branch from 792680c to a909835 Jun 24, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 24, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2015

📌 Commit a909835 has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2015

Testing commit a909835 with merge 6247a96...

bors-servo pushed a commit that referenced this pull request Jun 24, 2015
Merge generic funs to share them across all bindings (fixes #2684)



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

bors-servo commented Jun 24, 2015

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

@bors-servo bors-servo merged commit a909835 into servo:master Jun 24, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nox nox deleted the nox:merge-generic-functions branch Aug 20, 2015
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

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