[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 #10 from Philippe Normand <pnormand at igalia.com>  2013-01-30 23:48:14 PST ---
(From update of attachment 185577)
View in context: https://bugs.webkit.org/attachment.cgi?id=185577&action=review

Ok it looks good but I haven't made a full review because the patch is not based on ToT. Can you rebase it please? There were several changes recently including audio pitch preservation and native fullscreen support (with GStreamerGWorld). Having a rebased patch will also allow the EWS to chew this up :)

> Source/WebCore/ChangeLog:9
> +        [gtk] Refactor the media player implementation so more of the
> +        internal functionality can be shared between the current media
> +        backend and the mediastream player backend.  Common code is
> +        broken out into a MediaPlayerPrivateGStreamerBase, and both
> +        MediaPlayerPrivateGStreamer and
> +        StreamMediaPlayerPrivateGStreamer inherit from this base class.
> +        https://bugs.webkit.org/show_bug.cgi?id=100261

The change description is ok but doesn't comply with the ChangeLog format :) Usually it's:

bug title
bug url

Reviewed by ....

change description

> Source/WebCore/ChangeLog:13
> +        No new tests (OOPS!).

You can remove this line or simply state existing media tests cover this patch.

> Source/WebCore/GNUmakefile.list.am:6065
> +	Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp \
> +	Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h \

The Qt and EFL build systems need similar changes.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:262
> +#if 0 // implementation from StreamMediaPlayer

Perhaps this block can be removed for now

> Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:28
> +typedef struct _WebKitVideoSink WebKitVideoSink;

You can remove this I think

> Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:35
> +class GStreamerGWorld;

Ditto

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