Implement custom element registry #17112

cbrewster · 2017-05-31T16:15:44Z

Implements https://html.spec.whatwg.org/multipage/#customelementregistry

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

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-05-31T16:15:48Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/mod.rs, components/script/dom/customelementregistry.rs, components/script/dom/webidls/CustomElementRegistry.webidl, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl
@KiChjang: components/script/dom/mod.rs, components/script/dom/customelementregistry.rs, components/script/dom/webidls/CustomElementRegistry.webidl, components/script/dom/window.rs, components/script/dom/webidls/Window.webidl

highfive · 2017-05-31T16:15:49Z

Warning

These commits modify unsafe code. Please review it carefully!

cbrewster · 2017-05-31T16:16:26Z

r? @jdm

part of #9372

cbrewster · 2017-05-31T16:18:51Z

components/script/dom/customelementregistry.rs

+                }
+
+                // TODO: 7.2
+                // Check that the element interface for extends is not HTMLUnknownElement


I'm not entirely sure the best way to go about this. AFAICT we only determine the element interface here: https://dxr.mozilla.org/servo/rev/779edd7c4aaf900f12fab378a378b0fc52d4c628/components/script/dom/create.rs#129-272.

Yeah, you need a similar match expression for this step to be implemented.

Indeed. I am trying to think of a way that we could reuse the match here so we don't need to maintain two matches, but I haven't thought of anything yet. How often is that match changed?

nox

First review pass.

nox · 2017-06-02T08:49:06Z

components/script/dom/customelementregistry.rs

