[Webkit-unassigned] [Bug 100261] [gtk] core implementation for getUserMedia API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=100261





--- Comment #15 from Philippe Normand <pnormand at igalia.com>  2013-02-05 06:45:41 PST ---
(From update of attachment 186444)
View in context: https://bugs.webkit.org/attachment.cgi?id=186444&action=review

Looks good overall, the Stream player seems quite incomplete though, I wonder if it'd make sense to have only the refactoring in this patch and keep a minimal-working stream player for a separate patch?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:156
> +            guint m_videoCapsHandler;

This doesn't seem used anywhere?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:283
> +    // FIXME: use implementation from StreamMediaPlayer??

I don't understand this FIXME, is it a left-over from a previous version of the patch?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:368
> +// creates and initializes a bunch of internal internal variables, and returns a

Double "internal" :) Also comments should be full sentences.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:110
> +    GRefPtr<GstStreamVolume> m_volume;

This name could be confusing, what about m_volumeElement? Also, OOC don't we need a GRefPtr specific implementation for GstStreamVolume, which is an interface, I'm not sure how it combines with GRefPtr in this patch.

> Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp:126
> +        // go go go

Hum :)

> Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:89
> +    GstElement* m_videoSinkBin;
> +    GstElement* m_audioSinkBin;

Why not reuse the ones from the base class?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.