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

Implement Element::set_url_attribute #22568

Closed
dlrobertson opened this issue Dec 28, 2018 · 4 comments
Closed

Implement Element::set_url_attribute #22568

dlrobertson opened this issue Dec 28, 2018 · 4 comments

Comments

@dlrobertson
Copy link
Contributor

@dlrobertson dlrobertson commented Dec 28, 2018

Element::set_url_attribute the counterpart to Element::get_url_attribute is not implemented yet. Once implemented the macro make_url_setter should use the new method instead of Element::set_string_attribute.

Related to: #22563 (comment) and #22454

@shanavas786
Copy link
Contributor

@shanavas786 shanavas786 commented Jan 11, 2019

@highfive assign me

@highfive highfive added the C-assigned label Jan 11, 2019
@highfive
Copy link

@highfive highfive commented Jan 11, 2019

Hey @shanavas786! Thanks for your interest in working on this issue. It's now assigned to you!

@shanavas786 shanavas786 mentioned this issue Jan 14, 2019
2 of 4 tasks complete
@nox
Copy link
Member

@nox nox commented Jan 14, 2019

https://html.spec.whatwg.org/#reflecting-content-attributes-in-idl-attributes

If a reflecting IDL attribute is a USVString attribute whose content attribute is defined to contain a URL, then on getting, if the content attribute is absent, the IDL attribute must return the empty string. Otherwise, the IDL attribute must parse the value of the content attribute relative to the element's node document and if that is successful, return the resulting URL string. If parsing fails, then the value of the content attribute must be returned instead, converted to a USVString. On setting, the content attribute must be set to the specified new value.

I don't see why we should have a make_url_setter, given the setter is specified to have no URL-specific behaviour.

@dlrobertson
Copy link
Contributor Author

@dlrobertson dlrobertson commented Jan 14, 2019

@nox good catch

The make_url_setter is needed because the given structure has a USVString element and the setter is DOMString, so we need USVString -> DOMString, but I guess we don't necessarily need Element::set_url_attribute. We could just keep the USVString -> DOMString evaluation in the macro make_url_setter.

CC @ferjm

bors-servo added a commit that referenced this issue Jan 14, 2019
Implement Element::set_url_attribute

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22568

<!-- Either: -->
- [ ] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22682)
<!-- Reviewable:end -->
@ferjm ferjm added this to Done in Media playback Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

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