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

Implement EventListenerOptions for EventTarget #12952

Closed
wants to merge 1 commit into from
Closed

Implement EventListenerOptions for EventTarget #12952

wants to merge 1 commit into from

Conversation

@GuillaumeGomez
Copy link
Contributor

GuillaumeGomez commented Aug 20, 2016 •
edited

Fixes #9785.

Needs #11612 to be fixed first.

This change is Reviewable
@highfive
Copy link

highfive commented Aug 20, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/eventtarget.rs, components/script/dom/webidls/EventTarget.webidl
@highfive
Copy link

highfive commented Aug 20, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 20, 2016

r? @nox
@highfive highfive assigned nox and unassigned emilio Aug 20, 2016
@jdm
Copy link
Member

jdm commented Aug 20, 2016

I'm not sure that it makes sense to merge this without the actual behaviour changes for the new kinds of listeners.
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Aug 20, 2016

@jdm: I just opened it to not forget it (I did this a long time ago already).
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 22, 2016

I'm sure that it does not make sense, but we could take { capture } if there are tests.
@nox
Copy link
Member

nox commented Aug 23, 2016

We should only use EventListenerOptions for now given we don't support the rest.
@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2016

☔ The latest upstream changes (presumably #13600) made this pull request unmergeable. Please resolve the merge conflicts.
@KiChjang
Copy link
Member

KiChjang commented Dec 7, 2016

@GuillaumeGomez This still looks forgotten to me even after opening a PR for it.
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Dec 7, 2016

@KiChjang: It was on stand-by, waiting for a feature. However, I can't remember which at all...
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 7, 2016

Unions with objects
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Dec 7, 2016

Has it been added or not yet?
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 7, 2016

I think it was; that's what causing the dead code warning we have in script right now
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Dec 7, 2016

Ok, I'll update this PR then!
@GuillaumeGomez
Implement EventListenerOptions for EventTarget
Loading status checks…
483b942
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Dec 11, 2016

I still have the following error:

TypeError: Can't handle AddEventListenerOptions (Wrapper) in unions yet
@jdm
Copy link
Member

jdm commented Jan 19, 2017

I don't see a reason to keep this open until #11612 is fixed.
@jdm jdm closed this Jan 19, 2017
@KiChjang
Copy link
Member

KiChjang commented Jan 30, 2017

So #11612 is fixed now, @GuillaumeGomez would you still like to work on this?
@GuillaumeGomez
Copy link
Contributor Author

GuillaumeGomez commented Jan 30, 2017

Sure!
@GuillaumeGomez GuillaumeGomez mentioned this pull request Mar 2, 2017
Closed
bors-servo added a commit that referenced this pull request Mar 24, 2017
@bors-servo
Auto merge of #15803 - GuillaumeGomez:event_listener_options, r=<try>
Loading status checks…
a8afb0c 
Implement EventListenerOptions for EventTarget

It still needs to update tests and confirm that the code is the one expected. Reopening of #12952.

<!-- 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/15803)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
S-awaiting-review S-needs-rebase
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Implement EventListenerOptions for EventTarget
8 participants
@GuillaumeGomez @highfive @jdm @Ms2ger @nox @bors-servo @KiChjang @emilio
You can’t perform that action at this time.