Skip to content
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

Make TopTypeId an untagged union #13224

Merged
merged 2 commits into from Oct 6, 2016
Merged

Make TopTypeId an untagged union #13224

merged 2 commits into from Oct 6, 2016

Conversation

nox
Copy link
Contributor

@nox nox commented Sep 11, 2016

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/lib.rs

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 11, 2016
@nox
Copy link
Contributor Author

nox commented Sep 11, 2016

#[derive(Copy)]
pub union TopTypeId {
    /// ID used by abstract interfaces.
    pub abstract_: (),
    /// ID used by interfaces that are not castable.
    pub alone: (),
    /// ID used by interfaces that derive from Blob.
    pub blob: BlobTypeId,
    /// ID used by interfaces that derive from DOMPointReadOnly.
    pub dompointreadonly: DOMPointReadOnlyTypeId,
    /// ID used by interfaces that derive from DOMRectReadOnly.
    pub domrectreadonly: DOMRectReadOnlyTypeId,
    /// ID used by interfaces that derive from Event.
    pub event: EventTypeId,
    /// ID used by interfaces that derive from EventTarget.
    pub eventtarget: EventTargetTypeId,
    /// ID used by interfaces that derive from HTMLCollection.
    pub htmlcollection: HTMLCollectionTypeId,
    /// ID used by interfaces that derive from NodeList.
    pub nodelist: NodeListTypeId,
    /// ID used by interfaces that derive from TestBinding.
    pub testbinding: TestBindingTypeId,
    /// ID used by interfaces that derive from WebGLObject.
    pub webglobject: WebGLObjectTypeId,
}

impl Clone for TopTypeId {
    fn clone(&self) -> Self { *self }
}

impl EventTarget {
    pub fn type_id(&self) -> &'static EventTargetTypeId {
        unsafe {
            &get_dom_class(self.reflector().get_jsobject().get())
                .unwrap()
                .type_id
                .eventtarget
        }
    }
}

@nox
Copy link
Contributor Author

nox commented Sep 11, 2016

r? @Ms2ger

@highfive highfive assigned Ms2ger and unassigned glennw Sep 11, 2016
@nox
Copy link
Contributor Author

nox commented Sep 11, 2016

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 5ade2ad with merge 6a70ead...

bors-servo pushed a commit that referenced this pull request Sep 11, 2016
Make TopTypeId an untagged union

<!-- 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/13224)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 11, 2016
@nox
Copy link
Contributor Author

nox commented Sep 11, 2016

Tests with unexpected results:
  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/textures/default-texture.html:
  │ FAIL [expected PASS] WebGL test #0: at (0, 0) expected: 0,0,0,255 was 0,255,0,255
  │   → assert_true: at (0, 0) expected: 0,0,0,255 was 0,255,0,255 expected true got false
  │ 
  │ reportTestResultsToHarness/<@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:88:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1401:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  │ reportTestResultsToHarness@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:87:7
  │ testFailed@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:153:5
  │ checkCanvasRectColor@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1047:9
  │ checkCanvasRect@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1073:3
  │ checkCanvas@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1096:3
  └ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/textures/default-texture.html:15:1

That's #11618.

@nox nox removed the S-tests-failed The changes caused existing tests to fail. label Sep 11, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 12, 2016

Seems fine, but I'm not sure it's worth it.

r? @jdm

@highfive highfive assigned jdm and unassigned Ms2ger Sep 12, 2016
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Oct 4, 2016
@jdm
Copy link
Member

jdm commented Oct 4, 2016

Thought about it some more and convinced myself that it's fine. This can merge after a rebase.

@jdm jdm removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 4, 2016
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 5, 2016
@nox nox removed the S-needs-rebase There are merge conflict errors. label Oct 5, 2016
@nox
Copy link
Contributor Author

nox commented Oct 5, 2016

@jdm r?

@jdm
Copy link
Member

jdm commented Oct 5, 2016

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit c5ab846 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 5, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit c5ab846 with merge df6cf7c...

bors-servo pushed a commit that referenced this pull request Oct 6, 2016
Make TopTypeId an untagged union

<!-- 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/13224)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-css

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 6, 2016
@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 6, 2016



Tests with unexpected results:
  ▶ TIMEOUT [expected CRASH] /css-multicol-1_dev/html4/multicol-br-inside-avoidcolumn-001.htm
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ assertion failed: self.block_flow.base.children.len() == 1 (thread LayoutWorker worker 2/6, at /Users/servo/buildbot/slave/mac-rel-css/build/components/layout/multicol.rs:149)
  │ stack backtrace:
  │    0:        0x10c11b0de - backtrace::backtrace::trace::h0e60ef08c7c34e9f
  │    1:        0x10c11b3cc - backtrace::capture::Backtrace::new::h8bf319c36d8f5d1b
  │    2:        0x10be27e74 - servo::main::_{{closure}}::h8c06f3f8650b0061
  │    3:        0x10d71cc53 - std::panicking::rust_panic_with_hook::hb1322e5f2588b4db
  │    4:        0x10bfd6fb4 - std::panicking::begin_panic::h0914615a412ba184
  │    5:        0x10c0237ee - _<layout..multicol..MulticolFlow as layout..flow..Flow>::assign_block_size::h04801996ffa09f36
  │    6:        0x10c02455f - _<layout..traversal..AssignISizes<'a> as layout..parallel..ParallelPreorderFlowTraversal>::run_parallel::h778902546d71fe1c
  │    7:        0x10c0249d9 - layout::parallel::assign_inline_sizes::h2227b862ea935cab
  │    8:        0x10bf8a92f - _<std..panic..AssertUnwindSafe<F> as core..ops..FnOnce<()>>::call_once::h1446c482998a66c1
  │    9:        0x10bf326f4 - std::panicking::try::do_call::h7d24ea05c0eeb441
  │   10:        0x10d71df5a - __rust_maybe_catch_panic
  │   11:        0x10bf5afe1 - _<F as alloc..boxed..FnBox<A>>::call_box::he7038aa8c8acae23
  │   12:        0x10d71bb74 - std::sys::thread::Thread::new::thread_start::h022e3887023c6290
  │   13:     0x7fff92796059 - _pthread_body
  │   14:     0x7fff92795fd6 - _pthread_start
  └ ERROR:servo: assertion failed: self.block_flow.base.children.len() == 1

@nox
Copy link
Contributor Author

nox commented Oct 6, 2016

@bors-servo retry #13613

@bors-servo
Copy link
Contributor

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only mac-rel-css...

@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-css

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 6, 2016
@bors-servo
Copy link
Contributor

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only mac-rel-css...

@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-css

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 6, 2016
@nox nox added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 6, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit c5ab846 with merge 73aa4fc...

bors-servo pushed a commit that referenced this pull request Oct 6, 2016
Make TopTypeId an untagged union

<!-- 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/13224)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

@bors-servo bors-servo merged commit c5ab846 into servo:master Oct 6, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants