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 #18612

Merged
merged 1 commit into from Sep 30, 2017
Merged

Implement EventListenerOptions for EventTarget #18612

merged 1 commit into from Sep 30, 2017

Conversation

@nox
Copy link
Member

nox commented Sep 24, 2017 •
edited by SimonSapin

This change is Reviewable
@highfive
Copy link

highfive commented Sep 24, 2017

Heads up! This PR modifies the following files:

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

highfive commented Sep 24, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@nox nox mentioned this pull request Sep 24, 2017
Closed
@nox
Copy link
Member Author

nox commented Sep 24, 2017

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2017

⌛ Trying commit eb08b66 with merge 25df84b...
bors-servo added a commit that referenced this pull request Sep 24, 2017
@bors-servo
Auto merge of #18612 - servo:event-listener-options, r=<try>
Loading status checks…
25df84b 
Implement EventListenerOptions for EventTarget

<!-- 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/18612)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2017

💔 Test failed - linux-rel-wpt
@asajeffrey
asajeffrey reviewed Sep 26, 2017
View changes
Copy link
Member

asajeffrey left a comment

LGTM, you can r=me once the test issues are cleared up.
components/script/dom/mediaquerylist.rs
self.upcast::<EventTarget>().add_event_listener(
DOMString::from_string("change".to_owned()),
listener,
AddEventListenerOptions { parent: EventListenerOptions { capture: false } },

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Sep 26, 2017

Member

Worth implementing Default for AddEventListenerOptions?
components/script/dom/eventtarget.rs
listener: Option<Rc<EventListener>>,
options: EventListenerOptionsOrBoolean,
) {
self.remove_event_listener(ty, listener, options.into())

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Sep 26, 2017

Member

Ditto.
components/script/dom/eventtarget.rs
listener: Option<Rc<EventListener>>,
options: AddEventListenerOptionsOrBoolean,
) {
self.add_event_listener(ty, listener, options.into())

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Sep 26, 2017

Member

Nit: I always find x.into() very mysterious compared to Y::from(x).
@nox nox force-pushed the event-listener-options branch from eb08b66 to fa58bf9 Sep 29, 2017
@nox
Copy link
Member Author

nox commented Sep 29, 2017

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2017

⌛ Trying commit fa58bf9 with merge fe1534e...
bors-servo added a commit that referenced this pull request Sep 29, 2017
@bors-servo
Auto merge of #18612 - servo:event-listener-options, r=<try>
Loading status checks…
fe1534e 
Implement EventListenerOptions for EventTarget

<!-- 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/18612)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 30, 2017

💔 Test failed - mac-rel-wpt2
@GuillaumeGomez @nox
Implement EventListenerOptions for EventTarget
Loading status checks…
3d0b7fb 
For now, only "capture" is supported.
@nox nox force-pushed the event-listener-options branch from fa58bf9 to 3d0b7fb Sep 30, 2017
@nox
Copy link
Member Author

nox commented Sep 30, 2017

@bors-servo r=asajeffrey
@bors-servo
Copy link
Contributor

bors-servo commented Sep 30, 2017

📌 Commit 3d0b7fb has been approved by asajeffrey
@bors-servo
Copy link
Contributor

bors-servo commented Sep 30, 2017

⌛ Testing commit 3d0b7fb with merge 81044e0...
bors-servo added a commit that referenced this pull request Sep 30, 2017
@bors-servo
Auto merge of #18612 - servo:event-listener-options, r=asajeffrey
Loading status checks…
81044e0 
Implement EventListenerOptions for EventTarget

<!-- 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/18612)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 30, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: asajeffrey
Pushing 81044e0 to master...
@bors-servo bors-servo merged commit 3d0b7fb into master Sep 30, 2017
4 checks passed
4 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details
@nox nox deleted the event-listener-options branch Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asajeffrey asajeffrey

Assignees

@asajeffrey asajeffrey

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@nox @highfive @bors-servo @asajeffrey @GuillaumeGomez
You can’t perform that action at this time.