Bug 123145 – [WK2][GTK] enable-media-stream Setting

• Component: WebKit2
• Status: RESOLVED FIXED

[Philippe Normand]

I think it'd be good to have a enable-media-stream setting, similar to the ones for webgl and webaudio. I prepared a patch, including the necessary bits for the GTK port.

[Philippe Normand]

Created attachment 214827 [details]
[wk2] mediastream setting

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Commit Bot]

Attachment 214827 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/Settings.in', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/WebPreferencesStore.h', u'Source/WebKit2/UIProcess/API/C/WKPreferences.cpp', u'Source/WebKit2/UIProcess/API/C/WKPreferences.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1154:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1156:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1158:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1159:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 214828 [details]
[wk2] mediastream setting

[Carlos Garcia Campos]

Comment on attachment 214828 [details]
[wk2] mediastream setting

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

[Philippe Normand]

(In reply to comment #5)
> (From update of attachment 214828 [details])
> 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.
> 

Cool, thanks for the review! I added a test, did you see it?
Anyway, I'll update the patch.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 214828 [details] [details])
> > 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.
> > 
> 
> Cool, thanks for the review! I added a test, did you see it?

Oops, sorry, I missed it, I reviewed it too fast.

> Anyway, I'll update the patch.

Thanks

[Philippe Normand]

Created attachment 214934 [details]
[wk2] mediastream setting

[Philippe Normand]

Comment on attachment 214934 [details]
[wk2] mediastream setting

This is not all working yet.

[Carlos Garcia Campos]

(In reply to comment #9)
> (From update of attachment 214934 [details])
> This is not all working yet.

What doesn't work?

[Philippe Normand]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 214934 [details] [details])
> > This is not all working yet.
> 
> What doesn't work?

Right now the thing is a RuntimeEnabledFeature, not a Setting. I need to find where to change that logic in all involved code paths...

[Philippe Normand]

Created attachment 215178 [details]
[wk2] mediastream setting

[Philippe Normand]

Hi Anders, can you please review this patch?

[Philippe Normand]

Created attachment 215528 [details]
[wk2] mediastream setting

[Philippe Normand]

Committed r158360: <http://trac.webkit.org/changeset/158360>