[webkit-reviews] review granted: [Bug 120883] MediaStream API: Changing the device enumeration to be async : [Attachment 211900] YAP

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 120883: MediaStream API: Changing the device enumeration to be async
https://bugs.webkit.org/show_bug.cgi?id=120883

Attachment 211900: YAP
https://bugs.webkit.org/attachment.cgi?id=211900&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=211900&action=review


OK to land this as is, but there are a lot of minor programming mistakes in
this patch that you should fix.

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:109
> +    static NeverDestroyed<AtomicString> ended("ended",
AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> live("live",
AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> muted("muted",
AtomicString::ConstructFromLiteral);

I think that having these globals is probably excessive optimization. Is the
speed of creating a new atomic string really prohibitive?

> Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:34
> +#include <wtf/PassRefPtr.h>

Should not need to include this just to use PassRefPtr as an argument or return
type. No header at all, or <wtf/Forward.h> should suffice.

> Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:35
> +#include <wtf/RefCounted.h>

Not sure why you need to include this here. It seems you’d get it indirectly
from some header.

> Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:44
> +class MediaStreamTrackSourcesRequest : public
MediaStreamTrackSourcesRequestClient {

Mark this class FINAL?

> Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:49
> +    String origin() const { return m_origin; }

Might consider const String& for this instead.

> Source/WebCore/Modules/mediastream/MediaStreamTrackSourcesRequest.h:52
> +    // MediaStreamTrackSourcesRequestClient
> +    void didCompleteRequest(const TrackSourceInfoVector&) OVERRIDE;

Can this be private instead of public?

> Source/WebCore/Modules/mediastream/SourceInfo.cpp:50
> +    static NeverDestroyed<AtomicString> none("none",
AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> audioKind("audio",
AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> videoKind("video",
AtomicString::ConstructFromLiteral);

Is this optimization needed?

> Source/WebCore/Modules/mediastream/SourceInfo.cpp:70
> +    static NeverDestroyed<AtomicString> userFacing("user",
AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> environmentFacing("environment",
AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> leftFacing("left",
AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> rightFacing("right",
AtomicString::ConstructFromLiteral);

Is this optimization needed?

> Source/WebCore/Modules/mediastream/SourceInfo.h:34
> +#include <wtf/OwnPtr.h>

Should not need to include this.

> Source/WebCore/Modules/mediastream/SourceInfo.h:35
> +#include <wtf/PassRefPtr.h>

Should not need to include this, maybe Forward.h instead.

> Source/WebCore/Modules/mediastream/SourceInfo.h:45
> +    virtual ~SourceInfo() { }

This class isn’t polymorphic at all, so this just makes the code bigger for no
reason. I suggest omitting it entirely.

> Source/WebCore/Modules/mediastream/SourceInfo.h:50
> +    AtomicString id() const { return m_trackSourceInfo->id(); }
> +    AtomicString label() const { return m_trackSourceInfo->label(); }
> +    AtomicString kind() const;
> +    AtomicString facing() const;

Given the implementations of these, I think we can use const AtomicString&
instead, perhaps.

> Source/WebCore/Modules/mediastream/SourceInfo.h:53
> +    SourceInfo(PassRefPtr<TrackSourceInfo>);

Should mark this explicit.

> Source/WebCore/Modules/mediastream/SourceInfo.h:58
> +typedef Vector<RefPtr<SourceInfo> > SourceInfoVector;

Should not leave the space between "> >" -- that’s for older compilers that we
no longer support.

Also, I don’t think this typedef is a great idea. Generally Anders has been
pointing out that such typedefs hide important details, like whether the
elements are RefPtr or not, without making the code significantly more elegant.


> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:33

> +#include <wtf/text/WTFString.h>

I don’t understand how this file can use AtomicString without including the
relevant header.

> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:43

> +    enum SourceKind {
> +	   NoSource,
> +	   Audio,
> +	   Video
> +    };

I think that simple enums like this read better on a single line rather than
splayed out over multiple lines like this.

> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:51

> +    enum VideoFacingMode {
> +	   None,
> +	   User,
> +	   Environment,
> +	   Left,
> +	   Right
> +    };

Ditto.

> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:57

> +    virtual ~TrackSourceInfo() { }

This should be omitted. There is no polymorphic use of this class, and this
will just make the code bigger for no reason.

> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:60

> +    AtomicString id() const { return m_id; }
> +    AtomicString label() const { return m_label; }

Return type should be const AtomicString&.

> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:79

> +typedef Vector<RefPtr<TrackSourceInfo>> TrackSourceInfoVector;

Same comment as elsewhere about these vector typedefs.

> Source/WebCore/platform/mediastream/MediaStreamTrackSourcesRequestClient.h:85

> +public:
> +
> +    virtual ~MediaStreamTrackSourcesRequestClient() { }

Should remove this blank line.