[webkit-reviews] review denied: [Bug 123158] [WK2] UserMediaClient support : [Attachment 237495] Patch

Tue Sep 16 15:52:24 PDT 2014

Benjamin Poulain <benjamin at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 123158: [WK2] UserMediaClient support
https://bugs.webkit.org/show_bug.cgi?id=123158

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=237495&action=review

> LayoutTests/ChangeLog:16
> +	   * fast/mediastream/error-expected.txt: Added.
> +	   * fast/mediastream/error.html: Added.
> +	   * fast/mediastream/script-tests/error.js: Added.
> +	   * fast/mediastream/script-tests/success.js: Added.
> +	   * fast/mediastream/success-expected.txt: Added.
> +	   * fast/mediastream/success.html: Added.

It looks to me like you need one a few more tests:

First, two for "pending permission". For example:
-query for UserMedia.
-wait a small amount of time (say 25ms).
-check that it is not granted nor denied.
-grant/deny the permission.

The other kind you need is permission reset. You grant/deny the permission,
then reset the permission. For example:
-grand permission for user media
-query userMedia
-reset the permission
--> I am not familiar with mediaStream, but I guess there is a way to handle
this and it should be tested.

For Geolocation, only iOS has/tests reset.

It would also be useful to have tests with iframes. E.g.:
-request from an iFrame->pending->grand->request from a second iFrame->no
pending, immediately granted if same security origin.
-request from an iFrame->pending->kill the iframe->grant or deny permission->no
crash.
-request from different security origins if possible.

> LayoutTests/fast/mediastream/error.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Let's use the HTML5 doctype.

> LayoutTests/fast/mediastream/error.html:26
> +window.jsTestIsAsync = true;

Move this after description(), easier to follow IMHO.

> LayoutTests/fast/mediastream/success.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Same for doctype.

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:74
> +    bool audio() const { return m_audioConstraints; }
> +    bool video() const { return m_videoConstraints; }

Weird names, maybe requiresAudio()/wantsAudio() or something like that?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:34
> +    parametersMap.set("audio", API::Boolean::create(audio));
> +    parametersMap.set("video", API::Boolean::create(video));

ASCIILiteral() for the strings.