Update gUM with mediaDevices.getUserMedia

RESOLVED FIXED in mozilla36

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: jib, Assigned: jib)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla36
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=2], URL)

Attachments

(8 attachments, 37 obsolete attachments)

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]
Created attachment 8492266 [details] [diff] [review]
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.
Attachment #8492266 - Flags: review?(rjesup)
Created attachment 8492271 [details] [diff] [review]
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
Attachment #8492271 - Flags: feedback?(rjesup)
Attachment #8492271 - Flags: feedback?(annevk)
Created attachment 8492274 [details]
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.
Created attachment 8492374 [details]
pc_test_promise.html

Added in gUM polyfill so you can try it out without this patch.
Attachment #8492274 - Attachment is obsolete: true
Created attachment 8492552 [details]
pc_test_promise.html

Fixed some typos.
Attachment #8492374 - Attachment is obsolete: true
Created attachment 8492555 [details] [diff] [review]
add mediaDevices.getUserMedia with promises (2)

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)
Created attachment 8492620 [details]
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+
Created attachment 8497848 [details] [diff] [review]
Part 1: minor refactor of MediaManager::GetUserMedia to figure out privileged state itself (2) r=jesup

Updated nits. Carrying forward r=jesup.
Attachment #8492266 - Attachment is obsolete: true
Attachment #8497848 - Flags: review+
Keywords: checkin-needed, leave-open
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/5247a092fcd2
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5247a092fcd2
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
Created attachment 8507291 [details] [diff] [review]
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).
Attachment #8507291 - Flags: review?(rjesup)
Created attachment 8507294 [details] [diff] [review]
Part 3: add MediaStreamError interface

Implements http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-MediaStreamError
Attachment #8507294 - Flags: review?(rjesup)
Attachment #8507294 - Flags: review?(bzbarsky)
Created attachment 8507304 [details] [diff] [review]
Part 4: return MediaStreamError instead of error strings, to spec.
Attachment #8507304 - Flags: review?(rjesup)
Attachment #8507304 - Flags: review?(bzbarsky)
Created attachment 8507306 [details] [diff] [review]
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
Flags: needinfo?(bzbarsky)
Attachment #8507306 - Flags: review?(rjesup)
Attachment #8507306 - Flags: review?(bzbarsky)
Created attachment 8507310 [details] [diff] [review]
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.
Attachment #8507310 - Flags: review?(drno)
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....
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).

Comment 29

3 years ago
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)
Keywords: dev-doc-needed
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>".
Created attachment 8509030 [details] [diff] [review]
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!
Attachment #8509030 - Flags: review?(bzbarsky)
Attachment #8507294 - Attachment is obsolete: true
Attachment #8507294 - Flags: review?(rjesup)
Created attachment 8509080 [details] [diff] [review]
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.
Attachment #8507304 - Attachment is obsolete: true
Attachment #8507304 - Flags: review?(rjesup)
Attachment #8509080 - Flags: review?(rjesup)
Attachment #8509080 - Flags: review?(bzbarsky)
Created attachment 8509085 [details] [diff] [review]
Part 4: return MediaStreamError instead of error strings, to spec (3)

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)
Created attachment 8509163 [details] [diff] [review]
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.
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)
Created attachment 8509166 [details] [diff] [review]
Part 6: tests for mediaDevices.getUserMedia (2) r=drno

Unbitrot.

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1653fa8aa37e
Attachment #8507310 - Attachment is obsolete: true
Attachment #8509166 - Flags: review+
Created attachment 8509178 [details] [diff] [review]
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
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.
Created attachment 8509215 [details]
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
Created attachment 8509229 [details] [diff] [review]
Part 3: add MediaStreamError interface (4)

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)
Created attachment 8509238 [details] [diff] [review]
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"
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)
Created attachment 8509239 [details] [diff] [review]
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
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?  ;)
Created attachment 8509250 [details] [diff] [review]
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
Attachment #8509215 - Attachment is obsolete: true
(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...
Created attachment 8509262 [details] [diff] [review]
interdiff part 5
Created attachment 8510362 [details] [diff] [review]
Part 3: add MediaStreamError interface (5)

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)
Created attachment 8510366 [details] [diff] [review]
Part 4: return MediaStreamError instead of error strings, to spec (6)

