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

Bound several link properties. #4248

Closed
wants to merge 1 commit into from
Closed

Bound several link properties. #4248

wants to merge 1 commit into from

Conversation

@Zirak
Copy link
Contributor

Zirak commented Dec 5, 2014

Specifically: href, rel, media, hreflang and type.

Fixes #4226.

Review on Reviewable

Specifically: href, rel, media, hreflang and type.
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Dec 5, 2014

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

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.

@Zirak
Copy link
Contributor Author

Zirak commented Dec 5, 2014

Note that the wpt tests for link.href are broken, since they rely on implementing several HTMLAnchorElement properties: https://github.com/servo/web-platform-tests/blob/servo/html/dom/reflection.js#L5-L36

It may be desirable to first fix those before merging this commit.

@Manishearth

This comment has been minimized.

Copy link

Manishearth commented on a61e869 Dec 5, 2014

r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented on a61e869 Dec 5, 2014

saw approval from Manishearth
at Zirak@a61e869

This comment has been minimized.

Copy link
Contributor

bors-servo replied Dec 5, 2014

merging Zirak/servo/link-attrs = a61e869 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Dec 5, 2014

Zirak/servo/link-attrs = a61e869 merged ok, testing candidate = cbe93f7

This comment has been minimized.

Copy link
Contributor

bors-servo replied Dec 5, 2014

bors-servo pushed a commit that referenced this pull request Dec 5, 2014
Specifically: href, rel, media, hreflang and type.

Fixes #4226.
@Manishearth
Copy link
Member

Manishearth commented Dec 5, 2014

/html/dom/reflection-metadata.html
----------------------------------
PASS expected FAIL link.href: typeof IDL attribute
PASS expected FAIL link.href: IDL get with DOM attribute unset
PASS expected FAIL link.href: IDL set to "" followed by getAttribute()
PASS expected FAIL link.href: IDL set to " foo " followed by getAttribute()
PASS expected FAIL link.href: IDL set to "http://site.example/" followed by getAttribute()
PASS expected FAIL link.href: IDL set to "//site.example/path???@#l" followed by getAttribute()
PASS expected FAIL link.href: IDL set to "\0\x01\x02\x03\x04\x05\x06\x07 \b\t\n\v\f\r\x0e\x0f \x10\x11\x12\x13\x14\x15\x16\x17 \x18\x19\x1a\x1b\x1c\x1d\x1e\x1f " followed by getAttribute()
PASS expected FAIL link.href: IDL set to undefined followed by getAttribute()
PASS expected FAIL link.href: IDL set to 7 followed by getAttribute()
PASS expected FAIL link.href: IDL set to 1.5 followed by getAttribute()
PASS expected FAIL link.href: IDL set to true followed by getAttribute()
PASS expected FAIL link.href: IDL set to false followed by getAttribute()
PASS expected FAIL link.href: IDL set to object "[object Object]" followed by getAttribute()
PASS expected FAIL link.href: IDL set to NaN followed by getAttribute()
PASS expected FAIL link.href: IDL set to Infinity followed by getAttribute()
PASS expected FAIL link.href: IDL set to -Infinity followed by getAttribute()
PASS expected FAIL link.href: IDL set to "\0" followed by getAttribute()
PASS expected FAIL link.href: IDL set to null followed by getAttribute()
PASS expected FAIL link.href: IDL set to object "test-toString" followed by getAttribute()

Could you just remove these entries from tests/wpt/metadata/html/dom/reflection-metadata.html.ini (amend+force push is fine)? Thanks!

@Zirak
Copy link
Contributor Author

Zirak commented Dec 5, 2014

Oddly enough, after removing said tests locally, the build still fails, only now it didn't expect anything:

/html/dom/reflection-metadata.html
----------------------------------

FAIL link.href: setAttribute() to "" followed by IDL get
FAIL link.href: setAttribute() to " foo " followed by IDL get
FAIL link.href: setAttribute() to "http://site.example/" followed by IDL get
FAIL link.href: setAttribute() to "//site.example/path???@#l" followed by IDL get
FAIL link.href: setAttribute() to "\0\x01\x02\x03\x04\x05\x06\x07 \b\t\n\v\f\r\x0e\x0f \x10\x11\x12\x13\x14\x15\x16\x17 \x18\x19\x1a\x1b\x1c\x1d\x1e\x1f " followed by IDL get
FAIL link.href: setAttribute() to undefined followed by IDL get
FAIL link.href: setAttribute() to 7 followed by IDL get
FAIL link.href: setAttribute() to 1.5 followed by IDL get
FAIL link.href: setAttribute() to true followed by IDL get
FAIL link.href: setAttribute() to false followed by IDL get
FAIL link.href: setAttribute() to object "[object Object]" followed by IDL get
FAIL link.href: setAttribute() to NaN followed by IDL get
FAIL link.href: setAttribute() to Infinity followed by IDL get
FAIL link.href: setAttribute() to -Infinity followed by IDL get
FAIL link.href: setAttribute() to "\0" followed by IDL get
FAIL link.href: setAttribute() to null followed by IDL get
FAIL link.href: setAttribute() to object "test-toString" followed by IDL get
FAIL link.href: setAttribute() to object "test-valueOf" followed by IDL get
FAIL link.href: IDL set to "" followed by IDL get
FAIL link.href: IDL set to " foo " followed by IDL get
FAIL link.href: IDL set to "http://site.example/" followed by IDL get
FAIL link.href: IDL set to "//site.example/path???@#l" followed by IDL get
FAIL link.href: IDL set to "\0\x01\x02\x03\x04\x05\x06\x07 \b\t\n\v\f\r\x0e\x0f \x10\x11\x12\x13\x14\x15\x16\x17 \x18\x19\x1a\x1b\x1c\x1d\x1e\x1f " followed by IDL get
FAIL link.href: IDL set to undefined followed by IDL get
FAIL link.href: IDL set to 7 followed by IDL get
FAIL link.href: IDL set to 1.5 followed by IDL get
FAIL link.href: IDL set to true followed by IDL get
FAIL link.href: IDL set to false followed by IDL get
FAIL link.href: IDL set to object "[object Object]" followed by IDL get
FAIL link.href: IDL set to NaN followed by IDL get
FAIL link.href: IDL set to Infinity followed by IDL get
FAIL link.href: IDL set to -Infinity followed by IDL get
FAIL link.href: IDL set to "\0" followed by IDL get
FAIL link.href: IDL set to null followed by IDL get
FAIL link.href: IDL set to object "test-toString" followed by IDL get
FAIL link.href: IDL set to object "test-valueOf" followed by IDL get

Note that this includes tests which for some reason "passed" before.

@Manishearth
Copy link
Member

Manishearth commented Dec 8, 2014

I'll poke at this with wptupdate and see what happens.

@Manishearth
Copy link
Member

Manishearth commented Dec 9, 2014

This seems to work for me. I'll open this in a separate PR since there's a chance the manifest fix went wrong somehow on your side. (My wptupdate was behaving a bit wonky too)

@Manishearth
Copy link
Member

Manishearth commented Dec 9, 2014

Moved to #4298

@Manishearth Manishearth closed this Dec 9, 2014
bors-servo pushed a commit that referenced this pull request Dec 9, 2014
@Manishearth
Copy link
Member

Manishearth commented Dec 9, 2014

Woohoo, it merged! Thanks for the pull request!

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.

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