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

Implement promise bindings #12830

Merged
merged 12 commits into from Sep 22, 2016

Conversation

Projects
None yet
9 participants
@jdm
Member

jdm commented Aug 12, 2016

This implements support for using Promises in WebIDL, executing promise callbacks in batches (equivalent to running all enqueued jobs from a setTimeout(0)), and attaching native callbacks to promise objects. This is the combined work of myself, @dati91, and @mmatyas based on the following prior work in Gecko:

  • Codegen.py
  • Promise.webidl
  • Promise.cpp/Promise.h
  • PromiseNativeHandler.h

This does not implement microtasks per #4283. This allows us to make progress on testing code that requires the use of Promises right now; the microtasks work is more complicated, but also largely orthogonal to implement.

Requires servo/mozjs#89, servo/rust-mozjs#287, and servo/rust-mozjs#294.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #4282
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Aug 12, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/global.rs, components/script/dom/webidls/PromiseNativeHandler.webidl, components/script/dom/testbinding.rs, components/script/dom/mod.rs, components/script/dom/bindings/codegen/Bindings.conf, components/script/dom/promisenativehandler.rs, components/script/dom/webidls/TestBinding.webidl, components/script/script_thread.rs, components/script/dom/bindings/js.rs, components/script/dom/webidls/Promise.webidl, components/script/dom/promise.rs, components/script/dom/bindings/codegen/Configuration.py

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/global.rs, components/script/dom/webidls/PromiseNativeHandler.webidl, components/script/dom/testbinding.rs, components/script/dom/mod.rs, components/script/dom/bindings/codegen/Bindings.conf, components/script/dom/promisenativehandler.rs, components/script/dom/webidls/TestBinding.webidl, components/script/script_thread.rs, components/script/dom/bindings/js.rs, components/script/dom/webidls/Promise.webidl, components/script/dom/promise.rs, components/script/dom/bindings/codegen/Configuration.py
@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Aug 12, 2016

warning Warning warning

  • These commits include an empty title element (<title></title>). Consider adding appropriate metadata.
  • These commits modify unsafe code. Please review it carefully!

warning Warning warning

  • These commits include an empty title element (<title></title>). Consider adding appropriate metadata.
  • These commits modify unsafe code. Please review it carefully!
@wafflespeanut

This comment has been minimized.

Show comment
Hide comment
@wafflespeanut

wafflespeanut Aug 12, 2016

Member

r? @Ms2ger, @nox, and everyone else 😄

Member

wafflespeanut commented Aug 12, 2016

r? @Ms2ger, @nox, and everyone else 😄

@highfive highfive assigned Ms2ger and unassigned wafflespeanut Aug 12, 2016

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Aug 12, 2016

Member

This PR does not support workers yet. That should only require moving the various queues out of ScriptThread and into Runtime instead.

Member

jdm commented Aug 12, 2016

This PR does not support workers yet. That should only require moving the various queues out of ScriptThread and into Runtime instead.

@jdm

This comment has been minimized.

Show comment
Hide comment
Member

jdm commented Aug 12, 2016

@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Aug 13, 2016

Member

@jdm It doesn't compile.

Member

nox commented Aug 13, 2016

@jdm It doesn't compile.

Show outdated Hide outdated components/script/dom/bindings/global.rs
fn init_reflector(&mut self, _obj: *mut JSObject) {
unreachable!()
}
}

This comment has been minimized.

@nox

nox Aug 13, 2016

Member

Why is this impl needed?

@nox

nox Aug 13, 2016

Member

Why is this impl needed?

This comment has been minimized.

@jdm

jdm Aug 14, 2016

Member

So we can pass a GlobalRef to a function that requires a T: Reflectable.

@jdm

jdm Aug 14, 2016

Member

So we can pass a GlobalRef to a function that requires a T: Reflectable.

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Aug 14, 2016

Member

@nox It doesn't compile because it relies on open PRs in other repositories.

Member

jdm commented Aug 14, 2016

