specialized constructors for singly-exposed DOM types #14160

ajnirp · 2016-11-10T07:31:32Z

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

There are tests for these changes OR
These changes do not require tests because these are codegen changes and the generated code builds successfully

This change is

highfive · 2016-11-10T07:31:36Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/comment.rs, components/script/dom/webglcontextevent.rs, components/script/dom/dommatrixreadonly.rs, components/script/dom/dommatrix.rs, components/script/dom/document.rs, components/script/dom/documentfragment.rs, components/script/dom/text.rs, components/script/dom/keyboardevent.rs, components/script/dom/range.rs, components/script/dom/focusevent.rs, components/script/dom/extendablemessageevent.rs, components/script/dom/domparser.rs, components/script/dom/htmlimageelement.rs, components/script/dom/mouseevent.rs, components/script/dom/transitionevent.rs, components/script/dom/extendableevent.rs, components/script/dom/uievent.rs
@fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/comment.rs, components/script/dom/webglcontextevent.rs, components/script/dom/dommatrixreadonly.rs, components/script/dom/dommatrix.rs, components/script/dom/document.rs, components/script/dom/documentfragment.rs, components/script/dom/text.rs, components/script/dom/keyboardevent.rs, components/script/dom/range.rs, components/script/dom/focusevent.rs, components/script/dom/extendablemessageevent.rs, components/script/dom/domparser.rs, components/script/dom/htmlimageelement.rs, components/script/dom/mouseevent.rs, components/script/dom/transitionevent.rs, components/script/dom/extendableevent.rs, components/script/dom/uievent.rs
@emilio: components/script/dom/webglcontextevent.rs

highfive · 2016-11-10T07:31:37Z

Warning

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

nox

Thanks for your contribution!

nox · 2016-11-10T10:45:13Z

components/script/dom/bindings/codegen/CodegenRust.py

+            globalName = list(self.exposureSet)[0]
+            downcast = """let global = global.downcast::<dom::{}::{}>().unwrap();\n"""
+            downcast = downcast.format(globalName.lower(),
+                                       globalName[0].upper() + globalName[1:])


Could you explain what these two lines are supposed to do? Also you may want to look at MakeNativeName in Configuration.py.

This is for the case when a DOM type is exposed to exactly one global. In that case, we downcast global from GlobalScope to that particular struct.

Thank you for pointing out MakeNativeName! I have fixed it.

nox · 2016-11-10T10:45:13Z

components/script/dom/bindings/codegen/CodegenRust.py

