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 AddEventListenerOptions: once #22100

Merged
merged 1 commit into from Dec 22, 2018

Conversation

Projects
None yet
6 participants
@Eijebong
Copy link
Member

Eijebong commented Nov 3, 2018

Fixes #13242


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Nov 3, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/event.rs, components/script/dom/eventtarget.rs, components/script/dom/mediaquerylist.rs, components/script/dom/webidls/EventTarget.webidl
  • @KiChjang: components/script/dom/event.rs, components/script/dom/eventtarget.rs, components/script/dom/mediaquerylist.rs, components/script/dom/webidls/EventTarget.webidl
@Eijebong

This comment has been minimized.

Copy link
Member

Eijebong commented Nov 3, 2018

@bors-servo try=wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 3, 2018

⌛️ Trying commit 7260c30 with merge 1051b37...

bors-servo added a commit that referenced this pull request Nov 3, 2018

Auto merge of #22100 - Eijebong:once, r=<try>
Implement AddEventListenerOptions: once

Fixes #13242

<!-- 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/22100)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 3, 2018

☀️ Test successful - linux-rel-css, linux-rel-wpt
State: approved= try=True

@Eijebong Eijebong force-pushed the Eijebong:once branch from 7260c30 to 2aa36cb Nov 3, 2018


impl std::cmp::PartialEq for EventListenerEntry {
fn eq(&self, other: &Self) -> bool {
self.phase == other.phase && self.listener == other.listener

This comment has been minimized.

@nox

nox Nov 22, 2018

Member

Why is once ignored?

This comment has been minimized.

@Eijebong

Eijebong Nov 22, 2018

Member

Because removing an eventlistener without specifying once: true still needs to find the ones that are once

Show resolved Hide resolved components/script/dom/eventtarget.rs Outdated
Show resolved Hide resolved components/script/dom/eventtarget.rs

let listener = EventListenerType::Additive(listener);
for entry in handlers.get_mut(ty) {
if let Some(position) = entry.iter().position(|e| e.listener == listener && e.once) {

This comment has been minimized.

@nox

nox Dec 7, 2018

Member

You can probably use drain here, which will result in slightly more performant code.

});
}
},
}
}

pub fn remove_listener_if_once(&self, ty: &Atom, listener: Rc<EventListener>) {

This comment has been minimized.

@nox

nox Dec 7, 2018

Member

Make listener be a &Rc<EventListener> and you will be able to avoid a cloning operation in the caller.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 19, 2018

This should improve the behaviour of html/webappapis/dynamic-markup-insertion/opening-the-input-stream/bailout-exception-vs-return-xml.window.js; otherwise it ends up endlessly loading new iframes.

@Eijebong

This comment has been minimized.

Copy link
Member

Eijebong commented Dec 20, 2018

There, anything else ? :p

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 22, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 22, 2018

📌 Commit 4b45958 has been approved by jdm

@highfive highfive assigned jdm and unassigned nox Dec 22, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 22, 2018

⌛️ Testing commit 4b45958 with merge 46f5b8d...

bors-servo added a commit that referenced this pull request Dec 22, 2018

Auto merge of #22100 - Eijebong:once, r=jdm
Implement AddEventListenerOptions: once

Fixes #13242

<!-- 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/22100)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 22, 2018

💔 Test failed - status-taskcluster

@Eijebong Eijebong force-pushed the Eijebong:once branch from 4b45958 to 50c8327 Dec 22, 2018

@Eijebong

This comment has been minimized.

Copy link
Member

Eijebong commented Dec 22, 2018

Ran rustfmt

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 22, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 22, 2018

📌 Commit 50c8327 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 22, 2018

⌛️ Testing commit 50c8327 with merge cc0e7fd...

bors-servo added a commit that referenced this pull request Dec 22, 2018

Auto merge of #22100 - Eijebong:once, r=jdm
Implement AddEventListenerOptions: once

Fixes #13242

<!-- 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/22100)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 22, 2018

@bors-servo bors-servo merged commit 50c8327 into servo:master Dec 22, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment