This is a reminder to remove the dictionary-union workaround in Bug 882145 once Bug 767924 is fixed.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #0) > This is a reminder to remove the dictionary-union workaround in Bug 882145 > once Bug 767924 is fixed. Make that once Bug 918011 is fixed.

With this patch on top of move-rooted-dictionary and dict-in-union from bug 918011, I get his: Traceback (most recent call last): File ".../mozilla-central/config/pythonpath.py", line 56, in <module> main(sys.argv[1:]) File ".../mozilla-central/config/pythonpath.py", line 48, in main execfile(script, frozenglobals) File ".../mozilla-central/dom/bindings/GlobalGen.py", line 81, in <module> main() File ".../mozilla-central/dom/bindings/GlobalGen.py", line 76, in main generate_file(config, 'UnionTypes', 'declare') File .../mozilla-central/dom/bindings/GlobalGen.py", line 16, in generate_file root = getattr(GlobalGenRoots, name)(config) File ".../mozilla-central/dom/bindings/Codegen.py", line 10667, in UnionTypes config) File ".../mozilla-central/dom/bindings/Codegen.py", line 845, in UnionTypes map(addInfoForType, getAllTypes(descriptors, dictionaries, callbacks)) File ".../mozilla-central/dom/bindings/Codegen.py", line 840, in addInfoForType if typeNeedsRooting(f): TypeError: typeNeedsRooting() takes exactly 2 arguments (1 given)

Sorry, the patches in bug 918011 depend on bug 917958, but I had forgotten to mark the dependency...