@nox It doesn't compile because it relies on open PRs in other repositories.

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 22, 2016

Contributor

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

Contributor

bors-servo commented Aug 22, 2016

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

@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Aug 23, 2016

Member

Didn't see anything bad and just have a few questions. @Ms2ger Would be great if you could skim through it quickly, given you know SM better than me.

-S-awaiting-review +S-needs-squash


Reviewed 10 of 10 files at r1, 4 of 4 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 4 of 4 files at r5, 6 of 6 files at r6, 5 of 5 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


components/script/dom/bindings/global.rs, line 302 [r7] (raw file):

Previously, jdm (Josh Matthews) wrote…

So we can pass a GlobalRef to a function that requires a T: Reflectable.

Could you just make `reflector` an intrinsic method?

components/script/dom/webidls/Promise.webidl, line 11 [r1] (raw file):

// Need to escape "Promise" so it's treated as an identifier.
interface _Promise {
};

Is there no way to avoid doing that silly fake interface, kinda like WindowProxy?


components/script/dom/webidls/PromiseNativeHandler.webidl, line 13 [r5] (raw file):

 Exposed=(Window,Worker)]
interface PromiseNativeHandler {
};

Can we avoid that? Also nit: missing new line at the end.


Comments from Reviewable

Member

nox commented Aug 23, 2016

Didn't see anything bad and just have a few questions. @Ms2ger Would be great if you could skim through it quickly, given you know SM better than me.

-S-awaiting-review +S-needs-squash


Reviewed 10 of 10 files at r1, 4 of 4 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 4 of 4 files at r5, 6 of 6 files at r6, 5 of 5 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


components/script/dom/bindings/global.rs, line 302 [r7] (raw file):

Previously, jdm (Josh Matthews) wrote…

So we can pass a GlobalRef to a function that requires a T: Reflectable.

Could you just make `reflector` an intrinsic method?

components/script/dom/webidls/Promise.webidl, line 11 [r1] (raw file):

// Need to escape "Promise" so it's treated as an identifier.
interface _Promise {
};

Is there no way to avoid doing that silly fake interface, kinda like WindowProxy?


components/script/dom/webidls/PromiseNativeHandler.webidl, line 13 [r5] (raw file):

 Exposed=(Window,Worker)]
interface PromiseNativeHandler {
};

Can we avoid that? Also nit: missing new line at the end.


Comments from Reviewable

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Aug 23, 2016

Member

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


components/script/dom/bindings/global.rs, line 302 [r7] (raw file):

Previously, nox (Anthony Ramine) wrote…

Could you just make reflector an intrinsic method?

I'm not sure what you mean. We could split Reflectable into `Reflectable` and `MutReflectable` where the second inherits from the first and code that needs it uses it, though.

components/script/dom/webidls/Promise.webidl, line 11 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Is there no way to avoid doing that silly fake interface, kinda like WindowProxy?

I would rather not. We still need the rest of the items in this file, so I don't think it's a big deal.

components/script/dom/webidls/PromiseNativeHandler.webidl, line 13 [r5] (raw file):

Previously, nox (Anthony Ramine) wrote…

Can we avoid that? Also nit: missing new line at the end.

We need an type that we can unwrap into safely.

Comments from Reviewable

Member

jdm commented Aug 23, 2016

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


components/script/dom/bindings/global.rs, line 302 [r7] (raw file):

Previously, nox (Anthony Ramine) wrote…

Could you just make reflector an intrinsic method?

I'm not sure what you mean. We could split Reflectable into `Reflectable` and `MutReflectable` where the second inherits from the first and code that needs it uses it, though.

components/script/dom/webidls/Promise.webidl, line 11 [r1] (raw file):

Previously, nox (Anthony Ramine) wrote…

Is there no way to avoid doing that silly fake interface, kinda like WindowProxy?

I would rather not. We still need the rest of the items in this file, so I don't think it's a big deal.

components/script/dom/webidls/PromiseNativeHandler.webidl, line 13 [r5] (raw file):

Previously, nox (Anthony Ramine) wrote…

