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

Custom element upgrades #17935

Merged

bors-servo merged 10 commits into servo:master from cbrewster:ce_upgrades

Aug 9, 2017

Member

cbrewster commented Aug 1, 2017 •

edited by SimonSapin

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

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

This change is

highfive assigned Manishearth

highfive commented Aug 1, 2017

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/customelementregistry.rs, components/script/dom/document.rs, components/script/dom/element.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/create.rs and 4 more
@KiChjang: components/script/dom/customelementregistry.rs, components/script/dom/document.rs, components/script/dom/element.rs, components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/create.rs and 4 more

highfive commented Aug 1, 2017

Warning

These commits modify unsafe code. Please review it carefully!

highfive added the S-awaiting-review label

Member Author

cbrewster commented Aug 1, 2017

highfive assigned jdm and unassigned Manishearth

jdm requested changes

Member

jdm left a comment

Great work! These changes were quite readable and straightforward to review.

components/script/dom/bindings/interface.rs Outdated

  
                    Root::downcast(element).ok_or(Error::InvalidState)

                },

                // Step 10

                Some(ConstructionStackEntry::AlreadyConstructedMarker) => return Err(Error::InvalidState),

jdm Aug 9, 2017

Member

No need for return here.

cbrewster Aug 9, 2017

Author Member

Done.

components/script/dom/customelementregistry.rs Outdated

  
                    // TODO: Catch exceptions

                    return;

                }

                // Step 8

jdm Aug 9, 2017

Member

This is 7.2.

cbrewster Aug 9, 2017

Author Member

Done.

components/script/dom/customelementregistry.rs Outdated

  
                // Go into the constructor's compartment

                let _ac = JSAutoCompartment::new(cx, definition.constructor.callback());

                let args = HandleValueArray::new();

                // Step 7

jdm Aug 9, 2017

Member

7.1

cbrewster Aug 9, 2017

Author Member

Done.

