Skip to content

/ servo

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

Implement RootedReference<T> for Option<JS<T>> #9664

Merged
merged 1 commit into from Feb 17, 2016
Merged

Implement RootedReference<T> for Option<JS<T>> #9664

merged 1 commit into from Feb 17, 2016

Conversation

@dlrobertson
Copy link
Contributor

dlrobertson commented Feb 16, 2016

A basic implementation of RootedReference for Option<JS<T>> based off of
other implementations of RootedReference for Option wrapped types.

Really I just wanted an excuse to read more in bindings 😄 Let me know
if you have any comments or critiques.

Fixes #9654

Review on Reviewable
@highfive
Copy link

highfive commented Feb 16, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@dlrobertson
Copy link
Contributor Author

dlrobertson commented Feb 16, 2016

r? @Ms2ger
@KiChjang
Copy link
Member

KiChjang commented Feb 16, 2016

Can I get you to look for places where this might be used? A quick git grep "Option<JS<" reveals 3 files:

  • browsingcontext.rs
  • formdata.rs
  • htmlinputelement.rs
@KiChjang KiChjang assigned KiChjang and unassigned Ms2ger Feb 16, 2016
@dlrobertson
Implement RootedReference<T> for Option<JS<T>>
Loading status checks…
7a35ef1 
An implementation of RootedReference for Option<JS<T>> based off of
other implementations of RootedReference for Option wrapped types.
@dlrobertson dlrobertson force-pushed the dlrobertson:i9654 branch from cd23585 to 7a35ef1 Feb 16, 2016
@dlrobertson
Copy link
Contributor Author

dlrobertson commented Feb 16, 2016

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

components/script/dom/browsingcontext.rs, line 81 [r1] (raw file):
This one is pretty obvious.

components/script/dom/htmlinputelement.rs, line 815 [r1] (raw file):
I went back an forth on this. Technically in the prior version o is a &JS<HTMLElement> while in the new revision o is an &HTMLElement. Please double check me on this.

Comments from the review on Reviewable.io
@dlrobertson
Copy link
Contributor Author

dlrobertson commented Feb 16, 2016

Thanks for the comments!
@nox
Copy link
Member

nox commented Feb 17, 2016

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/script/dom/htmlinputelement.rs, line 815 [r1] (raw file):
I think you are right, but this version is cleaner anyway.

Comments from the review on Reviewable.io
@KiChjang
Copy link
Member

KiChjang commented Feb 17, 2016

@bors-servo r+

Thanks for your work!

Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/script/dom/htmlinputelement.rs, line 815 [r1] (raw file):
There's not a lot of reason to keep a JS<T> pointer around when you can have a &T instead.

Comments from the review on Reviewable.io
@KiChjang
Copy link
Member

KiChjang commented Feb 17, 2016

@bors-servo r+
@KiChjang KiChjang closed this Feb 17, 2016
@KiChjang KiChjang reopened this Feb 17, 2016
@KiChjang
Copy link
Member

KiChjang commented Feb 17, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Feb 17, 2016

📌 Commit 7a35ef1 has been approved by KiChjang
@bors-servo
Copy link
Contributor

bors-servo commented Feb 17, 2016

⌛ Testing commit 7a35ef1 with merge 1545368...
bors-servo added a commit that referenced this pull request Feb 17, 2016
@bors-servo
Auto merge of #9664 - danlrobertson:i9654, r=KiChjang
1545368 
Implement RootedReference<T> for Option<JS<T>>

A basic implementation of `RootedReference for Option<JS<T>>` based off of
other implementations of `RootedReference` for `Option` wrapped types.

Really I just wanted an excuse to read more in `bindings` 😄 Let me know
if you have any comments or critiques.

Fixes  #9654

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

bors-servo commented Feb 17, 2016

💔 Test failed - linux-rel
@KiChjang
Copy link
Member

KiChjang commented Feb 17, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Feb 17, 2016

⌛ Testing commit 7a35ef1 with merge ab381cf...
bors-servo added a commit that referenced this pull request Feb 17, 2016
@bors-servo
Auto merge of #9664 - danlrobertson:i9654, r=KiChjang
Loading status checks…
ab381cf 
Implement RootedReference<T> for Option<JS<T>>

A basic implementation of `RootedReference for Option<JS<T>>` based off of
other implementations of `RootedReference` for `Option` wrapped types.

Really I just wanted an excuse to read more in `bindings` 😄 Let me know
if you have any comments or critiques.

Fixes  #9654

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

bors-servo commented Feb 17, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 7a35ef1 into servo:master Feb 17, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@dlrobertson dlrobertson deleted the dlrobertson:i9654 branch Apr 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@KiChjang KiChjang

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@dlrobertson @highfive @KiChjang @nox @bors-servo @Ms2ger
You can’t perform that action at this time.