@@ -3170,7 +3170,7 @@ def __init__(self, errorResult, arguments, argsPre, returnType,

        if isFallible:
            if static:
-                glob = "&global"
+                glob = "global.upcast::<GlobalScope>()"


Why is this needed?

It's needed to prevent compile errors in the call to throw_dom_exception that gets generated from CGCallGenerator::__init__(). Without this change, we end up passing a reference to a GlobalScope-derived struct as the 2nd parameter to throw_dom_exception whereas it expects to see a &GlobalScope.

nox · 2016-11-10T10:45:13Z

components/script/dom/webglcontextevent.rs

@@ -82,6 +83,8 @@ impl WebGLContextEvent {

        let cancelable = EventCancelable::from(init.parent.cancelable);

+        let global = window.upcast::<GlobalScope>();


WebGLContextEvent::new should probably be made to take a &Window too.

nox · 2016-11-10T10:45:13Z

components/script/dom/extendableevent.rs

                       type_: DOMString,
                       init: &ExtendableEventBinding::ExtendableEventInit) -> Fallible<Root<ExtendableEvent>> {
+        let global = worker.upcast::<GlobalScope>();


ExtendableEvent::new should take a &Window.

I think this should be &ServiceWorkerGlobalScope, based on ExtendableEvent.webidl Updated!

nox · 2016-11-10T10:45:13Z

components/script/dom/extendablemessageevent.rs

                       type_: DOMString,
                       init: &ExtendableMessageEventBinding::ExtendableMessageEventInit)
                       -> Fallible<Root<ExtendableMessageEvent>> {
+        let global = worker.upcast::<GlobalScope>();


ExtendableMessageEvent::new should take a &Window.

Likewise, I think this should be &ServiceWorkerGlobalScope as well. Updated!

Oh right! :)

nox · 2016-11-10T10:45:13Z

components/script/dom/dommatrixreadonly.rs

@@ -40,12 +42,14 @@ impl DOMMatrixReadOnly {
    }

    // https://drafts.fxtf.org/geometry-1/#dom-dommatrixreadonly-dommatrixreadonly
-    pub fn Constructor(global: &GlobalScope) -> Fallible<Root<Self>> {
+    pub fn Constructor(window: &Window) -> Fallible<Root<Self>> {


DOMMatrix and friends are supposed to be Exposed=(Window,Worker).

To address this, I uncommented the Exposed=(Window,Worker) lines in the WebIDLs for DOMMatrix{ReadOnly} and removed the up/down casting in their .rs files. Is this okay?

nox · 2016-11-11T13:23:31Z

components/script/dom/bindings/codegen/CodegenRust.py

+        preamble = """let global = GlobalScope::from_object(JS_CALLEE(cx, vp).to_object());\n"""
+        if len(self.exposureSet) == 1:
+            globalName = list(self.exposureSet)[0]
+            downcast = """let global = global.downcast::<dom::{}::{}>().unwrap();\n"""


I think you can just do:

let downcast = "global.downcast::<%s>().unwrap();" % descriptor.path

nox · 2016-11-11T13:23:31Z

components/script/dom/extendablemessageevent.rs

                       type_: DOMString,
                       init: &ExtendableMessageEventBinding::ExtendableMessageEventInit)
                       -> Fallible<Root<ExtendableMessageEvent>> {
+        let global = worker.upcast::<GlobalScope>();


Oh right! :)

nox · 2016-11-11T13:23:31Z

components/script/dom/webidls/DOMMatrix.webidl

-Constructor(sequence<unrestricted double> numberSequence)
-// Exposed=(Window,Worker)
+Constructor(sequence<unrestricted double> numberSequence),
+Exposed=(Window,Worker)


Could you do that in a first separate commit?

bors-servo · 2016-11-14T17:18:55Z

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


        specialized constructors for singly-exposed DOM types

nox · 2016-11-27T13:39:54Z

Carried in #14376, thanks for your contribution!

highfive assigned asajeffrey Nov 10, 2016

highfive added the S-awaiting-review label Nov 10, 2016

ajnirp mentioned this pull request Nov 10, 2016

Specialize constructors for DOM types that are only exposed to a single global #14071

Closed

nox requested changes Nov 10, 2016

View changes

nox assigned nox and unassigned asajeffrey Nov 10, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 10, 2016

ajnirp force-pushed the ajnirp:14071-specialized-constructors branch from 1b0a6c7 to 1736be7 Nov 11, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 11, 2016

nox requested changes Nov 11, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 11, 2016

highfive added the S-needs-rebase label Nov 14, 2016

ajnirp force-pushed the ajnirp:14071-specialized-constructors branch from 1736be7 to f0c0d6a Nov 23, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 23, 2016

specialized constructors for singly-exposed DOM types

Loading status checks…

c1a6e02

ajnirp force-pushed the ajnirp:14071-specialized-constructors branch from f0c0d6a to c1a6e02 Nov 24, 2016

nox closed this Nov 27, 2016

servo / servo

specialized constructors for singly-exposed DOM types #14160

specialized constructors for singly-exposed DOM types #14160

ajnirp commented Nov 10, 2016 •

edited by larsbergstrom

highfive commented Nov 10, 2016

highfive commented Nov 10, 2016

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

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.

bors-servo commented Nov 14, 2016

nox commented Nov 27, 2016

		@@ -82,6 +83,8 @@ impl WebGLContextEvent {

		let cancelable = EventCancelable::from(init.parent.cancelable);

		let global = window.upcast::<GlobalScope>();

servo / servo

Join GitHub today

specialized constructors for singly-exposed DOM types #14160

specialized constructors for singly-exposed DOM types #14160

Conversation

ajnirp commented Nov 10, 2016 • edited by larsbergstrom

highfive commented Nov 10, 2016

highfive commented Nov 10, 2016

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

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.

bors-servo commented Nov 14, 2016

nox commented Nov 27, 2016

ajnirp commented Nov 10, 2016 •

edited by larsbergstrom