Return an enum instead of a boolean from dispatch_event #13397

aochagavia · 2016-09-23T15:55:16Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13196.
These changes do not require tests because the functionality hasn't changed

This change is

highfive · 2016-09-23T15:55:24Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

highfive · 2016-09-23T15:55:24Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/worker.rs, components/script/dom/document.rs, components/script/dom/eventtarget.rs, components/script/dom/eventdispatcher.rs, components/script/dom/event.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/htmlscriptelement.rs

highfive · 2016-09-23T15:55:25Z

Warning

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

aochagavia · 2016-09-23T15:56:29Z

An unresolved question is whether handle_touch_event should use EventStatus as the return value instead of bool. The function has some early returns that seem unrelated to Event::fire, so I am unsure if they all mean the same. The current implementation converts the EventStatus back into a bool.

aochagavia · 2016-09-23T17:36:00Z

There is something wrong with the (Windows, GNU) build, but it seems unrelated to this PR

aochagavia · 2016-09-23T17:36:35Z

r? @Ms2ger

KiChjang · 2016-09-23T18:35:58Z

components/script/dom/eventdispatcher.rs

@@ -103,10 +103,15 @@ fn dispatch_to_listeners(event: &Event, target: &EventTarget, event_path: &[&Eve
    }
 }

+pub enum EventStatus {


You can derive PartialEq here so that you don't need if let to match for it.

I actually like if let, but if you think it is necessary I could change the code to use ==

I agree with @KiChjang here; please do use == rather than if let.

Ms2ger

Looking good, just needs a few small changes.

Ms2ger · 2016-09-26T11:32:18Z

components/script/dom/eventdispatcher.rs

@@ -103,10 +103,15 @@ fn dispatch_to_listeners(event: &Event, target: &EventTarget, event_path: &[&Eve
    }
 }

+pub enum EventStatus {


I agree with @KiChjang here; please do use == rather than if let.

Ms2ger · 2016-09-26T11:32:18Z

components/script/dom/eventdispatcher.rs

-    // Step 13.
-    !event.DefaultPrevented()
+    // Step 14.
+    match event.DefaultPrevented() {


I would suggest adding a method on Event that returned this.

Ms2ger · 2016-09-26T11:32:18Z

components/script/dom/eventtarget.rs

@@ -560,7 +560,10 @@ impl EventTargetMethods for EventTarget {
            return Err(Error::InvalidState);
        }
        event.set_trusted(false);
-        Ok(self.dispatch_event(event))
+        match self.dispatch_event(event) {


Nit: you can use Ok(match ...).

aochagavia · 2016-09-26T13:43:48Z

@Ms2ger Thanks for your feedback! I have pushed the changes.

Ms2ger · 2016-09-26T14:21:40Z

components/script/dom/event.rs

@@ -153,6 +154,14 @@ impl Event {
        self.cancelable.set(cancelable);
    }

+    pub fn status(&self) -> EventStatus
+    {


{ should go on the previous line

Nice catch... Too much C# programming these days...

Ms2ger · 2016-09-26T14:21:40Z

components/script/dom/dedicatedworkerglobalscope.rs

+        let event_status = event.upcast::<Event>().fire(self.upcast::<EventTarget>());
+
+        // Step 15
+        if EventStatus::NotCanceled == event_status {


If you want to swap around the left-hand and right-hand sides of these comparisons, I think that would read better. r+ either way, if you fix the other comment below.


        Return an enum instead of a boolean from dispatch_event

Fixes #13196

aochagavia · 2016-09-26T15:59:34Z

@Ms2ger I pushed new changes, including swapping the comparisons.

Ms2ger · 2016-09-26T16:01:55Z

Thanks a lot!

@bors-servo r+

bors-servo · 2016-09-26T16:01:55Z

📌 Commit 6c8bfdb has been approved by Ms2ger

bors-servo · 2016-09-26T17:09:49Z

⌛ Testing commit 6c8bfdb with merge 9b77080...


        Auto merge of #13397 - aochagavia:dispatch-event, r=Ms2ger

Return an enum instead of a boolean from dispatch_event Fixes #13196 --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13196. - [X] These changes do not require tests because the functionality hasn't changed  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13397)

bors-servo · 2016-09-26T18:37:44Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned emilio Sep 23, 2016

highfive added the S-awaiting-review label Sep 23, 2016

aochagavia mentioned this pull request Sep 23, 2016

Return an enum instead of a boolean from dispatch_event #13196

Closed

highfive assigned Ms2ger and unassigned emilio Sep 23, 2016

KiChjang reviewed Sep 23, 2016

View changes

Ms2ger requested changes Sep 26, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 26, 2016

Return an enum instead of a boolean from dispatch_event

Loading status checks…

6c8bfdb

Fixes #13196

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 26, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 26, 2016

bors-servo merged commit 6c8bfdb into servo:master Sep 26, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 26, 2016

servo / servo

Return an enum instead of a boolean from dispatch_event #13397

Return an enum instead of a boolean from dispatch_event #13397

aochagavia commented Sep 23, 2016 •

edited by larsbergstrom

highfive commented Sep 23, 2016

highfive commented Sep 23, 2016

highfive commented Sep 23, 2016

aochagavia commented Sep 23, 2016

aochagavia commented Sep 23, 2016

aochagavia commented Sep 23, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Ms2ger left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aochagavia commented Sep 26, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aochagavia commented Sep 26, 2016

Ms2ger commented Sep 26, 2016

bors-servo commented Sep 26, 2016

bors-servo commented Sep 26, 2016

bors-servo commented Sep 26, 2016

servo / servo

Join GitHub today

Return an enum instead of a boolean from dispatch_event #13397

Return an enum instead of a boolean from dispatch_event #13397

Conversation

aochagavia commented Sep 23, 2016 • edited by larsbergstrom

highfive commented Sep 23, 2016

highfive commented Sep 23, 2016

highfive commented Sep 23, 2016

aochagavia commented Sep 23, 2016

aochagavia commented Sep 23, 2016

aochagavia commented Sep 23, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Ms2ger left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aochagavia commented Sep 26, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aochagavia commented Sep 26, 2016

Ms2ger commented Sep 26, 2016

bors-servo commented Sep 26, 2016

bors-servo commented Sep 26, 2016

bors-servo commented Sep 26, 2016

aochagavia commented Sep 23, 2016 •

edited by larsbergstrom