[webkit-reviews] review denied: [Bug 94373] [gtk] adding methods and signals for usermedia communication between webkit and browser : [Attachment 159705] 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 159705: Patch
https://bugs.webkit.org/attachment.cgi?id=159705&action=review

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


I have no idea about media stream API, so I'll just comment on general things.
It would help to include unit tests with the patch. Setting r- because the
patch seems incomplete.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:25
> +#include "MediaStreamSource.h"

This is already included by webkitwebusermedialistprivate.h

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:29
> +#include <glib.h>

This is already included in the header.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:30
> +#include <glib/gi18n-lib.h>

There aren't translatable strings in this file, no?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:31
> +#include <stdio.h>

What do you need this for?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:50
> +    gboolean* sourceSelections;

Use Vector<bool>

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:53
> +G_DEFINE_TYPE(WebKitWebUserMediaList, webkit_web_user_media_list,
G_TYPE_OBJECT);

The trailing ; is not needed.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:58
> +static void webkit_web_user_media_list_dispose(GObject* object)
> +{
> +   
G_OBJECT_CLASS(webkit_web_user_media_list_parent_class)->dispose(object);
> +}

Don't define dispose just to chain up.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:60
> +static void webkit_web_user_media_list_finalize(GObject* object)

We use the WebKit coding style for internal private methods,
webkitWebViewFinalize().

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:64
> +    g_free(userMediaList->priv->sourceSelections);

Use the the placement new syntax to allocate the private structure and simply
call the destructor here. See webkitwebview.cpp as an example.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:83
> +    WebKitWebUserMediaListPrivate* priv = G_TYPE_INSTANCE_GET_PRIVATE(list,
WEBKIT_TYPE_WEB_USER_MEDIA_LIST, WebKitWebUserMediaListPrivate);
> +    list->priv = priv;

Use placement new syntax, see webkit_web_view_init()

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:122
> +    return g_strdup(sources[index]->name().utf8().data());

We could cache the name on demand using a CString to return a const gchar *
instead.

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:136
> +gboolean
webkit_web_user_media_list_get_item_selected(WebKitWebUserMediaList* list,
guint index)

is_item_selected?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:141
> +    WebCore::MediaStreamSourceVector& sources = core(list);
> +    g_return_val_if_fail(index < sources.size(), FALSE);

Could this be just g_return_val_if_fail(index < core(list).size(), FALSE); ?

> Source/WebKit/gtk/webkit/webkitwebusermedialist.cpp:160
> +    WebCore::MediaStreamSourceVector& sources = core(list);
> +    g_return_if_fail(index < sources.size());

Ditto.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:31
> +#include <glib.h>

This is already included by the header.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:32
> +#include <glib/gi18n-lib.h>

No translatable strings here either.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:52
> +G_DEFINE_TYPE(WebKitWebUserMediaRequest, webkit_web_user_media_request,
G_TYPE_OBJECT);

Trailing ; is not needed

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:64
> +    WebKitWebUserMediaRequestPrivate* priv =
G_TYPE_INSTANCE_GET_PRIVATE(request, WEBKIT_TYPE_WEB_USER_MEDIA_REQUEST,
WebKitWebUserMediaRequestPrivate);
> +    request->priv = priv;

Use the placement new syntax here too, so that the destructor is called and the
RefPtr is destroyed.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:111
> +const gchar*
webkit_web_user_media_request_get_origin(WebKitWebUserMediaRequest* request)

get_security_origin? This is confusing, because there's a WebKitSecurityOrigin
calls in the API, shouldn't we return that instead?

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

Use 0 instead of NULL.

> Source/WebKit/gtk/webkit/webkitwebusermediarequest.cpp:115
> +    return
core(request)->scriptExecutionContext()->securityOrigin()->toString().utf8().da
ta();

This looks like a temporary value, it should be cached to return a const gchar
*, or duplicated and return a gchar *

> Source/WebKit/gtk/webkit/webkitwebview.cpp:126
> +#include "MediaStreamSource.h"
> +#include "UserMediaClientGtk.h"
> +#include "webkitwebusermedialist.h"
> +#include "webkitwebusermedialistprivate.h"

webkitwebusermedialistprivate.h already includes webkitwebusermedialist.h and
MediaStreamSource.h

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2749
> +#if ENABLE(MEDIA_STREAM)

I don't think having public API that depends on compilation options is a good
idea. The API should always be there, but if media stream support is not
compiled, the API should do nothing or fallback to a default behaviour.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2751
> +    /*

/* -> /**

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2752
> +	* WebKitWebView::user-media-requested

Trailing : missing here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2759
> +	* When the application request userMedia, this signal will be emited
with the request
> +	* information and the available options to be selected.

Then the user is expected to call either
webkit_web_view_accept_user_media_request() or
webkit_web_view_reject_user_media_request()?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2765
> +	       (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),

Why G_SIGNAL_ACTION?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2774
> +    /*

/* -> /**

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2775
> +	* WebKitWebView::user-media-request-cancelled

Trailing : missing here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2779
> +	* This signal will be emited when the user canceled its userMedia
request.

Why do we need to notify the user that request has cancelled by him/her?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2785
> +	       (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),

Why G_SIGNAL_ACTION?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2792
> +#endif /* ENABLE(MEDIA_STREAM) */

Where are the signals emitted?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5398
> +void webkit_web_view_accept_user_media_request(WebKitWebView *webView,
WebKitWebUserMediaRequest *webRequest, WebKitWebUserMediaList *audioMediaList,
WebKitWebUserMediaList *videoMediaList)

All * are incorrectly placed.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5410
> +    for (size_t i = audioSources.size() - 1; i != (size_t) -1; --i)

Why do you need to iterate from size -1 to (size_t) - 1? Can't you just iterate
from i = 0 to audioSources.size() - 1? or from size() - 1 to 0?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5414
> +    for (size_t i = videoSources.size() - 1; i != (size_t) -1; --i)

Ditto.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5420
> +    // TODO: uncomment the following lines when the UserMediaClientGtk
implementation is completed
> +    // UserMediaClientGtk* client =
static_cast<UserMediaClientGtk*>(priv->userMediaClient.get());
> +    // client->userMediaRequestSucceeded(core(webRequest), audioSources,
videoSources);

What happen if the user doesn't handle the signal? would the client always
expect that either userMediaRequestSucceeded or userMediaRequestFailed are
called? In that case we should have a default implementation in cade the signal
is not handled.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5441
> +    // TODO: uncomment thw following lines when the UserMediaClientGtk
implementation is completed
> +    // UserMediaClientGtk* client =
static_cast<UserMediaClientGtk*>(priv->userMediaClient.get());
> +    // client->userMediaRequestFailed(core(webRequest));

If WebCore implementation is not yet ready, I think it's better to wait until
it's done to add the public API instead of adding API that does nothing.