- Move getUserMedia() from Navigator to MediaDevices. - Add MediaDevices.getSupportedConstraints() function. - remove require - add ideal/exact

- DOMString facingMode and mediaSource.

Cleanup refactor for upcoming patch. Note that with this change, assuming webspeech/SpeechRecognition.cpp's use of getUserMedia is in a chrome window - which I believe it is - this means that it's use of getUserMedia is now privileged as well, whereas before it wasn't by virtue of it passing in false for aPrivileged.

This is a proof-of-concept implementation of mediaDevices.getUserMedia with promises as defined in https://www.w3.org/Bugs/Public/show_bug.cgi?id=26810 for upcoming w3c teleconference. Try - https://tbpl.mozilla.org/?tree=Try&rev=e6b3843c7726

A test-page using mediaDevices.getUserMedia with promises. This test-page also contains wrappers for peerConnection, and suggests what examples could look like if promises were prevalent in all webrtc APIs. Feel free to take a peek.

Added in gUM polyfill so you can try it out without this patch.

Fixed some typos.

Should compile on Windows. Try - https://tbpl.mozilla.org/?tree=Try&rev=ed433cc24a42

Super-simple mediaDevices.getUserMedia promise test.

Comment on attachment 8492266 [details] [diff] [review] minor refactor of MediaManager::GetUserMedia to figure out privileged state itself Review of attachment 8492266 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaManager.h @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +#ifndef MediaManager_h__ > +#define MediaManager_h__ MOZILLA_MEDIAMANAGER_H @@ +631,5 @@ > }; > > } // namespace mozilla > + > +#endif // MediaManager_h__ match above

Updated nits. Carrying forward r=jesup.

jib, are the warning from the try run - https://tbpl.mozilla.org/php/getParsedLog.php?id=48527246&tree=Try the m3 test failures also fixed with this updated patches ?

Those are from the promise-using mediaDevice patch which is not for landing. I just wanted to land the other "minor refactor" patch with r+.

Comment on attachment 8492552 [details] pc_test_promise.html See http://lists.w3.org/Archives/Public/public-webrtc/2014Sep/0063.html for a walkthrough of this patch.

With errors no longer being strings shortly, free up 'mError' so it can hold the error rather than the failure-callback (the opposite of success is failure, not error).

Implements http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamError

Promises are go for navigator.mediaDevices.getUserMedia [1]. Implements part of https://github.com/w3c/mediacapture-main/pull/29 (see URL on bug). Gecko's promise.MayReject implementation has some unfortunate limitations however, in that I cannot get it to reject our promise with a MediaStreamError instance. Is this fixable? [1] http://lists.w3.org/Archives/Public/public-media-capture/2014Oct/0162.html

Instead of doubling our gUM tests (which seemed redundant), I went 50/50. I left the callback-ingrained peerConnectionTest harness with navigator.getUserMedia and changed all tests that use gUM directly to navigator.mediaDevices.getUserMedia (except the exceptions one). This shows off promises well.

Try - https://tbpl.mozilla.org/?tree=Try&rev=f0c16980852b

Comment on attachment 8507294 [details] [diff] [review] Part 3: add MediaStreamError interface >+#include "mozilla/dom/Event.h" Why do you need this in MediaStreamError.h? > Gecko's promise.MayReject implementation has some unfortunate limitations > however Those are very much on purpose. I went out of my way to put those in place, to catch people misusing it. The idea is that promise rejection values should be Error instances. If the intent is that this object will be used to reject promises with, it should really be an Error subclass (in the sense of having Error.prototype on its proto chain), no? Then we could add a MaybeReject variant that takes objects of this type and all that. Has the TAG looked at this API at all? r=me on the code bits, but I'm not convinced about the basic premise here....

Comment on attachment 8507294 [details] [diff] [review] Part 3: add MediaStreamError interface Oh, also, why are the webidl getters NS_IMETHOD instead of just void? They should be void.

Comment on attachment 8507304 [details] [diff] [review] Part 4: return MediaStreamError instead of error strings, to spec. >+ auto error = static_cast<MediaStreamError*>(aError); Why is this a safe cast? I see no indication that it would be. You probably want to give MediaStreamError an IID so you can QI to it. >+GetWindow(uint64_t id) { Curly on next line. >+ return static_cast<nsPIDOMWindow*> (nsGlobalWindow::GetInnerWindowWithId(id)); You don't need the static_cast. It's implicit here. >+ nsRefPtr<MediaStreamError> error = new MediaStreamError(GetWindow(mWindowID), Is something guaranteeing GetWindow doesn't return null here? I'm not a big fan of action-at-a-distance like this.... >+ const nsAString& aMessage = NS_LITERAL_STRING("")) { EmptyString() ? Various places in this patch. >+ if (!mMessage.Length()) { if (mMessage.IsEmpty()) { >+ mMessage.AssignLiteral(MOZ_UTF16("The object can not be found here.")); Why do you need the MOZ_UTF16 there? Similar at the other AssignLiteral callsites. I think the answer is you don't need it. >+class MediaStreamError MOZ_FINAL : public BaseMediaMgrError, I'm rather worried that this compiled. I thought we had static asserts to make sure that nsISupports is the first thing you inherit from.... Please fix that. I'd really like to undestand what the window id story is here. In comment form in the patch, ideally.

Comment on attachment 8507306 [details] [diff] [review] Part 5: add mediaDevices.getUserMedia with promises (3) +#include "MediaDevices.h" mozilla/dom/MediaDevices.h >+MediaDevices::CreateInstance(nsPIDOMWindow* aWindow) Why does this exist at all? Why not just call |new MediaDevices| in callers explicitly? >+ OnSuccess(nsISupports* aStream) This is MOZ_OVERRIDE, right? >+ mPromise->MaybeResolve(static_cast<DOMLocalMediaStream*>(aStream)); Why is this case safe? >+ auto error = static_cast<MediaStreamError*>(aError); And this one. >+ new GumResolver(p), >+ new GumResolver(p), This is pretty fishy: passing a 0-refcount object to a function. Better to hold a ref to it on the stack first. >+ MediaDevices(nsPIDOMWindow* aWindow) : mWindow(aWindow) {} Why is this not calling the appropiate DOMEventTargetHelper constructor? It should be! Then incidentally you won't need a GetParentObject() override or indeed an mWindow member.

Comment on attachment 8507310 [details] [diff] [review] Part 6: tests for mediaDevices.getUserMedia Review of attachment 8507310 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: dom/media/tests/mochitest/constraints.js @@ +57,5 @@ > + return p.then(function() { > + return navigator.mediaDevices.getUserMedia(test.constraints); > + }) > + .then(function() { > + is(null, test.error, test.message); nit: I think is(false,...) is more obvious to understand then null.

(In reply to Boris Zbarsky [:bz] from comment #22) > > Gecko's promise.MayReject implementation has some unfortunate limitations > > however > > Those are very much on purpose. I went out of my way to put those in place, > to catch people misusing it. > > The idea is that promise rejection values should be Error instances. If the > intent is that this object will be used to reject promises with, it should > really be an Error subclass (in the sense of having Error.prototype on its > proto chain), no? Then we could add a MaybeReject variant that takes > objects of this type and all that. I'm confused. We were told by Anne that we couldn't inherit from Error. [1] > Has the TAG looked at this API at all? Domenic and Anne have been extremely helpful with getting Promises in here before last call, which was hard-fought. I've asked Anne if he thinks we could get a DOMException.customArg, so we can kill MediaStreamError (and MediaKeyError in EME, two birds/stone). See thread and links in [1] for the story. > r=me on the code bits, but I'm not convinced about the basic premise here.... Nothing would make me happier than nuking MediaStreamError in favor of, say, registered DOMException names, and I think the WG would be amenable, except I worry the whole promise API might blow up if we tell the WG now that they can't have their MediaStreamError.constraintName arg. All that aside, I'm also wondering about the state of DOMException in Gecko. So I wrote these patches to move the ball so we can have a concrete discussion about what the next step is. [1] http://lists.w3.org/Archives/Public/public-media-capture/2014Oct/0055.html

> nit: I think is(false,...) is more obvious to understand then null. Um... You should be using ise(), not is(), in that situation. And then you'll need to pass in the actual value you expect, not one that happens to coerce to something like it. > I'm confused. We were told by Anne that we couldn't inherit from Error. [1] OK, but then why aren't we doing that in our IDL? We can mark the interface as [ExceptionClass] to get Error.prototype on the proto chain. At least in Gecko. In the spec there seems to be no way to do it right now, as you already knew. > I'm also wondering about the state of DOMException in Gecko. Something specific about it? Mostly, DOMException is tracking the spec (except the spec just totally changed and we haven't updated to that change yet).

Jan-Ivar, sorry, I guess I could have been clearer about the Error requirement. I think long term we do want these to be proper exceptions so they can have stack traces and such. That'd be one argument to get them converted. However, using [ExceptionClass] in the face of a hostile WG seems appropriate :-/

I'm not sure you can have a reasonable stack trace for an _async_ exception (promise rejection)... What would the stack be, exactly?

(In reply to Nils Ohlmeier [:drno] from comment #26) > > + .then(function() { > > + is(null, test.error, test.message); > > nit: I think is(false,...) is more obvious to understand then null. It must be null because it compares against the expected error, which in the test cases that expect success is marked by { error: null } as opposed to { error: "soandso" } The ordering is perhaps confusing, but is most easily understood by reading the two lines that follow first (the case where the test gets an error): }, function(reason) { is(reason.name, test.error, test.message + ": " + reason.message); }); Here, the actual reason is compared against the expected reason, producing "Got <reason.name>, expected <test.error>". Applied to the success case where success (absence of error) is expected (reason == null): The actual value - success (i.e. null) since we succeeded - is compared against the expected reason, producing "Got null, expected <test.error>".

Incorporated nits and added [ExceptionClass] to MediaStreamError. Asking for re-review. (In reply to Boris Zbarsky [:bz] from comment #28) > We can mark the interface as [ExceptionClass] to get Error.prototype on the proto chain. (In reply to Boris Zbarsky [:bz] from comment #22) > Then we could add a MaybeReject variant that takes objects of this type and all that. How do I go about adding a MaybeReject variant? Thanks for the reviews!

Incorporated feedback. The use of window id predates this patch. Just making the best of it.

Removed accidental debug printfs in tests

Incorporated feedback. Made QueryInterface work with DOMLocalMediaStream. Still coerces MediaStreamError into NS_ERRORs until new promise.MaybeReject can be had.

Unbitrot. Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1653fa8aa37e

MediaStreamError being a NoInterfaceObject should not be in top level test. Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=75a1daa38ea3

> How do I go about adding a MaybeReject variant? Just add an overload in Promise.h. Something like: void MaybeReject(const MediaStreamError& aArg); And in the impl: MaybeSomething(aArg, &Promise::MaybeReject); You'll probably need to include MediaStreamError.h in whatever file has the MaybeSomething call. I would really appreciate interdiffs from the previous patches I looked at here...

Judging by try I obviously have some more work to do. Feel free to review or wait.

I'm pretty behind on other reviews, so happy to wait here until you have patches you're happy with.

Interdiffs (splinter couldn't do part 4 that one's attached. note: slightly newer): Part 3: https://bugzilla.mozilla.org/attachment.cgi?oldid=8507294&action=interdiff&newid=8509178&headers=1 Part 4: https://bugzilla.mozilla.org/attachment.cgi?oldid=8507304&action=interdiff&newid=8509085&headers=1 Part 5: https://bugzilla.mozilla.org/attachment.cgi?oldid=8507306&action=interdiff&newid=8509163&headers=1

Added #includes to compile on all platforms.

Addresses try failures. - Avoid windows.h GetMessageW macro. :-/ - Forgot MediaPermissionGonk.cpp again - Update tests still failing on try to expect spec error names: "PERMISSION_DENIED" --> "PermissionDeniedError" "NO_DEVICES_FOUND" --> "NotFoundError"

Never work on B2G in the evening. Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1b290ca64f06

> Part 5: https://bugzilla.mozilla.org/attachment.cgi?oldid=8507306&action=interdiff&newid=8509163&headers=1 The big red message about how it's a lie doesn't worry you? ;)

Updated interdiffs (part 4 attached again): Part 3: https://bugzilla.mozilla.org/attachment.cgi?oldid=8507294&action=interdiff&newid=8509229&headers=1 Part 4: https://bugzilla.mozilla.org/attachment.cgi?oldid=8507304&action=interdiff&newid=8509239&headers=1 Part 5: https://bugzilla.mozilla.org/attachment.cgi?oldid=8507306&action=interdiff&newid=8509163&headers=1

(In reply to Boris Zbarsky [:bz] from comment #45) > The big red message about how it's a lie doesn't worry you? ;) Well, I interpreted it as "nevermind the line numbers", rather than part 4's "fugedaboudit" :o) ok, let me put one together for part 5...

Really builds on windows this time. Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b26436c9487a

- Fixed browser_devices_get_user_media.js better - Unbitrot

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=da08ac704097

Sorry for the review-request spam. There was a lot of yellow, but I think this is the last one that's mine. - Fixes b2g/components/test/mochitest/test_permission_deny.html

OK green on B2G finally. Should be done now. Green try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c87a47682db0

I'm ready now. Again, sorry for the spam. Updated interdiffs (part 5 is unchanged): Part 3: https://bugzilla.mozilla.org/attachment.cgi?oldid=8507294&action=interdiff&newid=8510362&headers=1 Part 4: this patch. Part 5: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1033885&attachment=8509262

Comment on attachment 8509163 [details] [diff] [review] Part 5: add mediaDevices.getUserMedia with promises (4) Review of attachment 8509163 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDevices.cpp @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "base/basictypes.h" > +#include "mozilla/dom/MediaDevices.h" Minor nit: Shouldn't MediaDevices.h come first, and include basictypes.h if it needs it? @@ +54,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + > + // TODO: Replace with proper errors throughout. File a bug and ref here?

Removed #include "base/basictypes.h". Carrying forward r=jesup. (In reply to Randell Jesup [:jesup] from comment #56) > > + // TODO: Replace with proper errors throughout. > > File a bug and ref here? Part 7 replaces it.

Fixed leak and merged in Part 7. Carrying forward r=jesup. Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a733f0ae70ab

Unbitrot from domquake. Carrying forward r=jesup.

Unbitrot from domquake. Carrying forward r=jesup.

Unbitrot from domquake. Carrying forward r=jesup.

Part 3 interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8507294&action=interdiff&newid=8511764&headers=1

More stable compile-fix for Windows (other solution stopped working with the domshift). Carrying forward r=jesup.

Comment on attachment 8511984 [details] [diff] [review] Part 3: add MediaStreamError interface (7) r=jesup r=me. Thank you for the interdiff!

Comment on attachment 8511767 [details] [diff] [review] Part 4: return MediaStreamError instead of error strings, to spec (9) r=jesup >+++ b/dom/media/MediaManager.cpp >+ auto window = nsGlobalWindow::GetInnerWindowWithId(mWindowID); s/auto/nsGlobalWindow*/ or whatever pointer type you really want here is not that painful to write and makes it clear that this is not in fact leaking and so forth... Please do that, all four places. r=me. Thank you for the interdiff; it made things much simpler!

Comment on attachment 8511768 [details] [diff] [review] Part 5: add mediaDevices.getUserMedia with promises (7) r=jesup >+ auto window = GetOwner(); Again, would prefer using an actual type here. I'm probably OK with using MaybeRejectBrokenly here, though given this _is_ a subclass of Error I would be OK with MaybeReject too.

Green (on our stuff) try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ce485d5c3509

Forgot one #undef GetMessage to fully compile on Windows. Carrying forward r=jesup, r=bz.

Incorporated feedback. Carrying forward r=jesup, bz.

Incorporated feedback and switched from MaybeRejectBrokenly to MaybeReject. Carrying forward r=jesup, r=bz.

No change.

What happened to Part 2? Also, does this need a leave-open anymore?

Isn't Part 2 here? attachment 8497848 [details] [diff] [review] I have two more patches of work to reach the ambitious title of this bug, but opening a new bug for those probably makes sense.

Ugh, I meant Part 2 is attachment 8507291 [details] [diff] [review].

https://hg.mozilla.org/integration/mozilla-inbound/rev/08f011fc1b47 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c4df7a3a1bb https://hg.mozilla.org/integration/mozilla-inbound/rev/6f383f465a4d https://hg.mozilla.org/integration/mozilla-inbound/rev/a461238e8fb2 https://hg.mozilla.org/integration/mozilla-inbound/rev/162d085b8019

https://hg.mozilla.org/mozilla-central/rev/08f011fc1b47 https://hg.mozilla.org/mozilla-central/rev/2c4df7a3a1bb https://hg.mozilla.org/mozilla-central/rev/6f383f465a4d https://hg.mozilla.org/mozilla-central/rev/a461238e8fb2 https://hg.mozilla.org/mozilla-central/rev/162d085b8019

This work was finished some time ago in the context of dealing with docs for another bug.