Can we avoid that? Also nit: missing new line at the end.

We need an type that we can unwrap into safely.

Comments from Reviewable

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Aug 23, 2016

Member

This is blocked on #12954 due to breaking changes in the JS conversion APIs.

Member

jdm commented Aug 23, 2016

This is blocked on #12954 due to breaking changes in the JS conversion APIs.

@jdm

This comment has been minimized.

Show comment
Hide comment
Member

jdm commented Aug 24, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 24, 2016

Contributor

⌛️ Trying commit a726a15 with merge b380723...

Contributor

bors-servo commented Aug 24, 2016

⌛️ Trying commit a726a15 with merge b380723...

bors-servo added a commit that referenced this pull request Aug 24, 2016

Auto merge of #12830 - jdm:promises, r=<try>
Implement promise bindings

This implements support for using Promises in WebIDL, executing promise callbacks in batches (equivalent to running all enqueued jobs from a setTimeout(0)), and attaching native callbacks to promise objects. This is the combined work of myself, @dati91, and @mmatyas based on the following prior work in Gecko:
* Codegen.py
* Promise.webidl
* Promise.cpp/Promise.h
* PromiseNativeHandler.h

This does not implement microtasks per #4283. This allows us to make progress on testing code that requires the use of Promises right now; the microtasks work is more complicated, but also largely orthogonal to implement.

Requires servo/mozjs#89, servo/rust-mozjs#287, and servo/rust-mozjs#294.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #4282
- [X] There are tests for these changes

<!-- 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/12830)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 24, 2016

Contributor

💔 Test failed - linux-rel

Contributor

bors-servo commented Aug 24, 2016

💔 Test failed - linux-rel

Show outdated Hide outdated tests/wpt/mozilla/tests/mozilla/promise.html
});
p.then(test.unreached_func());
return Promise.resolve('success').then(p);
}, 'Native resolve callback gets argument');

This comment has been minimized.

@jdm

jdm Aug 24, 2016

Member

Note to self: this test appears to be bogus? It's supposed to be testing rejection...

@jdm

jdm Aug 24, 2016

Member

Note to self: this test appears to be bogus? It's supposed to be testing rejection...

@jdm

This comment has been minimized.

Show comment
Hide comment
Member

jdm commented Aug 25, 2016

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 25, 2016

Contributor

⌛️ Trying commit b1368a3 with merge 58a3f74...

Contributor

bors-servo commented Aug 25, 2016

⌛️ Trying commit b1368a3 with merge 58a3f74...

bors-servo added a commit that referenced this pull request Aug 25, 2016

Auto merge of #12830 - jdm:promises, r=<try>
Implement promise bindings

This implements support for using Promises in WebIDL, executing promise callbacks in batches (equivalent to running all enqueued jobs from a setTimeout(0)), and attaching native callbacks to promise objects. This is the combined work of myself, @dati91, and @mmatyas based on the following prior work in Gecko:
* Codegen.py
* Promise.webidl
* Promise.cpp/Promise.h
* PromiseNativeHandler.h

This does not implement microtasks per #4283. This allows us to make progress on testing code that requires the use of Promises right now; the microtasks work is more complicated, but also largely orthogonal to implement.

Requires servo/mozjs#89, servo/rust-mozjs#287, and servo/rust-mozjs#294.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #4282
- [X] There are tests for these changes

<!-- 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/12830)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 25, 2016

Contributor

💔 Test failed - mac-rel-wpt

Contributor

bors-servo commented Aug 25, 2016

💔 Test failed - mac-rel-wpt

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 25, 2016

Contributor

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

Contributor

bors-servo commented Aug 25, 2016

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

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Aug 25, 2016

Member

Thinking more about this, I don't believe Rc<Promise> makes sense at all. I'm going to try making this PR use Root<Promise> instead, because otherwise there is a clear GC hazard when using Promise::new - the reflector doesn't exist anywhere else in the list of known roots.

Member

jdm commented Aug 25, 2016

