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.

Sign up for GitHub

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

Jump to bottom

Rename ambigous unwrapping in bindings code #2108

Closed
jdm opened this issue Apr 14, 2014 · 4 comments
Closed

Rename ambigous unwrapping in bindings code #2108

jdm opened this issue Apr 14, 2014 · 4 comments
Labels
A-content/bindings The DOM bindings C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor.

Comments

@jdm
Copy link
Member

jdm commented Apr 14, 2014

@bholley has feelings: "In a plea to have better terminology than Gecko, I think this should be called native_from_reflector or somesuch. "Unwrap" is especially confusing here, because we're both stripping off wrappers and getting a native from a reflector." This refers to methods like unwrap_object, unwrap_jsmanaged, and unwrap in dom/bindings/utils.rs.
@Ms2ger Ms2ger added the E-easy label Jun 9, 2014
@hgentry
Copy link
Contributor

hgentry commented Jan 31, 2015

So just rename them all from unwrap* to native_from_reflector*? I can do that.

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 31, 2015

Yep, pretty much

@jdm jdm added the C-assigned There is someone working on resolving the issue label Jan 31, 2015
@jdm
Copy link
Member Author

jdm commented Feb 20, 2015

Still working on this, @hgentry?

@jdm jdm removed the C-assigned There is someone working on resolving the issue label Mar 11, 2015
@doublec
Copy link
Contributor

doublec commented Mar 12, 2015

@jdm, I'd like to work on this if possible!

@jdm jdm added the C-assigned There is someone working on resolving the issue label Mar 12, 2015
@doublec doublec mentioned this issue Mar 12, 2015
Merged
bors-servo pushed a commit that referenced this issue Mar 12, 2015
auto merge of #5202 : doublec/servo/rename_unwrap, r=Ms2ger
d7e9b8b 
Fix for issue #2108. That issue mentions an `unwrap_object` which doesn't seem to exist so I renamed `unwrap` to `native_from_reflector` and `unwrap_jsmanaged` to `native_from_reflector_jsmanaged`. The latter is a bit unweildy - maybe a shorter name might be better?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/bindings The DOM bindings C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@doublec @jdm @Ms2ger @hgentry