[webkit-reviews] review denied: [Bug 123145] [WK2][GTK] enable-media-stream Setting : [Attachment 214828] [wk2] mediastream setting

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 123145: [WK2][GTK] enable-media-stream Setting
https://bugs.webkit.org/show_bug.cgi?id=123145

Attachment 214828: [wk2] mediastream setting
https://bugs.webkit.org/attachment.cgi?id=214828&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=214828&action=review


Thanks for the patch. API looks good to me, except for the mediastream ->
media_stream change. WebKit2 cross-platform changes require a wk2 owner
approval. Units tests are missing too, please add a test for get/set to the
TestWebKitSettings test.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:139
> +    PROP_ENABLE_MEDIASTREAM

This should be MEDIA_STREAM

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1146
> +	*

There's an extra line here.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1152
> +	*/

Since: 2.4

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2832
> + * Returns: %TRUE If mediastream support is enabled or %FALSE otherwise.
> + * Since: 2.2

Leave and extra line between this. 2.2 was already released, this should be 2.4


> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2834
> +gboolean webkit_settings_get_enable_mediastream(WebKitSettings* settings)

it should be media_stream. The property is media-stream and the C API is
MediaStream

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2847
> + * Set the #WebKitSettings:enable-media-stream property.
> + * Since: 2.2

Same here.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2849
> +void webkit_settings_set_enable_mediastream(WebKitSettings* settings,
gboolean enabled)

media_stream