Implement ExtendableEvent as base type for ServiceWorker events #13292

creativcoder · 2016-09-16T15:12:17Z

r? @jdm

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

These changes do not require tests because refactor

This change is

highfive · 2016-09-16T15:12:23Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/webidls/ExtendableEvent.webidl, components/script/dom/mod.rs, components/script/dom/webidls/ExtendableMessageEvent.webidl, components/script/dom/extendablemessageevent.rs, components/script/dom/extendableevent.rs, components/script/dom/serviceworkerglobalscope.rs

highfive · 2016-09-16T15:12:24Z

Warning

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

jdm · 2016-09-16T15:49:49Z

components/script/dom/serviceworkerglobalscope.rs

+
+    fn dispatch_activate(&self) {
+        let event = ExtendableEvent::new(GlobalRef::Worker(self.upcast::<WorkerGlobalScope>()),
+                                             atom!("activate"),


nit: indentation

jdm · 2016-09-16T15:49:49Z

components/script/dom/webidls/ExtendableEvent.webidl

+[Constructor(DOMString type,
+ optional ExtendableEventInit eventInitDict),
+ Exposed=ServiceWorker,
+ Pref="dom.serviceworker.enabled" ]


nit: remove the extra space before ]

jdm · 2016-09-16T15:49:49Z

components/script/dom/webidls/ExtendableMessageEvent.webidl

+
+[ Constructor(DOMString type, optional ExtendableMessageEventInit eventInitDict),
+  Exposed=ServiceWorker,
+  Pref="dom.serviceworker.enabled" ]


nit: remove the extra space after [ and before ]

jdm · 2016-09-16T15:49:49Z

components/script/dom/extendablemessageevent.rs

+impl ExtendableMessageEvent {
+    pub fn new_uninitialized(global: GlobalRef) -> Root<ExtendableMessageEvent> {
+        ExtendableMessageEvent::new_initialized(global,
+                                      HandleValue::undefined(),


nit: indentation

jdm · 2016-09-16T15:49:49Z

components/script/dom/extendableevent.rs

+            extensions_allowed: true
+        }
+    }
+    pub fn new_uninitialized(global: GlobalRef) -> Root<ExtendableEvent> {


I don't see any need to have the split between initialized/uninitialized. That's only for older event types that can be created in an uninitialized states.

jdm · 2016-09-16T15:49:49Z

components/script/dom/extendableevent.rs

+                            init.parent.cancelable))
+    }
+
+    fn init_extendable_event(&self,


Let's move the initialization into new directly.

jdm · 2016-09-16T15:49:49Z

components/script/dom/extendablemessageevent.rs

+                                      DOMString::new())
+    }
+
+    pub fn new_initialized(global: GlobalRef,


No need for the initialized/uninitialized split.

jdm · 2016-09-16T15:49:49Z

components/script/dom/extendableevent.rs

+    pub fn WaitUntil(&self, _cx: *mut JSContext, val: HandleValue) {
+        // Step 1
+        if !self.extensions_allowed {
+            // TODO throw invalid state error, but this does not return a `Fallible` ?


Add [Throws] to the webidl method.

jdm · 2016-09-16T15:49:49Z

components/script/dom/extendableevent.rs

+                   type_: DOMString,
+                   init: &ExtendableEventBinding::ExtendableEventInit) -> Fallible<Root<ExtendableEvent>> {
+        Ok(ExtendableEvent::new(global,
+                            Atom::from(type_),


nit: indentation.

jdm

The indentation in dispatch_activate is still incorrect.

jdm · 2016-09-16T16:38:23Z

components/script/dom/extendableevent.rs

+        let ev = reflect_dom_object(box ExtendableEvent::new_inherited(), global, ExtendableEventBinding::Wrap);
+        {
+            let ev = ev.upcast::<Event>();
+            if !ev.dispatching() {


No need for this; we can't be dispatching in the constructor.

jdm · 2016-09-16T16:38:23Z

components/script/dom/extendablemessageevent.rs

-                                      DOMString::new(),
-                                      DOMString::new())
-    }
-
    pub fn new_initialized(global: GlobalRef,


This can be called new.

jdm

We can include the update to string_cache now, since it's been published.

jdm · 2016-09-16T23:09:10Z

components/script/dom/extendableevent.rs

-        reflect_dom_object(box ExtendableEvent::new_inherited(),
-                           global,
-                           ExtendableEventBinding::Wrap)
-    }
    pub fn new(global: GlobalRef,
               type_: Atom,
               bubbles: bool,
               cancelable: bool)


These aren't used now, are they?

Yes, in event.init_event(). Made changes.

jdm · 2016-09-16T23:09:10Z

components/script/dom/extendableevent.rs

-        let ev = ExtendableEvent::new_uninitialized(global);
-        ev.init_extendable_event(type_, bubbles, cancelable);
-        ev
+        reflect_dom_object(box ExtendableEvent::new_inherited(), global, ExtendableEventBinding::Wrap)
    }

    pub fn Constructor(global: GlobalRef,
                   type_: DOMString,
                   init: &ExtendableEventBinding::ExtendableEventInit) -> Fallible<Root<ExtendableEvent>> {


nit: indentation.

jdm · 2016-09-16T23:09:10Z

components/script/dom/extendableevent.rs

    }

    // https://w3c.github.io/ServiceWorker/#wait-until-method
-    pub fn WaitUntil(&self, _cx: *mut JSContext, val: HandleValue) {
+    pub fn WaitUntil(&self, _cx: *mut JSContext, val: HandleValue) -> Fallible<()> {


We can use ErrorResult instead of Fallible<()>.

jdm · 2016-09-16T23:09:10Z

components/script/dom/extendablemessageevent.rs

        {
            let event = ev.upcast::<Event>();
            event.init_event(type_, bubbles, cancelable);
        }
-        ev
+        reflect_dom_object(ev, global, ExtendableMessageEventBinding::Wrap)


We should reflect the object as soon as possible after creating it, otherwise we risk GC-unsafety.

creativcoder · 2016-09-17T14:26:36Z

components/script/dom/extendablemessageevent.rs

@@ -38,11 +38,12 @@ impl ExtendableMessageEvent {
            lastEventId: lastEventId,
        };
        ev.data.set(data.get());
+        let ev = reflect_dom_object(ev, global, ExtendableMessageEventBinding::Wrap);
        {


@jdm Will this be safe place of invocation for reflect_dom_object ?

jdm · 2016-09-17T15:01:25Z

@bors-servo: delegate+
Please squash them all into one commit, then you can merge with r=jdm.

bors-servo · 2016-09-17T15:01:26Z

✌️ @creativcoder can now approve this pull request


        implement ExtendableEvent as base type for ServiceWorker events

creativcoder · 2016-09-17T15:33:00Z

@bors-servo r=jdm

bors-servo · 2016-09-17T15:33:01Z

📌 Commit 8b10cca has been approved by jdm

bors-servo · 2016-09-17T15:33:04Z

⌛ Testing commit 8b10cca with merge 83f687a...


        Auto merge of #13292 - creativcoder:extendable, r=jdm

@jdm

Implement ExtendableEvent as base type for ServiceWorker events  r? @jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [X] These changes do not require tests because refactor   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13292)

bors-servo · 2016-09-17T16:36:41Z

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

highfive assigned larsbergstrom Sep 16, 2016

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

creativcoder force-pushed the creativcoder:extendable branch from 69e6765 to 1b7e0ae Sep 16, 2016

jdm requested changes Sep 16, 2016

View changes

creativcoder force-pushed the creativcoder:extendable branch from 1b7e0ae to 55ee2db Sep 16, 2016

jdm assigned jdm and unassigned larsbergstrom Sep 16, 2016

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

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

creativcoder force-pushed the creativcoder:extendable branch from 58a9301 to c06c2b4 Sep 16, 2016

jdm requested changes Sep 16, 2016

View changes

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

creativcoder force-pushed the creativcoder:extendable branch from c06c2b4 to bddb13a Sep 16, 2016

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

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

jdm requested changes Sep 16, 2016

View changes

creativcoder force-pushed the creativcoder:extendable branch from bddb13a to 1533d3e Sep 17, 2016

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

creativcoder force-pushed the creativcoder:extendable branch from 1533d3e to 10d43c6 Sep 17, 2016

creativcoder reviewed Sep 17, 2016

View changes

jdm approved these changes Sep 17, 2016

View changes

jdm added the S-needs-squash label Sep 17, 2016

jdm removed the S-awaiting-review label Sep 17, 2016

implement ExtendableEvent as base type for ServiceWorker events

Loading status checks…

8b10cca

creativcoder force-pushed the creativcoder:extendable branch from 10d43c6 to 8b10cca Sep 17, 2016

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

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Sep 17, 2016

bors-servo merged commit 8b10cca into servo:master Sep 17, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

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

servo / servo

Implement ExtendableEvent as base type for ServiceWorker events #13292

Implement ExtendableEvent as base type for ServiceWorker events #13292

creativcoder commented Sep 16, 2016 •

edited

highfive commented Sep 16, 2016

highfive commented Sep 16, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Sep 17, 2016

bors-servo commented Sep 17, 2016

creativcoder commented Sep 17, 2016

bors-servo commented Sep 17, 2016

bors-servo commented Sep 17, 2016

bors-servo commented Sep 17, 2016

servo / servo

Join GitHub today

Implement ExtendableEvent as base type for ServiceWorker events #13292

Implement ExtendableEvent as base type for ServiceWorker events #13292

Conversation

creativcoder commented Sep 16, 2016 • edited

highfive commented Sep 16, 2016

highfive commented Sep 16, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

creativcoder Sep 17, 2016 • edited

This comment has been minimized.

jdm commented Sep 17, 2016

bors-servo commented Sep 17, 2016

creativcoder commented Sep 17, 2016

bors-servo commented Sep 17, 2016

bors-servo commented Sep 17, 2016

bors-servo commented Sep 17, 2016

creativcoder commented Sep 16, 2016 •

edited

creativcoder Sep 17, 2016 •

edited