Thinking more about this, I don't believe Rc<Promise> makes sense at all. I'm going to try making this PR use Root<Promise> instead, because otherwise there is a clear GC hazard when using Promise::new - the reflector doesn't exist anywhere else in the list of known roots.

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 22, 2016

Contributor

Still want to have another look at the refcounting; will do that later today.


Reviewed 1 of 10 files at r1, 1 of 5 files at r20, 5 of 30 files at r21, 1 of 8 files at r24, 1 of 1 files at r26, 3 of 7 files at r27, 7 of 8 files at r28, 2 of 2 files at r29, 108 of 108 files at r30.
Review status: 132 of 133 files reviewed at latest revision, 20 unresolved discussions, some commit checks broke.


components/script/script_runtime.rs, line 146 at r20 (raw file):

Previously, Ms2ger wrote…

Holding the borrow on self.flushing_job_queue may be problematic if this can end up calling back into enqueue or itself. If that can't happen, please make a note (and perhaps just borrow self.flushing_job_queue for the entire function).

So there really is no point to putting `flushing_job_queue` in a field rather than a local mutable variable, is there? Except tracing, maybe...

components/script/script_runtime.rs, line 105 at r30 (raw file):

}

/// A promise callback scheduled to run during the next microtask checkpoint.

Do we support microtasks for real? If not, do we have an issue number that could improve this comment?


components/script/script_runtime.rs, line 122 at r30 (raw file):

    /// The list of enqueued promise callbacks that will be invoked at the next microtask checkpoint.
    promise_job_queue: DOMRefCell<Vec<EnqueuedPromiseCallback>>,
    /// True if there is an outstanding runnable responsible for evaluating the promise job queue.

I'm not quite clear on why this is necessary; would be nice to explain.


components/script/script_runtime.rs, line 176 at r30 (raw file):

                                 _allocation_site: HandleObject,
                                 _data: *mut c_void) -> bool {
    let global = global_root_from_object(job.get());

Do we need panic protection?


components/script/dom/promise.rs, line 28 at r30 (raw file):

    reflector: Reflector,
    #[ignore_heap_size_of = "SM handles JS values"]
    root: MutHeapJSVal,

Terrible name :)


components/script/dom/promise.rs, line 31 at r30 (raw file):

}

trait PromiseHelper {

Explain the purpose of this trait, please. Is it so only rc-wrapped promises can be initialized?


components/script/dom/promise.rs, line 125 at r30 (raw file):

