Florian Quèze [:florian] Reporter
	Description • 10 years ago

The current API http://mxr.mozilla.org/mozilla-central/source/dom/media/nsIMediaManager.idl#22 :
void mediaCaptureWindowState(in nsIDOMWindow aWindow, out boolean aVideo, out boolean aAudio);

is no longer enough now that we are adding support for screen/window sharing (bug 983504).

	Paul Kerr [:pkerr]
	Comment 2 • 10 years ago

Comment on attachment 8456936 [details] [diff] [review]
Add screen and window sharing booleans to MediaCaptureWindowState

Review of attachment 8456936 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +329,5 @@
>  
> +  virtual const dom::MediaSourceEnum GetMediaSource() {
> +    return dom::MediaSourceEnum::Microphone;
> +  }
> +

I am assuming, since I do not see a matching method on MediaEngineWebRTCVideoSource, that the audio source is always a dom::MediaSourceEnum::Microphone but that the GetMediaSource() for video is sorted out in the subclass.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 4 • 10 years ago

Comment on attachment 8457736 [details] [diff] [review]
interdiffs for changes to avoid over-reuse of dom::MediaSourceEnum

The MediaSourceEnum dom type is for Video constraints, so adding microphone to it wouldn't be reasonable or match the proposal; use our own internal enum that's a superset of that.

	Gian-Carlo Pascutto [:gcp]
	Comment 5 • 10 years ago

Comment on attachment 8457736 [details] [diff] [review]
interdiffs for changes to avoid over-reuse of dom::MediaSourceEnum

Review of attachment 8457736 [details] [diff] [review]:
-----------------------------------------------------------------

This reeks of "we're going to have to replace it all eventually".

::: content/media/webrtc/MediaEngine.h
@@ +44,5 @@
> +enum MediaSourceType {
> +  Camera = (int) dom::MediaSourceEnum::Camera,
> +  Screen = (int) dom::MediaSourceEnum::Screen,
> +  Application = (int) dom::MediaSourceEnum::Application,
> +  Window, // = (int) dom::MediaSourceEnum::Window,

Too subtle. Need at least a big XXX and a bug nr to remove the commented part, which I assume is the goal.

@@ +45,5 @@
> +  Camera = (int) dom::MediaSourceEnum::Camera,
> +  Screen = (int) dom::MediaSourceEnum::Screen,
> +  Application = (int) dom::MediaSourceEnum::Application,
> +  Window, // = (int) dom::MediaSourceEnum::Window,
> +  //Browser = (int) dom::MediaSourceEnum::Browser,

Same remark.

@@ +46,5 @@
> +  Screen = (int) dom::MediaSourceEnum::Screen,
> +  Application = (int) dom::MediaSourceEnum::Application,
> +  Window, // = (int) dom::MediaSourceEnum::Window,
> +  //Browser = (int) dom::MediaSourceEnum::Browser,
> +  Microphone

So dom::MediaSourceEnum is really dom::VideoSourceEnum?

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 6 • 10 years ago

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)

> This reeks of "we're going to have to replace it all eventually".

We'll see where the WG goes; I think having an internal type separate from theirs is good in any case.

> 
> ::: content/media/webrtc/MediaEngine.h
> @@ +44,5 @@
> > +enum MediaSourceType {
> > +  Camera = (int) dom::MediaSourceEnum::Camera,
> > +  Screen = (int) dom::MediaSourceEnum::Screen,
> > +  Application = (int) dom::MediaSourceEnum::Application,
> > +  Window, // = (int) dom::MediaSourceEnum::Window,
> 
> Too subtle. Need at least a big XXX and a bug nr to remove the commented
> part, which I assume is the goal.

Bug 1038926 (window-sharing bug)

> 
> @@ +45,5 @@
> > +  Camera = (int) dom::MediaSourceEnum::Camera,
> > +  Screen = (int) dom::MediaSourceEnum::Screen,
> > +  Application = (int) dom::MediaSourceEnum::Application,
> > +  Window, // = (int) dom::MediaSourceEnum::Window,
> > +  //Browser = (int) dom::MediaSourceEnum::Browser,
> 
> Same remark.
> 
> @@ +46,5 @@
> > +  Screen = (int) dom::MediaSourceEnum::Screen,
> > +  Application = (int) dom::MediaSourceEnum::Application,
> > +  Window, // = (int) dom::MediaSourceEnum::Window,
> > +  //Browser = (int) dom::MediaSourceEnum::Browser,

Browser is in the proposal today, but we don't support it and it's unclear if it's going to stay in the proposal or handle that a different way.  (I suspect Browser will go away)

> > +  Microphone
> 
> So dom::MediaSourceEnum is really dom::VideoSourceEnum?

In the Proposal: yes.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 7 • 10 years ago

merged patch for checkin

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 8 • 10 years ago

Comment on attachment 8458013 [details] [diff] [review]
Add screen and window sharing booleans to MediaCaptureWindowState

carry forward r+=pkerr,gcp

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 9 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d00f111241a8

In 34, we'll replace this with a list of what's being shared

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/d00f111241a8

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 11 • 10 years ago

(In reply to Randell Jesup [:jesup] PTO until Jan 5 from comment #6)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> 
> > This reeks of "we're going to have to replace it all eventually".
> 
> We'll see where the WG goes; I think having an internal type separate from
> theirs is good in any case.

I ran across MediaSourceType in the code and traced it back to this bug which I missed when it went by, sorry.

Unless I hear objections, I'd like to merge this back to using the pure dom enums in Bug 1006707. That patch will make MediaSource a regular DOMString instead of a webidl enum - to match the spec - so adding "Microphone" will no longer have any behavioral impact.