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.
<rdar://problem/21614466>
Created attachment 255887 [details] Patch
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.
Created attachment 255932 [details] Patch
Created attachment 255934 [details] Patch
Created attachment 255935 [details] Patch
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.
Created attachment 255937 [details] Patch
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) ....
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.
Created attachment 255939 [details] Patch
Created attachment 255940 [details] Patch
Created attachment 255941 [details] Patch
Created attachment 255973 [details] Patch
Created attachment 256020 [details] Patch
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.
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
Created attachment 256497 [details] Patch
Created attachment 256499 [details] Patch
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.
Comment on attachment 256499 [details] Patch Clearing flags on attachment: 256499 Committed r186608: <http://trac.webkit.org/changeset/186608>
All reviewed patches have been landed. Closing bug.