MediaStream API: Changing the device enumeration to be async

MediaStream API: Changing the device enumeration to be async
When we stared to implement this in chrome we discovered that sometimes
device enumeration could take some time and therefore the API needed
to be adjusted. This has been discussed on the W3C email list without
objections but not yet reached the editors draft.
The old name getSourceInfos got changed to getSources as well.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152771

[abarth-chromium, 2013-06-11 23:50:44]

https://codereview.chromium.org/16778002/diff/4001/Source/core/platform/media...
File Source/core/platform/mediastream/MediaStreamTrackSourcesRequest.h (right):

https://codereview.chromium.org/16778002/diff/4001/Source/core/platform/media...
Source/core/platform/mediastream/MediaStreamTrackSourcesRequest.h:50: virtual
String url() = 0;
Why do we need this class?  We can probably skip it now-a-days.

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
File Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
Source/modules/mediastream/MediaStreamTrack.cpp:124:
RefPtr<MediaStreamTrackSourcesRequest> request =
MediaStreamTrackSourcesRequestImpl::create(context, callback);
MediaStreamTrackSourcesRequestImpl <-- We can probably merge
MediaStreamTrackSourcesRequest and MediaStreamTrackSourcesRequestImpl into one
class.

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
File Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp (right):

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp:50: m_url =
context->securityOrigin()->toString();
The toString of the securityOrigin isn't a URL....

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp:77:
m_callback.clear();
You shouldn't need to build this notification pathway yourself.  The callback
object should do this work for you automatically.

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
File Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.h (right):

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.h:36: class
MediaStreamTrackSourcesRequestImpl : public MediaStreamTrackSourcesRequest,
public ActiveDOMObject {
Why does this need to be an active DOM object?  The
MediaStreamTrackSourcesCallback object should already be notified when the
context shuts down...

[Tommy Widenflycht, 2013-06-12 12:29:34]

https://codereview.chromium.org/16778002/diff/4001/Source/core/platform/media...
File Source/core/platform/mediastream/MediaStreamTrackSourcesRequest.h (right):

https://codereview.chromium.org/16778002/diff/4001/Source/core/platform/media...
Source/core/platform/mediastream/MediaStreamTrackSourcesRequest.h:50: virtual
String url() = 0;
On 2013/06/11 23:50:44, abarth wrote:
> Why do we need this class?  We can probably skip it now-a-days.

So is the concept of core/platform deprecated then? Certainly makes for simpler
code. Merge done.

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
File Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
Source/modules/mediastream/MediaStreamTrack.cpp:124:
RefPtr<MediaStreamTrackSourcesRequest> request =
MediaStreamTrackSourcesRequestImpl::create(context, callback);
On 2013/06/11 23:50:44, abarth wrote:
> MediaStreamTrackSourcesRequestImpl <-- We can probably merge
> MediaStreamTrackSourcesRequest and MediaStreamTrackSourcesRequestImpl into one
> class.

Done.

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
File Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp (right):

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp:50: m_url =
context->securityOrigin()->toString();
On 2013/06/11 23:50:44, abarth wrote:
> The toString of the securityOrigin isn't a URL....

The name isn't great I agree, but I don't really know what to call it.
We need this so that we can salt the device ids to get rid of any fingerprinting
possibilities.

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.cpp:77:
m_callback.clear();
On 2013/06/11 23:50:44, abarth wrote:
> You shouldn't need to build this notification pathway yourself.  The callback
> object should do this work for you automatically.

Done.

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
File Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.h (right):

https://codereview.chromium.org/16778002/diff/4001/Source/modules/mediastream...
Source/modules/mediastream/MediaStreamTrackSourcesRequestImpl.h:36: class
MediaStreamTrackSourcesRequestImpl : public MediaStreamTrackSourcesRequest,
public ActiveDOMObject {
On 2013/06/11 23:50:44, abarth wrote:
> Why does this need to be an active DOM object?  The
> MediaStreamTrackSourcesCallback object should already be notified when the
> context shuts down...

Double safety belts... Fixed.

[jamesr, 2013-06-12 17:39:40]

https://codereview.chromium.org/16778002/diff/12001/Source/core/platform/chro...
File Source/core/platform/chromium/support/WebMediaStreamTrackSourcesRequest.cpp
(right):

https://codereview.chromium.org/16778002/diff/12001/Source/core/platform/chro...
Source/core/platform/chromium/support/WebMediaStreamTrackSourcesRequest.cpp:30:
#include "modules/mediastream/MediaStreamTrackSourcesRequest.h"
hmm, it's not good to #include modules/ code from core/. can this be avoided?

https://codereview.chromium.org/16778002/diff/12001/Source/core/platform/medi...
File Source/core/platform/mediastream/chromium/MediaStreamCenterChromium.cpp
(right):

https://codereview.chromium.org/16778002/diff/12001/Source/core/platform/medi...
Source/core/platform/mediastream/chromium/MediaStreamCenterChromium.cpp:37:
#include "modules/mediastream/MediaStreamTrackSourcesRequest.h"
this is also unfortunate

[abarth-chromium, 2013-06-12 20:43:35]

We should just move all the code in core/platform/mediastream to
modules/mediastream.  Then you won't need to bother with core at all.

[Tommy Widenflycht, 2013-06-13 11:32:39]

On 2013/06/12 20:43:35, abarth wrote:
> We should just move all the code in core/platform/mediastream to
> modules/mediastream.  Then you won't need to bother with core at all.

Whereas I really like that approach it currently has a major problem which is
that DRT/ContentShell only links against core/platform and not modules. This
means that I can't move
Source/core/platform/chromium/support/WebMediaStreamTrackSourcesRequest.cpp to
Source/modules/mediastream/.

jamesr: If you look at Patch Set 2 you can see how we used to solve the issue by
having an abstract interface in platform with an impl class in modules.

[jamesr, 2013-06-13 19:59:43]

On 2013/06/13 11:32:39, Tommy Widenflycht wrote:
> On 2013/06/12 20:43:35, abarth wrote:
> > We should just move all the code in core/platform/mediastream to
> > modules/mediastream.  Then you won't need to bother with core at all.
> 
> Whereas I really like that approach it currently has a major problem which is
> that DRT/ContentShell only links against core/platform and not modules.

That's not correct.  It's not actually possible to link against core/platform
and not link against modules today, so I'm not sure where this idea comes from. 
The layout tests test the web platform including these modules.

[Tommy Widenflycht, 2013-06-14 07:30:40]

On 2013/06/13 19:59:43, jamesr wrote:
> On 2013/06/13 11:32:39, Tommy Widenflycht wrote:
> > On 2013/06/12 20:43:35, abarth wrote:
> > > We should just move all the code in core/platform/mediastream to
> > > modules/mediastream.  Then you won't need to bother with core at all.
> > 
> > Whereas I really like that approach it currently has a major problem which
is
> > that DRT/ContentShell only links against core/platform and not modules.
> 
> That's not correct.  It's not actually possible to link against core/platform
> and not link against modules today, so I'm not sure where this idea comes
from. 
> The layout tests test the web platform including these modules.

The "idea" comes from the compiler which can't find the definitions for the Web*
classes if I move them to modules/. But I should have been more specific: chrome
links just fine but DRT/ContentShell 
doesn't. I really don't understand why either.

[jochen (gone - plz use gerrit), 2013-06-14 07:59:52]

On 2013/06/14 07:30:40, Tommy Widenflycht wrote:
> On 2013/06/13 19:59:43, jamesr wrote:
> > On 2013/06/13 11:32:39, Tommy Widenflycht wrote:
> > > On 2013/06/12 20:43:35, abarth wrote:
> > > > We should just move all the code in core/platform/mediastream to
> > > > modules/mediastream.  Then you won't need to bother with core at all.
> > > 
> > > Whereas I really like that approach it currently has a major problem which
> is
> > > that DRT/ContentShell only links against core/platform and not modules.
> > 
> > That's not correct.  It's not actually possible to link against
core/platform
> > and not link against modules today, so I'm not sure where this idea comes
> from. 
> > The layout tests test the web platform including these modules.
> 
> The "idea" comes from the compiler which can't find the definitions for the
Web*
> classes if I move them to modules/. But I should have been more specific:
chrome
> links just fine but DRT/ContentShell 
> doesn't. I really don't understand why either.

content shell/DRT only see stuff in public/ directories. It doesn't see
core/platform either.

[Tommy Widenflycht, 2013-06-14 12:04:08]

Jochen offered to help debugging my linker issues but when I prepared a patch
for him the issue had gone away... Talk about superpowers ;)

I have moved WebMediaStreamTrackSourcesRequest.cpp to modules but don;t want to
touch the other files still in core/platform yet.

How does this look like?

[abarth-chromium, 2013-06-14 14:17:47]

This LGTM

You might have trouble in the Windows shared-library build.  Would you be
willing to send this to a debug windows try server before landing?

https://codereview.chromium.org/16778002/diff/25001/Source/modules/mediastrea...
File Source/modules/mediastream/MediaStreamTrackSourcesRequest.cpp (right):

https://codereview.chromium.org/16778002/diff/25001/Source/modules/mediastrea...
Source/modules/mediastream/MediaStreamTrackSourcesRequest.cpp:45: m_url =
context->securityOrigin()->toString();
I still don't understand why this field is called |m_url| if it's really the
toString of the origin.  (Sorry if you answered this before.)

[Tommy Widenflycht, 2013-06-17 11:22:53]

On 2013/06/14 14:17:47, abarth wrote:
> This LGTM
> 
> You might have trouble in the Windows shared-library build.  Would you be
> willing to send this to a debug windows try server before landing?
> 
>
https://codereview.chromium.org/16778002/diff/25001/Source/modules/mediastrea...
> File Source/modules/mediastream/MediaStreamTrackSourcesRequest.cpp (right):
> 
>
https://codereview.chromium.org/16778002/diff/25001/Source/modules/mediastrea...
> Source/modules/mediastream/MediaStreamTrackSourcesRequest.cpp:45: m_url =
> context->securityOrigin()->toString();
> I still don't understand why this field is called |m_url| if it's really the
> toString of the origin.  (Sorry if you answered this before.)

I answered with: "The name isn't great I agree, but I don't really know what to
call it. We need this so that we can salt the device ids to get rid of any
fingerprinting possibilities." 

I have renamed it origin; does that sound better?

And yes, this patch still have some component build linker issue so will not
commit it until that is resolved.

[abarth-chromium, 2013-06-17 18:22:56]

On 2013/06/17 11:22:53, Tommy Widenflycht wrote:
> I have renamed it origin; does that sound better?

Much better, thanks!

> And yes, this patch still have some component build linker issue so will not
> commit it until that is resolved.

Ok.  Do you have a path forward for that, or are you looking for help?

[jochen (gone - plz use gerrit), 2013-06-17 20:13:00]

On 2013/06/17 18:22:56, abarth wrote:
> On 2013/06/17 11:22:53, Tommy Widenflycht wrote:
> > I have renamed it origin; does that sound better?
> 
> Much better, thanks!
> 
> > And yes, this patch still have some component build linker issue so will not
> > commit it until that is resolved.
> 
> Ok.  Do you have a path forward for that, or are you looking for help?

https://codereview.chromium.org/16881011/ fixes the link issue. Without it, the
WEBKIT_EXPORT macro doesn't work inside modules/

[Tommy Widenflycht, 2013-06-18 09:22:39]

PTAL
While waiting for the gyp file fix I have modified the Request object to handle
a synchronous/direct callback using a RefPtr<> m_protect = this construct.

[jochen (gone - plz use gerrit), 2013-06-18 21:14:15]

I landed my change, so this CL should be unblocked

[jochen (gone - plz use gerrit), 2013-06-19 13:09:32]

https://codereview.chromium.org/16778002/diff/42001/Source/modules/mediastrea...
File Source/modules/mediastream/MediaStreamTrackSourcesRequest.cpp (right):

https://codereview.chromium.org/16778002/diff/42001/Source/modules/mediastrea...
Source/modules/mediastream/MediaStreamTrackSourcesRequest.cpp:55: if (m_callback
&& !m_scheduledEventTimer.isActive()) {
is it possible that requestSucceeded() is called multiple times? It seems like
the source infos are ignored then?

[Tommy Widenflycht, 2013-06-19 13:11:59]

https://codereview.chromium.org/16778002/diff/42001/Source/modules/mediastrea...
File Source/modules/mediastream/MediaStreamTrackSourcesRequest.cpp (right):

https://codereview.chromium.org/16778002/diff/42001/Source/modules/mediastrea...
Source/modules/mediastream/MediaStreamTrackSourcesRequest.cpp:55: if (m_callback
&& !m_scheduledEventTimer.isActive()) {
On 2013/06/19 13:09:33, jochen wrote:
> is it possible that requestSucceeded() is called multiple times? It seems like
> the source infos are ignored then?
No, it isn't allowed to call this more than once.

[jochen (gone - plz use gerrit), 2013-06-19 15:32:11]

lgtm

[commit-bot: I haz the power, 2013-06-20 07:23:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommyw@chromium.org/16778002/65001

[commit-bot: I haz the power, 2013-06-20 11:16:46]

Change committed as 152771