Bug 146461 – Implement platform specific part of enumerateDevices

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Matthew Daiter]

Need to get platform specific code in for enumerateDevices. Should be the first code committed: everything depends on it and nothing is necessary to implement.

[Radar WebKit Bug Importer]

<rdar://problem/21614466>

[Matthew Daiter]

Created attachment 255887 [details]
Patch

[Eric Carlson]

Comment on attachment 255887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255887&action=review

> Source/WebCore/Modules/mediastream/MediaDevicesPrivate.cpp:49
> +    Vector<RefPtr<TrackSourceInfo>> capturedDevices = AVCaptureDeviceManager::singleton().getSourcesInfo("");

Nit: I think you mean "captureDevices", not "capturedDevices".

> Source/WebCore/Modules/mediastream/MediaDevicesPrivate.cpp:57
> +            RefPtr<MediaDeviceInfo> mediaDeviceInfo = MediaDeviceInfo::create(context, capturedDevices[i].get()->label(), capturedDevices[i].get()->deviceId(), capturedDevices[i].get()->groupId(), "audioinput");
> +            mediaDevicesInfo.append(mediaDeviceInfo);
> +        } else {
> +            RefPtr<MediaDeviceInfo> mediaDeviceInfo = MediaDeviceInfo::create(context, capturedDevices[i].get()->label(), capturedDevices[i].get()->deviceId(), capturedDevices[i].get()->groupId(), "videoinput");
> +            mediaDevicesInfo.append(mediaDeviceInfo);

1) you should put capturedDevices[i].get() into a local instead of calling it four times.

2) the MediaDeviceKind string values should be constants exported by the MediaDeviceInfo class

3) this would be easier to understand if you rewrote it to get rid of the "else"

   TrackSourceInfo* trackInfo = capturedDevices[i].get();
   String deviceType = trackInfo->kind() == TrackSourceInfo::SourceKind::Audio ? MediaDeviceInfo::audioInputType() : MediaDeviceInfo::videoInputType();
   mediaDevicesInfo.append(MediaDeviceInfo::create(context, trackInfo->label(), trackInfo->deviceId(), trackInfo->groupId(), deviceType));

> Source/WebCore/Modules/mediastream/MediaDevicesPrivate.h:39
> +

Nit: don't need this extra blank line here.

> Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:342
> +            sourcesInfo.append(TrackSourceInfo::create(devices[i].m_videoSourceId, TrackSourceInfo::Video, device.localizedName, device.localizedName, devices[i].m_captureDeviceID));
>          if (devices[i].m_audioSource)
> -            sourcesInfo.append(TrackSourceInfo::create(devices[i].m_audioSourceId, TrackSourceInfo::Audio, device.localizedName));
> +            sourcesInfo.append(TrackSourceInfo::create(devices[i].m_audioSourceId, TrackSourceInfo::Audio, device.localizedName, device.localizedName, devices[i].m_captureDeviceID));

It is probably not always correct to use the localized name for the groupID. I don't know what we should use for group ID, but please add a FIXME here and we can talk about this.

[Matthew Daiter]

Created attachment 255932 [details]
Patch

[Matthew Daiter]

Created attachment 255934 [details]
Patch

[Matthew Daiter]

Created attachment 255935 [details]
Patch

[Eric Carlson]

Comment on attachment 255935 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255935&action=review

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.h:52
> +    static const String audioInputType() { return "audioinput"; }
> +    static const String audioOutputType() { return "audiooutput"; }
> +    static const String videoInputType() { return "videoinput"; }

These create a new String each time they are called. It would be much more efficient to create and return a static AtomicString reference, see TextTrack::subtitlesKeyword for example.

[Matthew Daiter]

Created attachment 255937 [details]
Patch

[Brent Fulgham]

Comment on attachment 255937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255937&action=review

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.cpp:52
> +    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, audioinput, ("audioinput", AtomicString::ConstructFromLiteral));

We aren't supposed to use DEPRECATED_DEFINE_STATIC_LOCAL anymore. I think you want the NeverDestroy<> template. Talk to andersca if you want more details.

