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

NamedGetter and NamedSetter codegen assumes parameter named name #2240

Closed
jsanders opened this issue Apr 27, 2014 · 14 comments
Closed

NamedGetter and NamedSetter codegen assumes parameter named name #2240

jsanders opened this issue Apr 27, 2014 · 14 comments
Labels
A-content/bindings The DOM bindings E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss

Comments

@jsanders
Copy link
Contributor

I ran into this while looking into #2117. A few places dealing with NamedGetter and NamedSetter in CodeGenRust.py explicitly define a variable name (example) while the name of the parameter that actually gets passed is defined in the webidl. It seems like currently only HTMLCollection is using this functionality, and things work because the getter in its webidl has name as the parameter name. But the Storage webidl uses key as the parameter name.

The quick fix is to just change the webidl to use name, but I thought I'd file an issue, because this is confusing.

@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 27, 2014

So the issue is that we have

let name = ...;
foo.NamedGetter(key, ...);

This is https://bugzilla.mozilla.org/show_bug.cgi?id=804738

@jsanders
Copy link
Contributor Author

Ah, thanks, yeah that's definitely it. It looks like that fix didn't make it into servo?

@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 27, 2014

Yeah. Shouldn't be too hard to port the changes there, though.

@jdm jdm added A-content/bindings The DOM bindings E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss labels Oct 6, 2014
@JayNakrani
Copy link
Contributor

I'd like to work on this. Because there's Mozilla's code that I can use as reference, I think I can do it.
Just a quick question. How do I know if my changes are working as expected? Are there any tests to run?

@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 20, 2015

Running ./mach test-wpt webstorage should have some new passing tests after the change, at least

@JayNakrani
Copy link
Contributor

Okay, cool. I am on it.

@jdm jdm added the C-assigned There is someone working on resolving the issue label Apr 20, 2015
JayNakrani added a commit to JayNakrani/servo that referenced this issue Apr 22, 2015
@JayNakrani
Copy link
Contributor

@Ms2ger Here is what I have done so far. No build errors.

I re-built and ran the test using ./mach test-wpt webstorage both on master and the branch with changes. It gives the same result. Below is the summary of test results. Please let me know if I missed anything.

Summary
=======

Ran 185 tests (27 parents, 158 subtests)
Expected results: 177
Unexpected results: 8 (FAIL: 8)

Unexpected Results
==================

/webstorage/setitem.html
------------------------
FAIL localStorage[]
FAIL localStorage[] update
FAIL localStorage[undefined]
FAIL localStorage[null]
FAIL sessionStorage[]
FAIL sessionStorage[] update
FAIL sessionStorage[undefined]
FAIL sessionStorage[null]

@JayNakrani
Copy link
Contributor

@Ms2ger Should I go ahead and do pull?!

@jdm
Copy link
Member

jdm commented Apr 25, 2015

@Dhananjay92 Those changes aren't quite right; you want to modify dom/webidls/TestCodeGen.webidl instead of the one you did, and there's no point in modifying the C++ header - it's just left over from when I imported the code generation implementation from Firefox.

@klusark
Copy link
Contributor

klusark commented Jun 27, 2015

@Dhananjay92, are you still working on this?

@JayNakrani
Copy link
Contributor

@klusark Nope, not any more. Would love to, but I have very less time. You can take it. 😃
BTW you've got some code already done for you. 😉

@jdm jdm removed the C-assigned There is someone working on resolving the issue label Jul 7, 2015
@Yoric
Copy link
Contributor

Yoric commented Aug 6, 2015

I'll try and work on this.

@Yoric
Copy link
Contributor

Yoric commented Aug 6, 2015

I'll need some handholding, though, as I'm not familiar with Servo yet.

Do I understand correctly that the changes done by @Dhananjay92 on the Python file are correct and that I need to implement his interface TestIndexedGetterAndSetterAndNamedGetterInterface?

@jdm
Copy link
Member

jdm commented Aug 6, 2015

I believe the python changes are correct, but the test-related changes are incorrect. Servo doesn't use any of those files.

Yoric added a commit to Yoric/servo that referenced this issue Aug 26, 2015
Yoric added a commit to Yoric/servo that referenced this issue Aug 26, 2015
Yoric added a commit to Yoric/servo that referenced this issue Aug 26, 2015
Yoric added a commit to Yoric/servo that referenced this issue Aug 26, 2015
bors-servo pushed a commit that referenced this issue Aug 27, 2015
Fixes #2240 - NamedGetter and NamedSetter do not assume that the arg is named `name`

I'm not totally sure about how to test this.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7387)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/bindings The DOM bindings E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss
Projects
None yet
Development

No branches or pull requests

6 participants