Thanks! That fixed the codegen. However, I get compile errors when I try to use it in this patch (note that I've moved MediaStreamConstraintsInternal to a different webidl file, MediaStream.webidl, as suggested): .../mozilla-central/dom/media/MediaManager.cpp:757:7: error: call to deleted constructor of 'mozilla::dom::MediaStreamConstraintsInternal' : mConstraints(aConstraints) ^ ~~~~~~~~~~~~ ../../dist/include/mozilla/dom/MediaStreamBinding.h:80:3: note: function has been explicitly marked deleted here MediaStreamConstraintsInternal(const MediaStreamConstraintsInternal&) MOZ_DELETE; Looks like MOZ_DELETE came back. Here's a diff of the generated code before vs now: @@ -1,21 +1,22 @@ struct MediaStreamConstraintsInternal : public MainThreadDictionaryBase { - bool mAudio; - MediaTrackConstraintsInternal mAudiom; + OwningBooleanOrMediaTrackConstraintsInternal mAudio; bool mFake; bool mPicture; - bool mVideo; - MediaTrackConstraintsInternal mVideom; + OwningBooleanOrMediaTrackConstraintsInternal mVideo; inline MediaStreamConstraintsInternal() { } - explicit inline MediaStreamConstraintsInternal(const MediaStreamConstraintsInternal& aOther) - { - *this = aOther; - } +private: + MediaStreamConstraintsInternal(const MediaStreamConstraintsInternal&) MOZ_DELETE; + void operator=(const MediaStreamConstraintsInternal) MOZ_DELETE; + + static bool + InitIds(JSContext* cx, MediaStreamConstraintsInternalAtoms* atomsCache); +public: bool Init(JSContext* cx, JS::Handle<JS::Value> val, const char* sourceDescription = "Value"); @@ -27,13 +28,6 @@ void TraceDictionary(JSTracer* trc); - - void - operator=(const MediaStreamConstraintsInternal& aOther); - -private: - static bool - InitIds(JSContext* cx, MediaStreamConstraintsInternalAtoms* atomsCache); }; struct MediaStreamConstraintsInternalInitializer : public MediaStreamConstraintsInternal {

Yes, no copy ctor on unions.... Let me see if I can come up with a way to support that, maybe.

(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #5) > Yes, no copy ctor on unions.... > > Let me see if I can come up with a way to support that, maybe. I've filed bug 925737 for this.

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

Missed gonk. Try - https://tbpl.mozilla.org/?tree=Try&rev=ddf59465488f

Improved invariants.

Comment on attachment 8401043 [details] [diff] [review] Cleaned up gUM to use webidl unions + filter by media-type (clobber needed) (3) Review of attachment 8401043 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaManager.cpp @@ +1266,5 @@ > > dom::RootedDictionary<MediaStreamConstraintsInternal> c(aCx); > + JS::Rooted<JS::Value> temp(aCx); > + aRawConstraints.ToObject(aCx, &temp); > + bool success = c.Init(aCx, temp); bz, if you could look specifically at the copying here of MediaStreamConstraints into MediaStreamConstraintsInternal that would be great. I had a more explicit member-by-member copy routine here earlier that relied on the c++ binding code, but this seems to require much less maintenance, and seems safe given the circumstances. Hope it's ok. Thanks.

Comment on attachment 8401043 [details] [diff] [review] Cleaned up gUM to use webidl unions + filter by media-type (clobber needed) (3) Review of attachment 8401043 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits. Note that I don't really have the background necessary to review the JS context stuff, but bz will more than make up for that. ::: dom/media/MediaManager.cpp @@ +72,5 @@ > #else > #define LOG(msg) > #endif > > +using namespace dom; I'm not a huge fan of using namespace. Maybe this code should just be in namespace mozilla::dom if we're resorting to this? The source code is all in mozilla/dom/... Not going to r- over this or anything, just my two cents. @@ +659,5 @@ > char* media_device_name = nullptr) > { > + ScopedDeletePtr<SourceSet> result(new SourceSet); > + > + if (aConstraints.IsBoolean() && !aConstraints.GetAsBoolean()) { Isn't this just if(!IsOn(aConstraints)) ? @@ +693,5 @@ > #endif > } > } > > + if (!aConstraints.IsMediaTrackConstraintsInternal()) { Might be helpful to add a comment like // eg; {audio: true} @@ +754,5 @@ > +static JSObject * > +GetMandatoryJSObj(const dom::OwningBooleanOrMediaTrackConstraints &aUnion) { > + return aUnion.IsMediaTrackConstraints() ? > + (aUnion.GetAsMediaTrackConstraints().mMandatory.WasPassed() ? > + aUnion.GetAsMediaTrackConstraints().mMandatory.Value() : nullptr) : nullptr; Nested ternary, would be a lot more readable as if (aUnion.IsMediaTrackConstraint() && aUnion.GetAsMediaTrackConstraints().mMandatory.WasPassed()) { return aUnion.GetAsMediaTrackConstraints().mMandatory.Value(); } return nullptr;

Comment on attachment 8401043 [details] [diff] [review] Cleaned up gUM to use webidl unions + filter by media-type (clobber needed) (3) I'd generally avoid including binding headers in a header unless you really have to, since they pull in some annoying jsapi bits. Forward declarations of whatever it is you need might be nicer. Then again, there was one there already... + return aUnion.IsMediaTrackConstraints() ? + (aUnion.GetAsMediaTrackConstraints().mMandatory.WasPassed() ? + aUnion.GetAsMediaTrackConstraints().mMandatory.Value() : nullptr) : nullptr; Maybe "return foo && bar ? baz : nullptr" instead of "return foo ? (bar ? baz : nullptr) : nullptr"? >+ JSObject *audioManObj = GetMandatoryJSObj(aRawConstraints.mAudio); >+ JSObject *videoManObj = GetMandatoryJSObj(aRawConstraints.mVideo); These both need to be JS::Rooted<JSObject*>. I assume the static analysis box would have ended up catching that.... >+ aRawConstraints.ToObject(aCx, &temp); >+ bool success = c.Init(aCx, temp); This does work; it's just not the fastest thing in the world. Not sure ho much that matters here. It _definitely_ needs a comment, though. >+++ b/dom/webidl/MediaStream.webidl And a comment for why the dictionaries need to be in this file. r=me with those fixed.

(In reply to Byron Campen [:bwc] from comment #12) > > +using namespace dom; > > I'm not a huge fan of using namespace. Maybe this code should just be in > namespace mozilla::dom if we're resorting to this? The source code is all in > mozilla/dom/... Not going to r- over this or anything, just my two cents. It seems common enough in the tree, and I don't think this belongs in dom. To me, a great thing about namespaces over long_names is that they can be opened. If we're not to open them in .cpp files, then when? But since MediaManager.cpp is quite the file, and I tossed some comments in the bathwater here, I'll restore it for now so we can debate this at our leisure. > > + if (aConstraints.IsBoolean() && !aConstraints.GetAsBoolean()) { > > Isn't this just if(!IsOn(aConstraints)) ? Yes! Thanks for spotting that.

Updated with feedback. Carrying forward r+ from bwc and bz (*) (In reply to Boris Zbarsky [:bz] from comment #13) > I'd generally avoid including binding headers in a header unless you really > have to, since they pull in some annoying jsapi bits. Forward declarations > of whatever it is you need might be nicer. *) I couldn't forward-declare my way out of it since MediaStreamConstraintsInternal was embedded, so I ended up using an nsAutoPtr. Please look it over to see if you're ok with forwarded r+.

Comment on attachment 8401782 [details] [diff] [review] Cleaned up gUM to use webidl unions + filter by media-type (clobber needed) (4) r=bz, bwc Ah, hmm. I'm ok with including in the header if it's really needed; doing the extra heap-allocation seems a little silly.

Thanks, but it's just a tiny allocation once for every getUserMedia permission prompt shown to the user, so it hardly matters. Not including everything everywhere is nice too.