> Source/WebCore/Modules/mediastream/MediaDevicesPrivate.cpp:51
> +    for (size_t i = 0; i < capturedDevices.size(); i++) {

This should be a modern C++ loop:

for (auto& trackInfo : capturedDevices) ....

[Brent Fulgham]

Comment on attachment 255937 [details]
Patch

Looks great. Get rid of the deprecated DEPRECATED_DEFINE_STATIC_LOCAL and switch to a modern C++ iterator, please.

[Matthew Daiter]

Created attachment 255939 [details]
Patch

[Matthew Daiter]

Created attachment 255940 [details]
Patch

[Matthew Daiter]

Created attachment 255941 [details]
Patch

[Matthew Daiter]

Created attachment 255973 [details]
Patch

[Matthew Daiter]

Created attachment 256020 [details]
Patch

[Darin Adler]

Comment on attachment 256020 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256020&action=review

> Source/WebCore/Modules/mediastream/MediaDeviceInfo.cpp:34
>  #include "ScriptWrappable.h"
>  
> +#include <wtf/NeverDestroyed.h>

No blank line here in WebKit coding style.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.cpp:44
> +    return adoptRef(*new MediaDevicesPrivate());

I normally leave off the parentheses when there are no arguments like this.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:35
> +#include "MediaDeviceInfo.h"
> +
> +#include <wtf/RefCounted.h>

No blank spaces needed for this.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:36
> +#include <wtf/RefPtr.h>

Should probably include <wtf/Forward.h> instead since we don’t actually need to work with a RefPtr.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:41
> +class MediaDevicesPrivate : public RefCounted<MediaDevicesPrivate> {

Why does this need to be reference counted? Is there more than one owner for this? Since we need polymorphism but not sharing, then please use std::unique_ptr for this. Lets not reference count things unless there is actually some benefit.

I’m concerned that there are other classes already in the media stream implementation that are also unnecessarily reference counted.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:45
> +    static RefPtr<MediaDevicesPrivate> create();

Return type should be Ref, not RefPtr.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:49
> +    virtual Vector<RefPtr<MediaDeviceInfo>> availableMediaDevices(ScriptExecutionContext*);

Argument type should be Document& or, if this is being made to work with worker threads, then perhaps ScriptExecutionContext&.

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:56
> +#endif /* MediaDevicesPrivate_h */

We don’t use /* */ for this in WebKit coding style.

> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:46
> +    static PassRefPtr<TrackSourceInfo> create(const AtomicString& id, SourceKind kind, const AtomicString& label, const AtomicString& groupId, const AtomicString& deviceId)

Return type should be Ref, not PassRefPtr. Please see <http://www.webkit.org/coding/RefPtr.html>.

> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:72
> +    TrackSourceInfo(const AtomicString& id, SourceKind kind, const AtomicString& label, const AtomicString& groupId, const AtomicString& deviceId)
> +        : m_groupId(groupId)
> +        , m_deviceId(deviceId)
> +        , m_id(id)
> +        , m_kind(kind)
> +        , m_label(label)
> +    {
> +    }

Strange that the order in the constructor doesn’t match the order of the data members. Really we want a single order.

[Matthew Daiter]

Comment on attachment 256020 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256020&action=review

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:35
>> +#include <wtf/RefCounted.h>
> 
> No blank spaces needed for this.

Fixed

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:36
>> +#include <wtf/RefPtr.h>
> 
> Should probably include <wtf/Forward.h> instead since we don’t actually need to work with a RefPtr.

Fixed

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:41
>> +class MediaDevicesPrivate : public RefCounted<MediaDevicesPrivate> {
> 
> Why does this need to be reference counted? Is there more than one owner for this? Since we need polymorphism but not sharing, then please use std::unique_ptr for this. Lets not reference count things unless there is actually some benefit.
> 
> I’m concerned that there are other classes already in the media stream implementation that are also unnecessarily reference counted.

Just thought it'd conform to how the rest of the codebase was laid out. Fixed.

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:45
>> +    static RefPtr<MediaDevicesPrivate> create();
> 
> Return type should be Ref, not RefPtr.

Done

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:49
>> +    virtual Vector<RefPtr<MediaDeviceInfo>> availableMediaDevices(ScriptExecutionContext*);
> 
> Argument type should be Document& or, if this is being made to work with worker threads, then perhaps ScriptExecutionContext&.

Changed to ScriptExecutionContext&

>> Source/WebCore/platform/mediastream/MediaDevicesPrivate.h:56
>> +#endif /* MediaDevicesPrivate_h */
> 
> We don’t use /* */ for this in WebKit coding style.

Fixed

>> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:46
>> +    static PassRefPtr<TrackSourceInfo> create(const AtomicString& id, SourceKind kind, const AtomicString& label, const AtomicString& groupId, const AtomicString& deviceId)
> 
> Return type should be Ref, not PassRefPtr. Please see <http://www.webkit.org/coding/RefPtr.html>.

Fixed

>> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:72
>> +    }
> 
> Strange that the order in the constructor doesn’t match the order of the data members. Really we want a single order.

Fixed

[Matthew Daiter]

Created attachment 256497 [details]
Patch

[Matthew Daiter]

Created attachment 256499 [details]
Patch

[Brent Fulgham]

Comment on attachment 256499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256499&action=review

r=me

> Source/WebCore/platform/mediastream/MediaDevicesPrivate.cpp:47
> +        TrackSourceInfo* trackInfo = device.get();

Do you really need this temporary? I would have expected "trackInfo->kind()" and "device->kind()" to be the same thing.

[WebKit Commit Bot]

Comment on attachment 256499 [details]
Patch

Clearing flags on attachment: 256499

Committed r186608: <http://trac.webkit.org/changeset/186608>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.