[Webkit-unassigned] [Bug 94373] [gtk] adding methods and signals for usermedia communication between webkit and browser

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 19 08:38:34 PDT 2012 

https://bugs.webkit.org/show_bug.cgi?id=94373


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #159705|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-09-19 08:39:05 PST ---
(From update of attachment 159705)
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().data();

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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list