+        rooted!(in(global_scope.get_cx()) let mut prototype = UndefinedValue());
+        unsafe {
+            // Step 10.1
+            let c_name = CString::new("prototype").unwrap();


You don't need this here, just use b"prototype\0".

I think we have to use CString here. JS_GetProperty expects a *const i8 and b"prototype\0".as_ptr() gets a *const u8

Other places use b"foo\0".as_ptr() as *const _.

Ok, will change it to that

nox · 2017-06-02T08:49:06Z

components/script/dom/customelementregistry.rs

+    // https://html.spec.whatwg.org/multipage/#dom-customelementregistry-define
+    // Steps 10.1, 10.2
+    #[allow(unsafe_code)]
+    fn check_prototype(&self, constructor: HandleObject) -> ErrorResult {


Why make a separate method?

Step 10.1 (The second 10.1, which unsets the element definition is running) By making it a function I can return early without worrying about not unsetting the flag.

nox · 2017-06-02T08:49:06Z

components/script/dom/customelementregistry.rs

+
+            // Step 10.2
+            if !prototype.is_object() {
+                return Err(Error::Type("constructor.protoype is not an object".to_owned()));


Typo: prototype.

nox · 2017-06-02T08:49:06Z

components/script/dom/customelementregistry.rs

+}
+
+impl CustomElementRegistryMethods for CustomElementRegistry {
+    #[allow(unsafe_code, unrooted_must_root)]


Why is unrooted_must_root required?

We have to allow it in order to use Promises.

nox · 2017-06-02T08:49:06Z

components/script/dom/customelementregistry.rs

+
+        // Step 1
+        if unsafe { !IsConstructor(constructor.get()) } {
+            return Err(Error::Type("Second argument of CustomElementRegistry.defin is not a constructor".to_owned()));


Typo: define.

nox · 2017-06-02T08:49:06Z

components/script/dom/customelementregistry.rs

+
+    // https://html.spec.whatwg.org/multipage/#dom-customelementregistry-whendefined
+    #[allow(unrooted_must_root)]
+    fn WhenDefined(&self, name: DOMString) -> Fallible<Rc<Promise>> {


This method isn't actually fallible.

nox · 2017-06-02T08:49:06Z

components/script/dom/customelementregistry.rs

+        let mut map = self.when_defined.borrow_mut();
+
+        // Steps 4, 5
+        let promise = match map.get(&name).cloned() {


Using Option::unwrap_or_else would be cleaner.

nox · 2017-06-02T08:49:06Z

components/script/dom/customelementregistry.rs

+        }
+    } else {
+        return false;
+    }


if !name.chars().next().map_or(false, |c| c >= 'a' && c >= 'z') { return false; }

nox · 2017-06-02T08:49:06Z

components/script/dom/customelementregistry.rs

+
+        // Check if this character is not a PCENChar
+        // https://html.spec.whatwg.org/multipage/#prod-pcenchar
+        if c != '-' && c != '.' && c != '_' && c != '\u{B7}' &&


Please make a is_potential_custom_element_char function and use that here, all the reversed conditions don't help readability.

nox · 2017-06-02T08:49:06Z

components/script/dom/customelementregistry.rs

+
+    for c in name.chars() {
+        if c == '-' {
+            has_dash = true;


Nit: you can continue here.

cbrewster

Thanks for the review @nox!

cbrewster · 2017-06-02T17:06:55Z

components/script/dom/customelementregistry.rs

+    // https://html.spec.whatwg.org/multipage/#dom-customelementregistry-define
+    // Steps 10.1, 10.2
+    #[allow(unsafe_code)]
+    fn check_prototype(&self, constructor: HandleObject) -> ErrorResult {


Step 10.1 (The second 10.1, which unsets the element definition is running) By making it a function I can return early without worrying about not unsetting the flag.

cbrewster · 2017-06-02T17:06:55Z

components/script/dom/customelementregistry.rs

+        rooted!(in(global_scope.get_cx()) let mut prototype = UndefinedValue());
+        unsafe {
+            // Step 10.1
+            let c_name = CString::new("prototype").unwrap();


I think we have to use CString here. JS_GetProperty expects a *const i8 and b"prototype\0".as_ptr() gets a *const u8

cbrewster · 2017-06-02T17:06:55Z

components/script/dom/customelementregistry.rs

+
+            // Step 10.2
+            if !prototype.is_object() {
+                return Err(Error::Type("constructor.protoype is not an object".to_owned()));


cbrewster · 2017-06-02T17:06:55Z

components/script/dom/customelementregistry.rs

+}
+
+impl CustomElementRegistryMethods for CustomElementRegistry {
+    #[allow(unsafe_code, unrooted_must_root)]


We have to allow it in order to use Promises.

cbrewster · 2017-06-02T17:06:55Z

components/script/dom/customelementregistry.rs

+
+        // Step 1
+        if unsafe { !IsConstructor(constructor.get()) } {
+            return Err(Error::Type("Second argument of CustomElementRegistry.defin is not a constructor".to_owned()));


cbrewster · 2017-06-02T17:06:55Z

components/script/dom/customelementregistry.rs

+
+        // Step 1
+        if !is_valid_custom_element_name(&name) {
+            return Err(Error::Syntax);


Woops! misread spec. fixed.

cbrewster · 2017-06-02T17:06:55Z

components/script/dom/customelementregistry.rs

+        let mut map = self.when_defined.borrow_mut();
+
+        // Steps 4, 5
+        let promise = match map.get(&name).cloned() {


cbrewster · 2017-06-02T17:06:55Z

components/script/dom/customelementregistry.rs

+        }
+    } else {
+        return false;
+    }


cbrewster · 2017-06-02T17:06:55Z

components/script/dom/customelementregistry.rs

+
+    for c in name.chars() {
+        if c == '-' {
+            has_dash = true;


cbrewster · 2017-06-02T17:06:55Z

components/script/dom/customelementregistry.rs

+
+        // Check if this character is not a PCENChar
+        // https://html.spec.whatwg.org/multipage/#prod-pcenchar
+        if c != '-' && c != '.' && c != '_' && c != '\u{B7}' &&


jdm · 2017-06-02T19:46:31Z

tests/wpt/metadata/custom-elements/CustomElementRegistry.html.ini

-
-  [customElements.whenDefined must return an unresolved promise when the registry does not contain the entry with the given name]
-    expected: FAIL
-
  [customElements.whenDefined must return a rejected promise when the given name is not a valid custom element name]


I suspect this test passes now.

jdm

This is a really readable implementation!

jdm · 2017-06-02T22:19:44Z

components/script/dom/customelementregistry.rs

+        }
+
+        if !is_potential_custom_element_char(c)
+        {


nit: { on the previous line.

jdm · 2017-06-02T22:19:44Z

components/script/dom/customelementregistry.rs

+
+    for c in chars {
+        if c == '-' {
+            has_dash = true;


Should we return false if has_dash is already true?

The name can have more than one dash, it just needs at a minimum one.

jdm · 2017-06-02T22:19:44Z

components/script/dom/customelementregistry.rs

+    // https://html.spec.whatwg.org/multipage/#dom-customelementregistry-get
+    #[allow(unsafe_code)]
+    unsafe fn Get(&self, cx: *mut JSContext, name: DOMString) -> JSVal {
+        match self.definitions.borrow_mut().get(&name) {


borrow(), right?

jdm · 2017-06-02T22:19:44Z

components/script/dom/customelementregistry.rs

+            }
+
+            // TODO: 7.2
+            // Check that the element interface for extends is not HTMLUnknownElement


I don't see any way around it but duplicating the relevant data from create.rs, unfortunately.

jdm · 2017-06-02T22:23:39Z

tests/wpt/metadata/custom-elements/CustomElementRegistry.html.ini

-
-  [customElements.define unset the element definition is running flag before upgrading custom elements]
-    expected: FAIL
-
  [customElements.define must not throw when defining another custom element in a different global object during Get(constructor, "prototype")]


Can we look into why this test fails?

The test relies on createElement creating a custom element which isn't implemented yet:

assert_true(iframe.contentDocument.createElement('another-custom-element') instanceof InnerCustomElement);

cbrewster · 2017-06-03T03:32:33Z

components/script/dom/customelementregistry.rs

+    // https://html.spec.whatwg.org/multipage/#dom-customelementregistry-get
+    #[allow(unsafe_code)]
+    unsafe fn Get(&self, cx: *mut JSContext, name: DOMString) -> JSVal {
+        match self.definitions.borrow_mut().get(&name) {


cbrewster · 2017-06-03T03:32:33Z

components/script/dom/customelementregistry.rs

+
+    for c in chars {
+        if c == '-' {
+            has_dash = true;


The name can have more than one dash, it just needs at a minimum one.

cbrewster · 2017-06-03T03:32:33Z

components/script/dom/customelementregistry.rs

+        }
+
+        if !is_potential_custom_element_char(c)
+        {


cbrewster · 2017-06-03T03:32:33Z

tests/wpt/metadata/custom-elements/CustomElementRegistry.html.ini

-
-  [customElements.define unset the element definition is running flag before upgrading custom elements]
-    expected: FAIL
-
  [customElements.define must not throw when defining another custom element in a different global object during Get(constructor, "prototype")]


The test relies on createElement creating a custom element which isn't implemented yet:

assert_true(iframe.contentDocument.createElement('another-custom-element') instanceof InnerCustomElement);

cbrewster · 2017-06-03T03:32:33Z

tests/wpt/metadata/custom-elements/custom-element-registry/define.html.ini

@@ -1,5 +1,53 @@
 [define.html]
  type: testharness
-  [Custom Elements: Element definition]
+  [If constructor is arrow function, should throw a TypeError]


I am not sure how I should check for this, is there anything other than IsConstructor that I need to do?

I suspect it's a limitation of our old version of Spider monkey. It would be worth seeing if Gecko passes that test.

Ahh probably, Gecko does pass.

jdm · 2017-06-05T03:28:59Z

@bors-servo: r+

bors-servo · 2017-06-05T03:29:00Z

📌 Commit 5bcf748 has been approved by jdm

bors-servo · 2017-06-05T03:42:50Z

⌛ Testing commit 5bcf748 with merge fccaefa...


        Auto merge of #17112 - cbrewster:custom_element_registry, r=jdm

Implement custom element registry  Implements https://html.spec.whatwg.org/multipage/#customelementregistry ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #16753 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17112)

bors-servo · 2017-06-05T04:41:04Z

💔 Test failed - linux-rel-wpt

jdm · 2017-06-05T12:16:41Z

You should add a Preference annotation to the CustomElementRegistry interface as well.


        Add custom element registry


        Fix custom element registry define test

The test relies on DOM elements with ids beind added as properties of the window. Servo does not do this, and this is not a best practice.


        Update test expectations

cbrewster · 2017-06-05T15:19:56Z

@bors-servo r=jdm

bors-servo · 2017-06-05T15:19:57Z

📌 Commit 87bf4ca has been approved by jdm

bors-servo · 2017-06-05T15:38:05Z

⌛ Testing commit 87bf4ca with merge b584944...


        Auto merge of #17112 - cbrewster:custom_element_registry, r=jdm

Implement custom element registry  Implements https://html.spec.whatwg.org/multipage/#customelementregistry ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #16753 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17112)

bors-servo · 2017-06-05T17:06:51Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing b584944 to master...


        Fix custom element registry define test

The test relies on DOM elements with ids beind added as properties of the window. Servo does not do this, and this is not a best practice. Upstreamed from servo/servo#17112 [ci skip]

highfive assigned metajack May 31, 2017

highfive added the S-awaiting-review label May 31, 2017

highfive assigned jdm and unassigned metajack May 31, 2017

cbrewster reviewed May 31, 2017

View changes

cbrewster force-pushed the cbrewster:custom_element_registry branch from d022726 to 96d3193 Jun 1, 2017

nox requested changes Jun 2, 2017

View changes

nox added S-needs-code-changes and removed S-awaiting-review labels Jun 2, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 2, 2017

cbrewster reviewed Jun 2, 2017

View changes

jdm reviewed Jun 2, 2017

View changes

cbrewster force-pushed the cbrewster:custom_element_registry branch 2 times, most recently from 666f935 to e233823 Jun 2, 2017

jdm requested changes Jun 2, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jun 2, 2017

jdm reviewed Jun 2, 2017

View changes

cbrewster force-pushed the cbrewster:custom_element_registry branch from e233823 to 5bcf748 Jun 3, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Jun 3, 2017

cbrewster reviewed Jun 3, 2017

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 5, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 5, 2017

cbrewster added 3 commits May 11, 2017

Add custom element registry

e21e64a

Fix custom element registry define test

1800da7

The test relies on DOM elements with ids beind added as properties of the window. Servo does not do this, and this is not a best practice.

Update test expectations

Loading status checks…

87bf4ca

cbrewster force-pushed the cbrewster:custom_element_registry branch from 5bcf748 to 87bf4ca Jun 5, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Jun 5, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 5, 2017

bors-servo merged commit 87bf4ca into servo:master Jun 5, 2017
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 Jun 5, 2017

jdm mentioned this pull request Jun 19, 2017

Fix custom element registry define test web-platform-tests/wpt#6283

Merged

servo / servo

Join GitHub today

Implement custom element registry #17112

Implement custom element registry #17112

Conversation

cbrewster commented May 31, 2017 • edited

highfive commented May 31, 2017

highfive commented May 31, 2017

cbrewster commented May 31, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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.

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.

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

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.

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 commented Jun 5, 2017

bors-servo commented Jun 5, 2017

bors-servo commented Jun 5, 2017

bors-servo commented Jun 5, 2017

jdm commented Jun 5, 2017

cbrewster commented Jun 5, 2017

bors-servo commented Jun 5, 2017

bors-servo commented Jun 5, 2017

bors-servo commented Jun 5, 2017

cbrewster commented May 31, 2017 •

edited