- 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)
Created attachment 8510369 [details] [diff] [review]
Part 7: tolerate MediaStreamError as a promise rejection reason
Attachment #8510369 - Flags: review?(rjesup)
Attachment #8510369 - Flags: review?(bzbarsky)
Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=da08ac704097
Created attachment 8510550 [details] [diff] [review]
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
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)
Created attachment 8511071 [details] [diff] [review]
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
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)
Created attachment 8511101 [details] [diff] [review]
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
Attachment #8509250 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Attachment #8511101 - Attachment is patch: true

Updated

3 years ago
Attachment #8507291 - Flags: review?(rjesup) → review+

Updated

3 years ago
Attachment #8510362 - Flags: review?(rjesup) → review+

Updated

3 years ago
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+

Updated

3 years ago
Attachment #8510369 - Flags: review?(rjesup) → review+
Created attachment 8511292 [details] [diff] [review]
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.
Attachment #8509163 - Attachment is obsolete: true
Attachment #8509163 - Flags: review?(bzbarsky)
Attachment #8511292 - Flags: review?(bzbarsky)
Created attachment 8511736 [details] [diff] [review]
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
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)
Created attachment 8511742 [details] [diff] [review]
interdiff part 5 (2)
Attachment #8509262 - Attachment is obsolete: true
Attachment #8511742 - Attachment is patch: true
Keywords: leave-open
Keywords: leave-open
Created attachment 8511764 [details] [diff] [review]
Part 3: add MediaStreamError interface (6) r=jesup

Unbitrot from domquake. Carrying forward r=jesup.
Attachment #8510362 - Attachment is obsolete: true
Attachment #8510362 - Flags: review?(bzbarsky)
Attachment #8511764 - Flags: review?(bzbarsky)
Created attachment 8511767 [details] [diff] [review]
Part 4: return MediaStreamError instead of error strings, to spec (9) r=jesup

Unbitrot from domquake. Carrying forward r=jesup.
Attachment #8511071 - Attachment is obsolete: true
Attachment #8511071 - Flags: review?(bzbarsky)
Attachment #8511767 - Flags: review?(bzbarsky)
Created attachment 8511768 [details] [diff] [review]
Part 5: add mediaDevices.getUserMedia with promises (7) r=jesup

Unbitrot from domquake. Carrying forward r=jesup.
Attachment #8511736 - Attachment is obsolete: true
Attachment #8511736 - Flags: review?(bzbarsky)
Attachment #8511768 - Flags: review?(bzbarsky)
Part 3 interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8507294&action=interdiff&newid=8511764&headers=1
Created attachment 8511773 [details] [diff] [review]
interdiff part 4 (3)
Attachment #8511101 - Attachment is obsolete: true
Created attachment 8511774 [details] [diff] [review]
interdiff part 5 (3)
Attachment #8511742 - Attachment is obsolete: true
Created attachment 8511984 [details] [diff] [review]
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.
Attachment #8511764 - Attachment is obsolete: true
Attachment #8511764 - Flags: review?(bzbarsky)
Attachment #8511984 - Flags: review?(bzbarsky)
Created attachment 8511985 [details] [diff] [review]
interdiff part 3
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+
Green (on our stuff) try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ce485d5c3509
Created attachment 8512172 [details] [diff] [review]
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.
Attachment #8511984 - Attachment is obsolete: true
Attachment #8512172 - Flags: review+
Created attachment 8512176 [details] [diff] [review]
Part 4: return MediaStreamError instead of error strings, to spec (9) r=jesup, r=bz

Incorporated feedback. Carrying forward r=jesup, bz.
Attachment #8512176 - Flags: review+
Created attachment 8512177 [details] [diff] [review]
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.
Attachment #8511767 - Attachment is obsolete: true
Attachment #8511768 - Attachment is obsolete: true
Attachment #8512177 - Flags: review+
Created attachment 8512185 [details] [diff] [review]
Part 6: tests for mediaDevices.getUserMedia (3) r=drno

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
Keywords: checkin-needed
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
Comment hidden (typo)
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
Flags: in-testsuite+
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1050930
Depends on: 1301196
This work was finished some time ago in the context of dealing with docs for another bug.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.