Closed Bug 1033885 Opened 10 years ago Closed 10 years ago

Update gUM with mediaDevices.getUserMedia

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jib, Assigned: jib)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [p=2])

Attachments

(8 files, 37 obsolete files)

15.63 KB, text/html
Details
1.77 KB, text/html
Details
8.70 KB, patch
jib
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
30.83 KB, patch
jesup
: review+
Details | Diff | Splinter Review
7.32 KB, patch
jib
: review+
Details | Diff | Splinter Review
37.52 KB, patch
jib
: review+
Details | Diff | Splinter Review
17.56 KB, patch
jib
: review+
Details | Diff | Splinter Review
5.49 KB, patch
jib
: review+
Details | Diff | Splinter Review
- Move getUserMedia() from Navigator to MediaDevices.
- Add MediaDevices.getSupportedConstraints() function.
- remove require
- add ideal/exact
- DOMString facingMode and mediaSource.
Assignee: nobody → jib
Whiteboard: [p=2]
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.
Attachment #8492266 - Flags: review?(rjesup)
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
Attachment #8492271 - Flags: feedback?(rjesup)
Attachment #8492271 - Flags: feedback?(annevk)
Attached file pc_test_promise.html (obsolete) —
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.
Attached file pc_test_promise.html (obsolete) —
Added in gUM polyfill so you can try it out without this patch.
Attachment #8492274 - Attachment is obsolete: true
Attached file pc_test_promise.html
Fixed some typos.
Attachment #8492374 - Attachment is obsolete: true
Should compile on Windows.

