servo / 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 #15803

Closed

GuillaumeGomez wants to merge 1 commit into servo:master from unknown repository

+36 −10

Contributor

GuillaumeGomez commented Mar 2, 2017 •

edited by larsbergstrom

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

This change is

highfive assigned pcwalton

highfive commented Mar 2, 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 commented Mar 2, 2017

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

highfive added the S-awaiting-review label

Member

nox commented Mar 3, 2017

It's not. In #12952 (comment) @Ms2ger mentions we only want to support the capture key for now, given we don't implement the features related to the other keys.

nox added S-needs-code-changes and removed S-awaiting-review labels

nox assigned nox and unassigned pcwalton

Contributor Author

GuillaumeGomez commented Mar 3, 2017

So you want me to comment all ::Boolean cases for now?

Member

nox commented Mar 9, 2017

I don't understand the question.

I want:

dictionary EventListenerOptions {
  boolean capture = false;
};

dictionary AddEventListenerOptions : EventListenerOptions {
  // boolean passive = false;
  // boolean once = false;
};

Contributor Author

GuillaumeGomez commented Mar 9, 2017

Ok, makes more sense. I thought you were only speaking of the usage, not the declaration.

Contributor

Ms2ger commented Mar 15, 2017

@GuillaumeGomez ping

Contributor Author

GuillaumeGomez commented Mar 15, 2017

Sorry, was busy because of rustdoc. Will finish it this week-end.

highfive added S-awaiting-review and removed S-needs-code-changes labels

Contributor Author

GuillaumeGomez commented Mar 18, 2017

Updated.

nox reviewed

components/script/dom/eventtarget.rs Outdated

  
            capture: bool,

            passive: bool,

            once: bool,

            removed: bool,

nox Mar 19, 2017

Member

Don't add fields for which we don't support the corresponding thing, so just add capture.

nox added S-needs-code-changes and removed S-awaiting-review labels

highfive added S-awaiting-review and removed S-needs-code-changes labels

Contributor Author

GuillaumeGomez commented Mar 19, 2017

Updated.

nox requested changes

components/script/dom/eventtarget.rs Outdated

  
            capture: bool,

            // passive: bool,

            // once: bool,

            // removed: bool,

nox Mar 21, 2017

Member

Remove that.

components/script/dom/eventtarget.rs Outdated

  
                                capture: false,

                                // passive: false,

                                // once: false,

                                // removed: false,

nox Mar 21, 2017

Member

Remove that.

components/script/dom/eventtarget.rs Outdated

  
                            (false, false, false)

                        }

                        AddEventListenerOptionsOrBoolean::Boolean(o) => (o, false, false),

                    };

nox Mar 21, 2017

Member

let capture = match options {
    AddEventListenerOptionsOrBoolean::AddEventListenerOptions(options) => options.capture,
    AddEventListenerOptionsOrBoolean::Boolean(capture) => capture,
};

components/script/dom/eventtarget.rs Outdated

  
                    Vacant(entry) => entry.insert(EventListeners(vec!())),

                };

                                options: AddEventListenerOptionsOrBoolean) {

                if let Some(listener) = listener {

nox Mar 21, 2017

Member

Why that change? Please early return just like before.

components/script/dom/eventtarget.rs Outdated

  
                        capture: capture,

                        // passive: passive,

                        // once: once,

                        // removed: false,

nox Mar 21, 2017

Member

Remove.

components/script/dom/eventtarget.rs Outdated

  
                        phase: phase,

                        listener: EventListenerType::Additive(listener.clone())

                                   options: EventListenerOptionsOrBoolean) {

                if let Some(ref listener) = listener {

nox Mar 21, 2017

Member

Early return just like before.

components/script/dom/eventtarget.rs Outdated

  
      @@ -225,7 +228,11 @@ impl CompiledEventListener {
    
        /// A listener in a collection of event listeners.

        struct EventListenerEntry {

            phase: ListenerPhase,

            listener: EventListenerType

            listener: EventListenerType,

            capture: bool,

nox Mar 21, 2017

Member

This new field is useless for now, given phase already covers it, please remove.

components/script/dom/eventtarget.rs Outdated

  
                            capture: capture,

                            // passive: false,

                            // once: false,

                            // removed: false,

nox Mar 21, 2017

Member

Remove that.

nox added S-needs-code-changes and removed S-awaiting-review labels


        Implement EventListenerOptions for EventTarget

078931a

highfive added S-awaiting-review and removed S-needs-code-changes labels

Contributor Author

GuillaumeGomez commented Mar 22, 2017

Updated.

Member

nox commented Mar 24, 2017

@bors-servo try

Contributor

bors-servo commented Mar 24, 2017

⌛ Trying commit 078931a with merge a8afb0c...

bors-servo added a commit that referenced this pull request


        Auto merge of #15803 - GuillaumeGomez:event_listener_options, r=<try>

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

nox approved these changes

nox removed the S-awaiting-review label

Contributor

bors-servo commented Mar 24, 2017

💔 Test failed - linux-rel-css

highfive added the S-tests-failed label

Member

jdm commented Mar 24, 2017

All tests are failing with errors like:

  │ ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/resources/testharness.js:663:9 Value not an object.
  │ ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/resources/testharnessreport.js:8:1 setup is not defined
  └ ERROR:script::dom::bindings::error: Error at http://web-platform.test:8000/css-flexbox-1_dev/html/flexbox_computedstyle_flex-direction-column-reverse.htm:16:1 test is not defined

Member

jdm commented Apr 12, 2017

@GuillaumeGomez Are you planning to return to this?

Contributor Author

GuillaumeGomez commented Apr 12, 2017

Yes, I'm currently working on it (slowly but still).

Contributor Author

GuillaumeGomez commented Apr 13, 2017

Actually I'm completely stuck in here and I have no idea what change broke everything. If anyone could help, it'd very appreciated!

Contributor

bors-servo commented Jul 21, 2017

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

highfive added the S-needs-rebase label

Member

nox commented Sep 24, 2017

Superseded by #18612. Thanks for your contribution!

nox closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

You can’t perform that action at this time.