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

Jump to bottom

Implement URLSearchParams::getAll #5439

Closed
frewsxcv opened this issue Mar 28, 2015 · 9 comments
Closed

Implement URLSearchParams::getAll #5439

frewsxcv opened this issue Mar 28, 2015 · 9 comments
Labels
A-content/dom C-assigned E-easy

Comments

@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Mar 28, 2015

This might be a candidate for 'E-Easy' since URLSearchParams::get is already done

https://url.spec.whatwg.org/#dom-urlsearchparams-getall

https://github.com/servo/servo/blob/master/components/script/dom/urlsearchparams.rs
@jdm
Copy link
Member

@jdm jdm commented Mar 29, 2015

https://github.com/servo/servo/blob/master/components/script/dom/webidls/URLSearchParams.webidl
@bdero
Copy link
Contributor

@bdero bdero commented Mar 29, 2015

I think this is blocked by #544
@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Mar 29, 2015

Also, we need tests.
@dhananjay92
Copy link
Contributor

@dhananjay92 dhananjay92 commented Mar 29, 2015

I would like to work on this issue. Should I wait till #544 is solved?
@jdm jdm removed the E-easy label Mar 29, 2015
@jdm
Copy link
Member

@jdm jdm commented Mar 29, 2015

Yes.
@frewsxcv
Copy link
Member Author

@frewsxcv frewsxcv commented Jan 12, 2016

#544 is done. Marking this as E-Easy again
@frewsxcv frewsxcv added the E-easy label Jan 12, 2016
@fmmrtns
Copy link
Contributor

@fmmrtns fmmrtns commented Jan 15, 2016

Hey, I'd like to work on this one if no else is?

I added the GetAll function, and deleted the lines that expected to FAIL for getAll() in the files:

tests/wpt/metadata/url/interfaces.html.ini
tests/wpt/metadata/url/urlsearchparams-getall.html.ini (actually I deleted this file)

And now tests are passing, but I'm not sure if this is the correct way of doing this?
@nox
Copy link
Member

@nox nox commented Jan 15, 2016

@fmmartins It is! Feel free to make a PR!
@nox nox added the C-assigned label Jan 15, 2016
@fmmrtns
Copy link
Contributor

@fmmrtns fmmrtns commented Jan 15, 2016

@nox Thanks, will do!
@fmmrtns fmmrtns mentioned this issue Jan 15, 2016
Merged
bors-servo added a commit that referenced this issue Jan 17, 2016
@bors-servo
Auto merge of #9337 - fmmartins:urlsearchparams-getall, r=nox
Loading status checks…
a4e805d 
Implement URLSearchParams::getAll

Fixes #5439.

Please tell me if anything is wrong or needs some change!

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9337)
<!-- Reviewable:end -->
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/dom C-assigned E-easy
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
@jdm @Ms2ger @nox @frewsxcv @bdero @fmmrtns @dhananjay92
You can’t perform that action at this time.