Try - https://tbpl.mozilla.org/?tree=Try&rev=ed433cc24a42
Attachment #8492271 - Attachment is obsolete: true
Attachment #8492271 - Flags: feedback?(rjesup)
Attachment #8492271 - Flags: feedback?(annevk)
Attached file gum.html
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
Attachment #8492266 - Flags: review?(rjesup) → review+
Updated nits. Carrying forward r=jesup.
Attachment #8492266 - Attachment is obsolete: true
Attachment #8497848 - Flags: review+
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 ?
Flags: needinfo?(jib)
Keywords: checkin-needed
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+.
Flags: needinfo?(jib)
Keywords: checkin-needed
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.
Attachment #8497848 - Attachment description: minor refactor of MediaManager::GetUserMedia to figure out privileged state itself (2) r=jesup → Part1: minor refactor of MediaManager::GetUserMedia to figure out privileged state itself (2) r=jesup
Attachment #8497848 - Attachment description: Part1: minor refactor of MediaManager::GetUserMedia to figure out privileged state itself (2) r=jesup → Part 1: minor refactor of MediaManager::GetUserMedia to figure out privileged state itself (2) r=jesup
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).
Attachment #8507291 - Flags: review?(rjesup)
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
Flags: needinfo?(bzbarsky)
Attachment #8507306 - Flags: review?(rjesup)
Attachment #8507306 - Flags: review?(bzbarsky)
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.
Attachment #8507310 - Flags: review?(drno)
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....
Flags: needinfo?(bzbarsky)
Attachment #8507294 - Flags: review?(bzbarsky) → review+
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.
Attachment #8507304 - Flags: review?(bzbarsky) → review-
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.
Attachment #8507306 - Flags: review?(bzbarsky) → review-
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.
Attachment #8507310 - Flags: review?(drno) → review+
(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
Flags: needinfo?(annevk)
> 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 :-/
Flags: needinfo?(annevk)
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!
Attachment #8509030 - Flags: review?(bzbarsky)
Attachment #8507294 - Attachment is obsolete: true
Attachment #8507294 - Flags: review?(rjesup)
Incorporated feedback. The use of window id predates this patch. Just making the best of it.
Attachment #8507304 - Attachment is obsolete: true
Attachment #8507304 - Flags: review?(rjesup)
Attachment #8509080 - Flags: review?(rjesup)
Attachment #8509080 - Flags: review?(bzbarsky)
Removed accidental debug printfs in tests
Attachment #8509080 - Attachment is obsolete: true
Attachment #8509080 - Flags: review?(rjesup)
Attachment #8509080 - Flags: review?(bzbarsky)
Attachment #8509085 - Flags: review?(rjesup)
Attachment #8509085 - Flags: review?(bzbarsky)
Attachment #8509030 - Flags: review?(rjesup)
Attachment #8507306 - Attachment description: Part 5: add mediaDevices.getUserMedia with promises → Part 5: add mediaDevices.getUserMedia with promises (3)
Incorporated feedback. Made QueryInterface work with DOMLocalMediaStream.

Still coerces MediaStreamError into NS_ERRORs until new promise.MaybeReject can be had.
Attachment #8492555 - Attachment is obsolete: true
Attachment #8507306 - Attachment is obsolete: true
Attachment #8507306 - Flags: review?(rjesup)
Attachment #8509163 - Flags: review?(rjesup)
Attachment #8509163 - Flags: review?(bzbarsky)
MediaStreamError being a NoInterfaceObject should not be in top level test.

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=75a1daa38ea3
Attachment #8509030 - Attachment is obsolete: true
Attachment #8509030 - Flags: review?(rjesup)
Attachment #8509030 - Flags: review?(bzbarsky)
Attachment #8509178 - Flags: review?(rjesup)
Attachment #8509178 - Flags: review?(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...
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.
Added #includes to compile on all platforms.
Attachment #8509178 - Attachment is obsolete: true
Attachment #8509178 - Flags: review?(rjesup)
Attachment #8509178 - Flags: review?(bzbarsky)
Attachment #8509229 - Flags: review?(rjesup)
Attachment #8509229 - Flags: review?(bzbarsky)
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"
Attachment #8509085 - Attachment is obsolete: true
Attachment #8509085 - Flags: review?(rjesup)
Attachment #8509085 - Flags: review?(bzbarsky)
Attachment #8509238 - Flags: review?(rjesup)
Attachment #8509238 - Flags: review?(bzbarsky)
Never work on B2G in the evening.

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1b290ca64f06
Attachment #8509238 - Attachment is obsolete: true
Attachment #8509238 - Flags: review?(rjesup)
Attachment #8509238 - Flags: review?(bzbarsky)
Attachment #8509239 - Flags: review?(rjesup)
Attachment #8509239 - Flags: review?(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?  ;)
(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...
Attached patch interdiff part 5 (obsolete) — Splinter Review
Really builds on windows this time.

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b26436c9487a
Attachment #8509229 - Attachment is obsolete: true
Attachment #8509229 - Flags: review?(rjesup)
Attachment #8509229 - Flags: review?(bzbarsky)
Attachment #8510362 - Flags: review?(rjesup)
Attachment #8510362 - Flags: review?(bzbarsky)
- Fixed browser_devices_get_user_media.js better
- Unbitrot
Attachment #8509239 - Attachment is obsolete: true
Attachment #8509239 - Flags: review?(rjesup)
Attachment #8509239 - Flags: review?(bzbarsky)
Attachment #8510366 - Flags: review?(rjesup)
Attachment #8510366 - Flags: review?(bzbarsky)
Attachment #8510369 - Flags: review?(rjesup)
Attachment #8510369 - Flags: review?(bzbarsky)
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
Attachment #8510366 - Attachment is obsolete: true
Attachment #8510366 - Flags: review?(rjesup)
Attachment #8510366 - Flags: review?(bzbarsky)
Attachment #8510550 - Flags: review?(rjesup)
Attachment #8510550 - Flags: review?(bzbarsky)
OK green on B2G finally. Should be done now.

Green try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c87a47682db0
Attachment #8510550 - Attachment is obsolete: true
Attachment #8510550 - Flags: review?(rjesup)
Attachment #8510550 - Flags: review?(bzbarsky)
Attachment #8511071 - Flags: review?(rjesup)
Attachment #8511071 - Flags: review?(bzbarsky)
Attached patch interdiff part 4 (2) (obsolete) — Splinter Review
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
Attachment #8509250 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Attachment #8511101 - Attachment is patch: true
Attachment #8507291 - Flags: review?(rjesup) → review+
Attachment #8510362 - Flags: review?(rjesup) → review+
Attachment #8511071 - Flags: review?(rjesup) → review+
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?
Attachment #8509163 - Flags: review?(rjesup) → review+
Attachment #8510369 - Flags: review?(rjesup) → review+
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.
Attachment #8509163 - Attachment is obsolete: true
Attachment #8509163 - Flags: review?(bzbarsky)
Attachment #8511292 - Flags: review?(bzbarsky)
Fixed leak and merged in Part 7. Carrying forward r=jesup.

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a733f0ae70ab
Attachment #8510369 - Attachment is obsolete: true
Attachment #8511292 - Attachment is obsolete: true
Attachment #8510369 - Flags: review?(bzbarsky)
Attachment #8511292 - Flags: review?(bzbarsky)
Attachment #8511736 - Flags: review?(bzbarsky)
Attached patch interdiff part 5 (2) (obsolete) — Splinter Review
Attachment #8509262 - Attachment is obsolete: true
Attachment #8511742 - Attachment is patch: true
Keywords: leave-open
Keywords: leave-open
Unbitrot from domquake. Carrying forward r=jesup.
Attachment #8510362 - Attachment is obsolete: true
Attachment #8510362 - Flags: review?(bzbarsky)
Attachment #8511764 - Flags: review?(bzbarsky)
Unbitrot from domquake. Carrying forward r=jesup.
Attachment #8511071 - Attachment is obsolete: true
Attachment #8511071 - Flags: review?(bzbarsky)
Attachment #8511767 - Flags: review?(bzbarsky)
Unbitrot from domquake. Carrying forward r=jesup.
Attachment #8511736 - Attachment is obsolete: true
Attachment #8511736 - Flags: review?(bzbarsky)
Attachment #8511768 - Flags: review?(bzbarsky)
Attached patch interdiff part 4 (3) (obsolete) — Splinter Review
Attachment #8511101 - Attachment is obsolete: true
Attached patch interdiff part 5 (3) (obsolete) — Splinter Review
Attachment #8511742 - Attachment is obsolete: true
More stable compile-fix for Windows (other solution stopped working with the domshift). Carrying forward r=jesup.
Attachment #8511764 - Attachment is obsolete: true
Attachment #8511764 - Flags: review?(bzbarsky)
Attachment #8511984 - Flags: review?(bzbarsky)
Attached patch interdiff part 3 (obsolete) — Splinter Review
Comment on attachment 8511984 [details] [diff] [review]
Part 3: add MediaStreamError interface (7) r=jesup

r=me.  Thank you for the interdiff!
Flags: needinfo?(bzbarsky)
Attachment #8511984 - Flags: review?(bzbarsky) → review+
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!
Attachment #8511767 - Flags: review?(bzbarsky) → review+
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.
Attachment #8511768 - Flags: review?(bzbarsky) → review+
Forgot one #undef GetMessage to fully compile on Windows.

Carrying forward r=jesup, r=bz.
Attachment #8511984 - Attachment is obsolete: true
Attachment #8512172 - Flags: review+
Incorporated feedback. Carrying forward r=jesup, bz.
Attachment #8512176 - Flags: review+
Incorporated feedback and switched from MaybeRejectBrokenly to MaybeReject.
Carrying forward r=jesup, r=bz.
Attachment #8511767 - Attachment is obsolete: true
Attachment #8511768 - Attachment is obsolete: true
Attachment #8512177 - Flags: review+
No change.
Attachment #8511773 - Attachment is obsolete: true
Attachment #8511774 - Attachment is obsolete: true
Attachment #8511985 - Attachment is obsolete: true
Attachment #8512185 - Flags: review+
Attachment #8509166 - Attachment is obsolete: true
Attachment #8497848 - Flags: checkin+
What happened to Part 2? Also, does this need a leave-open anymore?
Flags: needinfo?(jib)
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.
Flags: needinfo?(jib)
Keywords: leave-open
Summary: Update getUserMedia constraints to spec → Update gUM with mediaDevices.getUserMedia
Blocks: 1050930
Depends on: 1301196
This work was finished some time ago in the context of dealing with docs for another bug.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: