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

Add HTMLFormControlsCollection interface #7845

Closed
wants to merge 1 commit into from

Conversation

@frewsxcv frewsxcv force-pushed the frewsxcv:htmlformcontrolscollection branch 2 times, most recently from 6aab148 to 25016f9 Oct 4, 2015
@Manishearth
Copy link
Member

Manishearth commented Oct 4, 2015

LGTM overall. Minor nits.


Review status: 0 of 8 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


components/script/dom/htmlcollection.rs, line 25 [r1] (raw file):
Would prefer if these were private with a constructor and #[privatize]. Too easy to accidentally extract a JS<T> otherwise


components/script/dom/htmlformcontrolscollection.rs, line 24 [r1] (raw file):
Spec link: https://html.spec.whatwg.org/multipage/forms.html#category-listed


components/script/dom/htmlformcontrolscollection.rs, line 57 [r1] (raw file):
What's this for?


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Manishearth commented Oct 4, 2015

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Manishearth commented Oct 4, 2015

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


components/script/dom/htmlformcontrolscollection.rs, line 59 [r1] (raw file):
@nox: Any reason why this needs to be explicitly implemented (given that in can inherit)?

We haven't uncommented the explicit overriding getter in the webidl. (I think we should file a followup for that though)


Comments from the review on Reviewable.io

@nox
Copy link
Member

nox commented Oct 4, 2015

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


components/script/dom/htmlformcontrolscollection.rs, line 59 [r1] (raw file):
You mean this? :) #5875


Comments from the review on Reviewable.io

@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 4, 2015

@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 4, 2015

components/script/dom/htmlformcontrolscollection.rs, line 57 [r1] (raw file):
The tidy checker makes sure all methods in *Methods traits have spec links. This comment allows that behavior to be overridden. I didn't link to the spec for these methods here because these are all inherited by the parent interface


Comments from the review on Reviewable.io

@frewsxcv frewsxcv force-pushed the frewsxcv:htmlformcontrolscollection branch from 25016f9 to 17124ee Oct 4, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 4, 2015

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Oct 4, 2015

📌 Commit 17124ee has been approved by Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Oct 4, 2015

Testing commit 17124ee with merge fd182a3...

bors-servo pushed a commit that referenced this pull request Oct 4, 2015
Add HTMLFormControlsCollection interface

https://html.spec.whatwg.org/multipage/#htmlformcontrolscollection

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7845)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 4, 2015

💔 Test failed - mac-rel-wpt

@Manishearth
Copy link
Member

Manishearth commented Oct 4, 2015

You need to update /_mozilla/mozilla/interfaces.html

@frewsxcv frewsxcv force-pushed the frewsxcv:htmlformcontrolscollection branch from 17124ee to 0cc6c61 Oct 4, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 4, 2015

@Manishearth
Copy link
Member

Manishearth commented Oct 4, 2015

That's fine, it's a window-exposed DOM type (try it in Firefox). Make sure you add it to the DOM interface array, not the ECMA array.

@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 4, 2015

Yep, it's added

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 5, 2015

There's ongoing discussion about this: whatwg/html#200

@bors-servo
Copy link
Contributor

bors-servo commented Oct 10, 2015

The latest upstream changes (presumably #7960) made this pull request unmergeable. Please resolve the merge conflicts.

@frewsxcv
Copy link
Member Author

frewsxcv commented Nov 17, 2015

@frewsxcv frewsxcv closed this Nov 17, 2015
@frewsxcv frewsxcv deleted the frewsxcv:htmlformcontrolscollection branch Nov 17, 2015
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.

None yet

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