Bug 123145

Created attachment 214827 [details] [wk2] mediastream setting

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

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.

Created attachment 214828 [details] [wk2] mediastream setting

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

(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.

(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

Created attachment 214934 [details] [wk2] mediastream setting

Comment on attachment 214934 [details] [wk2] mediastream setting This is not all working yet.

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

(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...

Created attachment 215178 [details] [wk2] mediastream setting

Hi Anders, can you please review this patch?

Created attachment 215528 [details] [wk2] mediastream setting

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