components/script/dom/customelementregistry.rs Outdated

  
                    return;

                }

                if !same {

                    // TODO: Throw InvalidStateError

jdm Aug 9, 2017

Member

?

cbrewster Aug 9, 2017

Author Member

Sorry, this was implemented in a later commit.

components/script/dom/customelementregistry.rs Outdated

  
                                     -> Option<Rc<CustomElementDefinition>> {

                // Step 1

jdm Aug 9, 2017

Member

Let's move this to Document::lookup_custom_element_definition instead.

cbrewster Aug 9, 2017

Author Member

Done.

components/script/dom/customelementregistry.rs Outdated

  
                    let is = candidate.get_is();

                    if *candidate.local_name() == local_name &&

                        *candidate.namespace() == ns!(html) &&

                        (is.is_none() || is.as_ref() == Some(&name))

jdm Aug 9, 2017

Member

Don't we need to check extends here?

cbrewster Aug 9, 2017

Author Member

Oops, yup. Fixed.

components/script/dom/customelementregistry.rs Outdated

  
        }

        #[allow(unsafe_code)]

        fn run_upgrade_constructor(constructor: &Rc<Function>, element: &Element) -> ErrorResult {

jdm Aug 9, 2017

Member

Add a spec link here.

cbrewster Aug 9, 2017

Author Member

Done.

components/script/dom/element.rs Outdated

  
      @@ -150,6 +150,7 @@ pub struct Element {
    
            /// https://dom.spec.whatwg.org/#concept-element-custom-element-definition

            #[ignore_heap_size_of = "Rc"]

            custom_element_definition: DOMRefCell<Option<Rc<CustomElementDefinition>>>,

            custom_element_state: Cell<CustomElementState>,

jdm Aug 9, 2017

Member

Add a link to https://dom.spec.whatwg.org/#concept-element-custom-element-state .

cbrewster Aug 9, 2017

Author Member

Done.

components/script/dom/element.rs Outdated

  
      @@ -179,6 +180,14 @@ pub enum CustomElementCreationMode {
    
            Asynchronous,

        }

        #[derive(Clone, Copy, PartialEq, Eq, HeapSizeOf, JSTraceable)]

        pub enum CustomElementState {

jdm Aug 9, 2017

Member

Add a link to https://dom.spec.whatwg.org/#concept-element-custom-element-state .

cbrewster Aug 9, 2017

Author Member

Done.

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

cbrewster added 3 commits


        Add construction stack

8c5005f


        Implement element upgrade algorithm

ec528e9


        Add upgrade reaction

e83a004

cbrewster force-pushed the cbrewster:ce_upgrades branch from 6b065b2 to 0c2725d

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

Member

jdm commented Aug 9, 2017

@bors-servo: r+

Contributor

bors-servo commented Aug 9, 2017

📌 Commit 0c2725d has been approved by jdm

highfive added S-awaiting-merge and removed S-awaiting-review labels

Contributor

bors-servo commented Aug 9, 2017

⌛ Testing commit 0c2725d with merge 866c8e9...

bors-servo added a commit that referenced this pull request


        Auto merge of #17935 - cbrewster:ce_upgrades, r=jdm

866c8e9

Custom element upgrades

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/17935)
<!-- Reviewable:end -->

Contributor

bors-servo commented Aug 9, 2017

💔 Test failed - linux-rel-wpt

highfive added S-tests-failed and removed S-awaiting-merge labels

Member

jdm commented Aug 9, 2017

  ▶ Unexpected subtest result in /_mozilla/mozilla/collections.html:
  │ FAIL [expected PASS] test_binding\n    page   43
  │   → assert_true: Should be function HTMLUnknownElement() {
    [native code]
} expected true got false
  │ 
  │ check_collection/<@http://web-platform.test:8000/_mozilla/mozilla/:86:11
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  │ check_collection@http://web-platform.test:8000/_mozilla/mozilla/:71:5
  │ check_tag@http://web-platform.test:8000/_mozilla/mozilla/:95:7
  │ @http://web-platform.test:8000/_mozilla/mozilla/:145:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  └ @http://web-platform.test:8000/_mozilla/mozilla/:69:1

  ▶ Unexpected subtest result in /_mozilla/mozilla/prototypes.html:
  │ FAIL [expected PASS] Untitled
  │   → assert_true: Should be HTMLUnknownElement expected true got false
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/prototypes.html:21:3
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  └ @http://web-platform.test:8000/_mozilla/mozilla/prototypes.html:10:1

cbrewster added 4 commits


        Use HTMLElement interface for possible CEs

37cbc85


        Fix compartment mismatch issue

1a9f4ca


        Check namespace during ce def lookup


        Enqueue upgrades

6d9d4ad

cbrewster added 3 commits


        Handle exceptions during upgrades

e700006


        Track custom element state

9f51c7d


        Change foo-á to fooá

aa9b9c1

These tests assumed that foo-á would create a HTMLUnknownElement; however,
foo-á is a valid custom element name; therefore, according to the spec, a
HTMLElement instace is created instead. I changed the name to fooá so a
HTMLUnknownElement is created as the test expects.

cbrewster force-pushed the cbrewster:ce_upgrades branch from 0c2725d to aa9b9c1

highfive added S-awaiting-review and removed S-tests-failed labels

Member Author

cbrewster commented Aug 9, 2017

@bors-servo r=jdm

Contributor

bors-servo commented Aug 9, 2017

📌 Commit aa9b9c1 has been approved by jdm

highfive removed the S-awaiting-review label

Contributor

bors-servo commented Aug 9, 2017

⌛ Testing commit aa9b9c1 with merge 931e205...

bors-servo added a commit that referenced this pull request


        Auto merge of #17935 - cbrewster:ce_upgrades, r=jdm

931e205

Custom element upgrades

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/17935)
<!-- Reviewable:end -->

highfive added the S-awaiting-merge label

Contributor

bors-servo commented Aug 9, 2017

💔 Test failed - linux-rel-css

highfive added S-tests-failed and removed S-awaiting-merge labels

Member

jdm commented Aug 9, 2017

@bors-servo: retry

Contributor

bors-servo commented Aug 9, 2017

⌛ Testing commit aa9b9c1 with merge a087fee...

bors-servo added a commit that referenced this pull request


        Auto merge of #17935 - cbrewster:ce_upgrades, r=jdm

a087fee

Custom element upgrades

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/17935)
<!-- Reviewable:end -->

highfive added S-awaiting-merge and removed S-tests-failed labels

Contributor

bors-servo commented Aug 9, 2017

☀️ 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 a087fee to master...

bors-servo merged commit aa9b9c1 into servo:master

2 checks passed

2 checks passed

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

Details

homu Test successful

Details

highfive removed the S-awaiting-merge label

cbrewster mentioned this pull request

Implement custom element upgrades #17772

Closed

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.