Closed Bug 939906 Opened 6 years ago Closed 6 years ago

Promise resolve(), reject(), then() and catch() should not error when no arguments or null is passed.

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: nsm, Assigned: nsm)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

for resolve and reject, no arg == passing undefined.
then(null, function() {}) is used in a lot of code when catch() wasn't part of the standard. The promises-unwrapping spec also makes a IsCallable() check, but does not error.
Similarly catch(null) is dumb, but allowed.
Assignee: nobody → nsm.nikhil
Blocks: promises
Notably, `promise.then()` gives you a "clone" of `promise`.
Are you actually talking about "no arg" or "null passed"?  Those are different things...
(In reply to Boris Zbarsky [:bz] from comment #2)
> Are you actually talking about "no arg" or "null passed"?  Those are
> different things...

I intend to fix both things, by changing stuff to |optional any| (resolve/reject) or |optional AnyCallback?| (then/catch)
Summary: Promise resolve(), reject(), then() and catch() should not error when no arguments are passed. → Promise resolve(), reject(), then() and catch() should not error when no arguments or null is passed.
Comment on attachment 8334061 [details] [diff] [review]
Make Promise.resolve(), Promise.reject(), Promise.prototype.then() and Promise.prototype.catch() spec compliant.

Review of attachment 8334061 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/promise/Promise.cpp
@@ +415,5 @@
>  }
>  
>  /* static */ already_AddRefed<Promise>
>  Promise::Resolve(const GlobalObject& aGlobal, JSContext* aCx,
> +                 const Optional<JS::Handle<JS::Value> >& aValue, ErrorResult& aRv)

we can remove this additional space Optional<JS::Handle<JS::Value>>& aValue.
Here and everywhere else.
Attachment #8334061 - Flags: review?(amarchesini) → review+
What about workers? Do you test all of these methods in workers too?
(In reply to Andrea Marchesini (:baku) from comment #7)
> What about workers? Do you test all of these methods in workers too?

The workers test in Bug 915233 tests these methods, I can update it to have the 4-5 new tests added here. There isn't major change, so it should work.
https://hg.mozilla.org/mozilla-central/rev/58f2fbf00c8c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.