No subject


Mon Jan 28 08:41:14 PST 2013


JS API can request either audio, video, or both. The API can also request
specific hardware like the front facing camera ("facingMode").
It seem to me that the UIClient interface should, at a minimum, expose the
device type. The message presented to the user should be different if the p=
age
request access to the microphone, the camera or both.

> Source/WebKit2/UIProcess/API/C/WKPage.h:290
> +

Extra line here.

> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:160
> +    GRefPtr<WebKitUserMediaPermissionRequest> userMediaPermissionRequest=
 =3D
adoptGRef(webkitUserMediaPermissionRequestCreate(toImpl(request)));
> +    webkitWebViewMakePermissionRequest(WEBKIT_WEB_VIEW(clientInfo),
WEBKIT_PERMISSION_REQUEST(userMediaPermissionRequest.get()));

I am a little concerned you do not use the security origin here.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:59
> +    // Only one decision at a time.
> +    if (priv->madeDecision)
> +	   return;

This is smart, I got into a but like this once. :)

But do you initialize "madeDecision" to false anywhere?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:62
> +    priv->madeDecision =3D true;

I think you should do this before priv->request->allow() since it is imposs=
ible
to ensure no side effect from allow() will re-enter this path.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:76
> +    priv->request->deny();
> +    priv->madeDecision =3D true;

Ditto.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:66
> +#if ENABLE(MEDIA_STREAM)
> +=20=20=20
m_page->process()->send(Messages::WebPage::DidReceiveUserMediaPermissionDec=
isio
n(userMediaRequestID, allowed), m_page->pageID());
> +#else
> +    UNUSED_PARAM(allowed);
> +#endif
> +
> +    m_pendingRequests.remove(it);

You should take a new ref of UserMediaPermissionRequestProxy, and remove it
immediately from the HashMap, then call send on the process.

That way you avoid the risk of holding an iterator after the ADT was modifi=
ed.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:4106
> +				4A2020A0181C15DE00E17834 /* MediaStream */,
>				755422C518064FFC0046F6A8 /* OriginData */,
>				512E352A130B559900ABD19A /* ApplicationCache
*/,

Build sections should be sorted alphabetically. Same for the the ones bello=
w.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:48
> +

Extra blank line.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:49
> +void
UserMediaRequestManager::requestPermission(PassRefPtr<WebCore::UserMediaReq=
uest
> prpRequest)

prp?

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:62
> +    WebFrameLoaderClient* webFrameLoaderClient =3D
toWebFrameLoaderClient(frame->loader().client());
> +    WebFrame* webFrame =3D webFrameLoaderClient ?
webFrameLoaderClient->webFrame() : nullptr;
> +    ASSERT(webFrame);

Seriously? That is the only way to get the WebFrame? :(

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:77
> +    for (UserMediaRequestToIDMap::const_iterator it =3D
m_userMediaRequestToIDMap.begin(), end =3D m_userMediaRequestToIDMap.end();=
 it !=3D
end; ++it) {
> +	   RefPtr<WebCore::UserMediaRequest> storedRequest =3D it->key;
> +	   if (storedRequest.get() =3D=3D request) {
> +	       m_idToUserMediaRequestMap.remove(it->value);
> +	       m_userMediaRequestToIDMap.remove(it->key);
> +	       return;
> +	   }
> +    }

I don't think that is what this is intended to do.

The idea behind the cancelRequests APIs is you can dismiss a dialog if it w=
as
never shown to the user. For example, if you make your dialog tab-modal, and
the tab that shows the dialog is not visible, you can dismiss the dialog
without the user ever knowing.

The reason this is not implemented in managers is simply that OS X does not=
 do
Tab Modal.

I don't mind this code. You can keep it as it is if it is enough for GTK. B=
ut
if you expose a frame-based API for request, you may want to expose this so
that your browser/apps can do a better job at presenting the information to=
 the
user.

> Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:36
> +WebUserMediaClient::~WebUserMediaClient()
> +{
> +}

This is unnecessary.

> Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h:34
> +    WebUserMediaClient(WebPage* page)
> +	   : m_page(page)

You should use references instead of pointers for page and m_page.=


More information about the webkit-reviews mailing list