    #[allow(unsafe_code)]
    pub fn maybe_resolve_native<T>(&self, cx: *mut JSContext, val: &T) where T: ToJSValConvertible {

What does maybe_ mean for all those?


components/script/dom/promisenativehandler.rs, line 13 at r30 (raw file):

use js::jsapi::{JSContext, HandleValue};

pub type CallbackType = Box<Callback>;

Not convinced this typedef is useful.


components/script/dom/promisenativehandler.rs, line 29 at r30 (raw file):

    pub fn new(global: GlobalRef,
           resolve: Option<CallbackType>,
           reject: Option<CallbackType>) -> Root<PromiseNativeHandler> {

Indentation; also move the return type to the line below.


components/script/dom/promisenativehandler.rs, line 38 at r30 (raw file):

    fn callback(callback: &Option<CallbackType>, cx: *mut JSContext, v: HandleValue) {
        if let &Some(ref callback) = callback {

Nit: if let Some(ref callback) = *callback {}


components/script/dom/testbinding.rs, line 695 at r30 (raw file):

        let global = self.global();
        let handler = PromiseNativeHandler::new(global.r(),
                                                resolve.map(|r| SimpleHandler::new(r)),

eta-reduce, please.


components/script/dom/workerglobalscope.rs, line 242 at r30 (raw file):

    pub fn flush_promise_jobs(&self) {
        let _ = self.script_chan().send(CommonScriptMsg::RunnableMsg(

I'd rather unwrap the result.


components/script/dom/bindings/error.rs, line 273 at r20 (raw file):

Previously, Ms2ger wrote…

Needs a better name, and better docs for the case where an exception is already pending.

Let's just assert that no exception is pending; this code is not called in a situation where there can be a pending exception anyway.

components/script/dom/bindings/global.rs, line 296 at r30 (raw file):

    pub fn enqueue_promise_job(&self, job: EnqueuedPromiseCallback) {
        match *self {
            GlobalRef::Window(ref _window) => ScriptThread::enqueue_promise_job(job, *self),

GlobalRef::Window(_) is fine too


components/script/dom/bindings/refcounted.rs, line 200 at r30 (raw file):

    fn addref_promise(&self, promise: Rc<Promise>) {
        let mut table = self.promise_table.borrow_mut();
        match table.entry(&*promise) {

I tend to prefer table.entry().or_insert(vec![]).push(promise)


components/script/dom/bindings/codegen/CodegenRust.py, line 661 at r30 (raw file):

    # they really should be!
    if exceptionCode is None:
        exceptionCode = "return false;\n"

Revert, please.


components/script/dom/bindings/codegen/CodegenRust.py, line 682 at r30 (raw file):

    # wrong type of value.
    def onFailureNotAnObject(failureCode):
        return CGGeneric(failureCode or

Revert this unrelated change, please.


components/script/dom/bindings/codegen/CodegenRust.py, line 824 at r30 (raw file):

                  rooted!(in(cx) let globalObj = CurrentGlobalOrNull(cx));
                  let _ac = JSAutoCompartment::new(cx, globalObj.handle().get());

Is this even necessary? You're entering the compartment of the global that's the global of the compartment the cx is in.


tests/wpt/mozilla/tests/mozilla/promise.html, line 61 at r30 (raw file):

      var timeoutFired = false;
      t.resolvePromiseDelayed(p, 'success', 100);
      setTimeout(function() { timeoutFired = true }, 100);

This looks like an intermittent failure waiting to happen.


Comments from Reviewable

Contributor

Ms2ger commented Sep 22, 2016

Still want to have another look at the refcounting; will do that later today.


Reviewed 1 of 10 files at r1, 1 of 5 files at r20, 5 of 30 files at r21, 1 of 8 files at r24, 1 of 1 files at r26, 3 of 7 files at r27, 7 of 8 files at r28, 2 of 2 files at r29, 108 of 108 files at r30.
Review status: 132 of 133 files reviewed at latest revision, 20 unresolved discussions, some commit checks broke.


components/script/script_runtime.rs, line 146 at r20 (raw file):

Previously, Ms2ger wrote…

Holding the borrow on self.flushing_job_queue may be problematic if this can end up calling back into enqueue or itself. If that can't happen, please make a note (and perhaps just borrow self.flushing_job_queue for the entire function).

So there really is no point to putting `flushing_job_queue` in a field rather than a local mutable variable, is there? Except tracing, maybe...

components/script/script_runtime.rs, line 105 at r30 (raw file):

}

/// A promise callback scheduled to run during the next microtask checkpoint.

Do we support microtasks for real? If not, do we have an issue number that could improve this comment?


components/script/script_runtime.rs, line 122 at r30 (raw file):

    /// The list of enqueued promise callbacks that will be invoked at the next microtask checkpoint.
    promise_job_queue: DOMRefCell<Vec<EnqueuedPromiseCallback>>,
    /// True if there is an outstanding runnable responsible for evaluating the promise job queue.

I'm not quite clear on why this is necessary; would be nice to explain.


components/script/script_runtime.rs, line 176 at r30 (raw file):

                                 _allocation_site: HandleObject,
                                 _data: *mut c_void) -> bool {
    let global = global_root_from_object(job.get());

Do we need panic protection?


components/script/dom/promise.rs, line 28 at r30 (raw file):

    reflector: Reflector,
    #[ignore_heap_size_of = "SM handles JS values"]
    root: MutHeapJSVal,

Terrible name :)


components/script/dom/promise.rs, line 31 at r30 (raw file):

}

trait PromiseHelper {

Explain the purpose of this trait, please. Is it so only rc-wrapped promises can be initialized?


components/script/dom/promise.rs, line 125 at r30 (raw file):

    #[allow(unsafe_code)]
    pub fn maybe_resolve_native<T>(&self, cx: *mut JSContext, val: &T) where T: ToJSValConvertible {

What does maybe_ mean for all those?


components/script/dom/promisenativehandler.rs, line 13 at r30 (raw file):

use js::jsapi::{JSContext, HandleValue};

pub type CallbackType = Box<Callback>;

Not convinced this typedef is useful.


components/script/dom/promisenativehandler.rs, line 29 at r30 (raw file):

    pub fn new(global: GlobalRef,
           resolve: Option<CallbackType>,
           reject: Option<CallbackType>) -> Root<PromiseNativeHandler> {

Indentation; also move the return type to the line below.


components/script/dom/promisenativehandler.rs, line 38 at r30 (raw file):

    fn callback(callback: &Option<CallbackType>, cx: *mut JSContext, v: HandleValue) {
        if let &Some(ref callback) = callback {

Nit: if let Some(ref callback) = *callback {}


components/script/dom/testbinding.rs, line 695 at r30 (raw file):

        let global = self.global();
        let handler = PromiseNativeHandler::new(global.r(),
                                                resolve.map(|r| SimpleHandler::new(r)),

eta-reduce, please.


components/script/dom/workerglobalscope.rs, line 242 at r30 (raw file):

    pub fn flush_promise_jobs(&self) {
        let _ = self.script_chan().send(CommonScriptMsg::RunnableMsg(

I'd rather unwrap the result.


components/script/dom/bindings/error.rs, line 273 at r20 (raw file):

Previously, Ms2ger wrote…

Needs a better name, and better docs for the case where an exception is already pending.

Let's just assert that no exception is pending; this code is not called in a situation where there can be a pending exception anyway.

components/script/dom/bindings/global.rs, line 296 at r30 (raw file):

    pub fn enqueue_promise_job(&self, job: EnqueuedPromiseCallback) {
        match *self {
            GlobalRef::Window(ref _window) => ScriptThread::enqueue_promise_job(job, *self),

GlobalRef::Window(_) is fine too


components/script/dom/bindings/refcounted.rs, line 200 at r30 (raw file):

    fn addref_promise(&self, promise: Rc<Promise>) {
        let mut table = self.promise_table.borrow_mut();
        match table.entry(&*promise) {

I tend to prefer table.entry().or_insert(vec![]).push(promise)


components/script/dom/bindings/codegen/CodegenRust.py, line 661 at r30 (raw file):

    # they really should be!
    if exceptionCode is None:
        exceptionCode = "return false;\n"

Revert, please.


components/script/dom/bindings/codegen/CodegenRust.py, line 682 at r30 (raw file):

    # wrong type of value.
    def onFailureNotAnObject(failureCode):
        return CGGeneric(failureCode or

Revert this unrelated change, please.


components/script/dom/bindings/codegen/CodegenRust.py, line 824 at r30 (raw file):

                  rooted!(in(cx) let globalObj = CurrentGlobalOrNull(cx));
                  let _ac = JSAutoCompartment::new(cx, globalObj.handle().get());

Is this even necessary? You're entering the compartment of the global that's the global of the compartment the cx is in.


tests/wpt/mozilla/tests/mozilla/promise.html, line 61 at r30 (raw file):

      var timeoutFired = false;
      t.resolvePromiseDelayed(p, 'success', 100);
      setTimeout(function() { timeoutFired = true }, 100);

This looks like an intermittent failure waiting to happen.


Comments from Reviewable

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 22, 2016

Member

Review status: 132 of 133 files reviewed at latest revision, 21 unresolved discussions, some commit checks broke.


components/script/script_runtime.rs, line 146 at r20 (raw file):

Previously, Ms2ger wrote…

So there really is no point to putting flushing_job_queue in a field rather than a local mutable variable, is there? Except tracing, maybe...

There is a point - the callbacks that have not yet been executed will be traced by the GC if they're in a field.

components/script/script_runtime.rs, line 176 at r30 (raw file):

Previously, Ms2ger wrote…

Do we need panic protection?

I don't see how the code would panic, so I don't see the point. It's up to you.

components/script/dom/promise.rs, line 31 at r30 (raw file):

Previously, Ms2ger wrote…

Explain the purpose of this trait, please. Is it so only rc-wrapped promises can be initialized?

Yes.

components/script/dom/promise.rs, line 125 at r30 (raw file):

Previously, Ms2ger wrote…

What does maybe_ mean for all those?

I copied the pattern from Gecko, but it doesn't make sense to me. I'll remove it.

Comments from Reviewable

Member

jdm commented Sep 22, 2016

Review status: 132 of 133 files reviewed at latest revision, 21 unresolved discussions, some commit checks broke.


components/script/script_runtime.rs, line 146 at r20 (raw file):

Previously, Ms2ger wrote…

So there really is no point to putting flushing_job_queue in a field rather than a local mutable variable, is there? Except tracing, maybe...

There is a point - the callbacks that have not yet been executed will be traced by the GC if they're in a field.

components/script/script_runtime.rs, line 176 at r30 (raw file):

Previously, Ms2ger wrote…

Do we need panic protection?

I don't see how the code would panic, so I don't see the point. It's up to you.

components/script/dom/promise.rs, line 31 at r30 (raw file):

Previously, Ms2ger wrote…

Explain the purpose of this trait, please. Is it so only rc-wrapped promises can be initialized?

Yes.

components/script/dom/promise.rs, line 125 at r30 (raw file):

Previously, Ms2ger wrote…

What does maybe_ mean for all those?

I copied the pattern from Gecko, but it doesn't make sense to me. I'll remove it.

Comments from Reviewable

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 22, 2016

Member

Review status: 132 of 133 files reviewed at latest revision, 21 unresolved discussions, some commit checks broke.


components/script/dom/bindings/codegen/CodegenRust.py, line 661 at r30 (raw file):

Previously, Ms2ger wrote…

Revert, please.

I'd rather not. This improved the codegen output for promise code that I was looking at.

components/script/dom/bindings/codegen/CodegenRust.py, line 682 at r30 (raw file):

Previously, Ms2ger wrote…

Revert this unrelated change, please.

Not unrelated - it made the output of the promise codegen easier to read.

Comments from Reviewable

Member

jdm commented Sep 22, 2016

Review status: 132 of 133 files reviewed at latest revision, 21 unresolved discussions, some commit checks broke.


components/script/dom/bindings/codegen/CodegenRust.py, line 661 at r30 (raw file):

Previously, Ms2ger wrote…

Revert, please.

I'd rather not. This improved the codegen output for promise code that I was looking at.

components/script/dom/bindings/codegen/CodegenRust.py, line 682 at r30 (raw file):

Previously, Ms2ger wrote…

Revert this unrelated change, please.

Not unrelated - it made the output of the promise codegen easier to read.

Comments from Reviewable

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 22, 2016

Member

Comments addressed, except for the ones I replied to explaining why they weren't addressed.

Member

jdm commented Sep 22, 2016

Comments addressed, except for the ones I replied to explaining why they weren't addressed.

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 22, 2016

Contributor

Reviewed 10 of 10 files at r31.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


components/script/script_runtime.rs, line 176 at r30 (raw file):

Previously, jdm (Josh Matthews) wrote…

I don't see how the code would panic, so I don't see the point. It's up to you.

`job` probably won't, but theoretically could be null... I'd feel happier if you added it.

components/script/dom/promise.rs, line 149 at r31 (raw file):

    #[allow(unrooted_must_root, unsafe_code)]
    pub fn resolve(&self,
                         cx: *mut JSContext,

Indent


components/script/dom/bindings/codegen/CodegenRust.py, line 661 at r30 (raw file):

Previously, jdm (Josh Matthews) wrote…

I'd rather not. This improved the codegen output for promise code that I was looking at.

But none of the explicit`exceptionCode` arguments end on a newline. If this improved the codegen, you should add `exceptionCode += "\n"` or something after the if.

components/script/dom/bindings/codegen/CodegenRust.py, line 682 at r30 (raw file):

Previously, jdm (Josh Matthews) wrote…

Not unrelated - it made the output of the promise codegen easier to read.

Hmm, this one actually makes sense.

Comments from Reviewable

Contributor

Ms2ger commented Sep 22, 2016

Reviewed 10 of 10 files at r31.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


components/script/script_runtime.rs, line 176 at r30 (raw file):

Previously, jdm (Josh Matthews) wrote…

I don't see how the code would panic, so I don't see the point. It's up to you.

`job` probably won't, but theoretically could be null... I'd feel happier if you added it.

components/script/dom/promise.rs, line 149 at r31 (raw file):

    #[allow(unrooted_must_root, unsafe_code)]
    pub fn resolve(&self,
                         cx: *mut JSContext,

Indent


components/script/dom/bindings/codegen/CodegenRust.py, line 661 at r30 (raw file):

Previously, jdm (Josh Matthews) wrote…

I'd rather not. This improved the codegen output for promise code that I was looking at.

But none of the explicit`exceptionCode` arguments end on a newline. If this improved the codegen, you should add `exceptionCode += "\n"` or something after the if.

components/script/dom/bindings/codegen/CodegenRust.py, line 682 at r30 (raw file):

Previously, jdm (Josh Matthews) wrote…

Not unrelated - it made the output of the promise codegen easier to read.

Hmm, this one actually makes sense.

Comments from Reviewable

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 22, 2016

Contributor

SQUASH!

-S-awaiting-review


Reviewed 3 of 3 files at r32.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Contributor

Ms2ger commented Sep 22, 2016

SQUASH!

-S-awaiting-review


Reviewed 3 of 3 files at r32.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 22, 2016

Member

@bors-servo: r=Ms2ger

Member

jdm commented Sep 22, 2016

@bors-servo: r=Ms2ger

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 22, 2016

Contributor

📌 Commit 7b3e262 has been approved by Ms2ger

Contributor

bors-servo commented Sep 22, 2016

📌 Commit 7b3e262 has been approved by Ms2ger

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 22, 2016

Member

@bors-servo: r=Ms2ger

Member

jdm commented Sep 22, 2016

@bors-servo: r=Ms2ger

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 22, 2016

Contributor

📌 Commit e9c0606 has been approved by Ms2ger

Contributor

bors-servo commented Sep 22, 2016

📌 Commit e9c0606 has been approved by Ms2ger

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 22, 2016

Contributor

⌛️ Testing commit e9c0606 with merge 2b1a39c...

Contributor

bors-servo commented Sep 22, 2016

⌛️ Testing commit e9c0606 with merge 2b1a39c...

bors-servo added a commit that referenced this pull request Sep 22, 2016

Auto merge of #12830 - jdm:promises, r=Ms2ger
Implement promise bindings

This implements support for using Promises in WebIDL, executing promise callbacks in batches (equivalent to running all enqueued jobs from a setTimeout(0)), and attaching native callbacks to promise objects. This is the combined work of myself, @dati91, and @mmatyas based on the following prior work in Gecko:
* Codegen.py
* Promise.webidl
* Promise.cpp/Promise.h
* PromiseNativeHandler.h

This does not implement microtasks per #4283. This allows us to make progress on testing code that requires the use of Promises right now; the microtasks work is more complicated, but also largely orthogonal to implement.

Requires servo/mozjs#89, servo/rust-mozjs#287, and servo/rust-mozjs#294.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #4282
- [X] There are tests for these changes

<!-- 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/12830)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
Contributor

bors-servo commented Sep 22, 2016

@bors-servo bors-servo merged commit e9c0606 into servo:master Sep 22, 2016

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment