1033885 - Update gUM with mediaDevices.getUserMedia

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

- DOMString facingMode and mediaSource.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: minor refactor of MediaManager::GetUserMedia to figure out privileged state itself

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.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: add mediaDevices.getUserMedia with promises

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

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: pc_test_promise.html

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.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: pc_test_promise.html

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

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: pc_test_promise.html

Fixed some typos.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: add mediaDevices.getUserMedia with promises (2)

Should compile on Windows.

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

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: gum.html

Super-simple mediaDevices.getUserMedia promise test.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 1: minor refactor of MediaManager::GetUserMedia to figure out privileged state itself (2) r=jesup

Updated nits. Carrying forward r=jesup.

Comment 11

[Carsten Book [:Tomcat]]

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 ?

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5247a092fcd2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5247a092fcd2

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 2: rename error to onFailure in code before introducing an actual error object

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).

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: add MediaStreamError interface

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

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: return MediaStreamError instead of error strings, to spec.

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 5: add mediaDevices.getUserMedia with promises (3)

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

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 6: tests for mediaDevices.getUserMedia

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.

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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 23

[Boris Zbarsky [:bzbarsky]]

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 24

[Boris Zbarsky [:bzbarsky]]

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 25

[Boris Zbarsky [:bzbarsky]]

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 26

[Nils Ohlmeier [:drno]]

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.

Comment 27

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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

Comment 28

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 29

[Anne (:annevk)]

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 :-/

Comment 30

[Boris Zbarsky [:bzbarsky]]

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

Comment 31

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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>".

Comment 32

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: add MediaStreamError interface (2)

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!

Comment 33

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: return MediaStreamError instead of error strings, to spec (2)

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

Comment 34

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: return MediaStreamError instead of error strings, to spec (3)

Removed accidental debug printfs in tests

Comment 35

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 5: add mediaDevices.getUserMedia with promises (4)

Incorporated feedback. Made QueryInterface work with DOMLocalMediaStream.

Still coerces MediaStreamError into NS_ERRORs until new promise.MaybeReject can be had.

Comment 36

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 6: tests for mediaDevices.getUserMedia (2) r=drno

Unbitrot.

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

Comment 37

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: add MediaStreamError interface (3)

MediaStreamError being a NoInterfaceObject should not be in top level test.

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

Comment 38

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 39

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 40

[Boris Zbarsky [:bzbarsky]]

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

Comment 41

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: interdiff part 4

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

Comment 42

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: add MediaStreamError interface (4)

Added #includes to compile on all platforms.

Comment 43

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: return MediaStreamError instead of error strings, to spec (4)

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"

Comment 44

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: return MediaStreamError instead of error strings, to spec (5)

Never work on B2G in the evening.

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

Comment 45

[Boris Zbarsky [:bzbarsky]]

> 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?  ;)

Comment 46

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: interdiff part 4

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

Comment 47

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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...

Comment 48

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: interdiff part 5

Comment 49

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: add MediaStreamError interface (5)

Really builds on windows this time.

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

Comment 50

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: return MediaStreamError instead of error strings, to spec (6)

- Fixed browser_devices_get_user_media.js better
- Unbitrot

Comment 51

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 7: tolerate MediaStreamError as a promise rejection reason

Comment 52

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 53

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: return MediaStreamError instead of error strings, to spec (7)

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

Comment 54

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: return MediaStreamError instead of error strings, to spec (8)

OK green on B2G finally. Should be done now.

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

Comment 55

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: interdiff part 4 (2)

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 56

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 57

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 5: add mediaDevices.getUserMedia with promises (5) r=jesup

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.

Comment 58

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 5: add mediaDevices.getUserMedia with promises (6) r=jesup

Fixed leak and merged in Part 7. Carrying forward r=jesup.

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

Comment 59

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: interdiff part 5 (2)

Comment 60

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: add MediaStreamError interface (6) r=jesup

Unbitrot from domquake. Carrying forward r=jesup.

Comment 61

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: return MediaStreamError instead of error strings, to spec (9) r=jesup

Unbitrot from domquake. Carrying forward r=jesup.

Comment 62

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 5: add mediaDevices.getUserMedia with promises (7) r=jesup

Unbitrot from domquake. Carrying forward r=jesup.

Comment 63

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 64

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: interdiff part 4 (3)

Comment 65

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: interdiff part 5 (3)

Comment 66

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: add MediaStreamError interface (7) r=jesup

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

Comment 67

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: interdiff part 3

Comment 68

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8511984 [details] [diff] [review]
Part 3: add MediaStreamError interface (7) r=jesup

r=me.  Thank you for the interdiff!

Comment 69

[Boris Zbarsky [:bzbarsky]]

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 70

[Boris Zbarsky [:bzbarsky]]

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.

Comment 71

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 72

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 3: add MediaStreamError interface (8) r=jesup, r=bz

Forgot one #undef GetMessage to fully compile on Windows.

Carrying forward r=jesup, r=bz.

Comment 73

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 4: return MediaStreamError instead of error strings, to spec (9) r=jesup, r=bz

Incorporated feedback. Carrying forward r=jesup, bz.

Comment 74

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 5: add mediaDevices.getUserMedia with promises (8) r=jesup, r=bz

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

Comment 75

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Part 6: tests for mediaDevices.getUserMedia (3) r=drno

No change.

Comment 76

[Ryan VanderMeulen [:RyanVM]]

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

Comment 77

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 78

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

oh I see

Comment 79

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 80

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 81

[Carsten Book [:Tomcat]]

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

Comment 82

[Eric Shepherd [:sheppy]]

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