[webkit-reviews] review denied: [Bug 94373] [gtk] adding methods and signals for usermedia communication between webkit and browser : [Attachment 166722] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Danilo Cesar Lemes de
Paula <danilo.cesar at collabora.co.uk>'s request for review:
Bug 94373: [gtk] adding methods and signals for usermedia communication between
webkit and browser
https://bugs.webkit.org/show_bug.cgi?id=94373

Attachment 166722: Patch
https://bugs.webkit.org/attachment.cgi?id=166722&action=review

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


Still someone who knows about the media stream api should review this, and it's
new api so we need two reviewers. I've just commented about general things
only.

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:29
> -#include "UserMediaClientGtk.h"
>  
>  #if ENABLE(MEDIA_STREAM)
> -#include "MediaStreamSource.h"
> -#include "NotImplemented.h"
> -#include "UserMediaRequest.h"
> +
> +#include "UserMediaClientGtk.h"
> +
> +#include "webkitwebusermedialistprivate.h"
> +#include "webkitwebusermediarequestprivate.h"

That was correct, it should be config and then UserMediaClientGtk.h, followed
by an empty line and then all other headers. UserMediaClientGtk.h already has
#if ENABLE(MEDIA_STREAM) so it's not a problem if it's used before the #if
here.

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:35
> +UserMediaClientGtk::UserMediaClientGtk(WebKitWebView *webView)

WebKitWebView *webView -> WebKitWebView* webView

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:49
> -void UserMediaClientGtk::requestUserMedia(WTF::PassRefPtr<UserMediaRequest>
prpRequest, const MediaStreamSourceVector& audioSource, const
MediaStreamSourceVector& videoSource)
> +void UserMediaClientGtk::requestUserMedia(WTF::PassRefPtr<UserMediaRequest>
prpRequest, const WebCore::MediaStreamSourceVector& audioSources, const
WebCore::MediaStreamSourceVector& videoSources)
>  {

This is not needed, there's using namespace WebCore; in this file

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:51
> +    RefPtr<UserMediaRequest> request = prpRequest;
> +    g_signal_emit_by_name(m_webView, "user-media-requested",
kitNew(request.get()), kitNew(audioSources), kitNew(videoSources));

Who will be responsible for releasing the ref you are taking here?

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.cpp:59
> -} // namespace WebKit;
> +}

Why do you remove the comment?

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.h:41
> -    UserMediaClientGtk();
> -    virtual ~UserMediaClientGtk();
> +    UserMediaClientGtk(WebKitWebView*);
> +    ~UserMediaClientGtk();

Was the virtual wrong for the destructor?

> Source/WebKit/gtk/WebCoreSupport/UserMediaClientGtk.h:52
> -} // namespace WebKit
> +}

why removing this?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:28
> +#include "config.h"
> +
> +#if ENABLE(MEDIA_STREAM)
> +
> +#include "webkitwebusermedialist.h"
> +
> +#include "webkitglobalsprivate.h"
> +#include "webkitwebusermedialistprivate.h"
> +
> +#include <wtf/text/CString.h>

This should be

#include "config.h"
#include "webkitwebusermedialist.h"

#if ENABLE(MEDIA_STREAM)
#include "webkitglobalsprivate.h"
#include "webkitwebusermedialistprivate.h"
#include <wtf/text/CString.h>

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:39
> + * Since: 1.10.0

1.10.0 was released already, this should be 2.0

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:59
> +static void webkitWebUserMediaListDispose(GObject* object)
> +{
> +    WebKitWebUserMediaList* userMediaList =
WEBKIT_WEB_USER_MEDIA_LIST(object);
> +    WebKitWebUserMediaListPrivate* priv = userMediaList->priv;
> +
> +    priv->sourceSelections.clear();
> +
> +   
G_OBJECT_CLASS(webkit_web_user_media_list_parent_class)->dispose(object);
> +}

I don't think you need this, the vector will be released by the struct
destructor in Finalize()

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:69
> +    GObjectClass* gobject_class = G_OBJECT_CLASS(listClass);

gobject_class -> gobjectClass

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:95
> + * Since: 1.10.0

2.0

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:114
> + * Since: 1.10.0

Ditto.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:135
> + * Since: 1.10.0

Ditto.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:141
> +    g_return_val_if_fail(WEBKIT_IS_WEB_USER_MEDIA_LIST(list), FALSE);
> +
> +    g_return_val_if_fail(index < core(list).size(), FALSE);

Extra new line there

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:153
> + * Since: 1.10.0

2.0

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:159
> +    g_return_if_fail(WEBKIT_IS_WEB_USER_MEDIA_LIST(list));
> +
> +    g_return_if_fail(index < core(list).size());

Extra new line there

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:24
> +#include "config.h"
> +
> +#if ENABLE(MEDIA_STREAM)
> +
> +#include "webkitwebusermediarequest.h"

ame issue here with the headers

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:42
> + * Since: 1.10.0

2.0

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:84
> + * Return value: True if the request contains an audio request.

True -> %TRUE

True if the request contains an audio request or %FALSE otherwise.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:101
> + * Return value: True if the request contains a video request.

Ditto

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:133
> +    g_return_val_if_fail(WEBKIT_IS_WEB_USER_MEDIA_REQUEST(request), 0);

This is not a public API mehod, use ASSERT or nothing instead of g_return
macros.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:140
> +    g_return_val_if_fail(request, 0);

Ditto.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.h:59
> +WEBKIT_API WebKitSecurityOrigin*

WebKitSecurityOrigin* -> WebKitSecurityOrigin *

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2742
> +	* Then the user is expected to call either
#webkit_web_view_accept_user_media_request() or

Don't need to use # for function names

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2745
> +	* Since: 1.10.0

2.0

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5410
> +    core(webRequest)->succeed(audioSources, videoSources);

I wonder why this is a method of WebView if it doesn't use the web view. Looks
too me like a method of WebKitWebUserMediaRequest, something like
webkit_web_user_media_request_accept();

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5427
> +void webkit_web_view_reject_user_media_request(WebKitWebView *webView,
WebKitWebUserMediaRequest *webRequest)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
> +    g_return_if_fail(WEBKIT_IS_WEB_USER_MEDIA_REQUEST(webRequest));
> +
> +    core(webRequest)->fail();

Same here, webView is unused