[webkit-reviews] review granted: [Bug 174500] WebRTC data channel only applications require capture permissions for direct connections : [Attachment 336720] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 174500: WebRTC data channel only applications require capture permissions
for direct connections
https://bugs.webkit.org/show_bug.cgi?id=174500

Attachment 336720: Patch

https://bugs.webkit.org/attachment.cgi?id=336720&action=review




--- Comment #21 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 336720
  --> https://bugs.webkit.org/attachment.cgi?id=336720
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336720&action=review

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:289
> +	       provider.resolveMDNSName(name, [peerConnection =
makeRef(m_peerConnection), this, name, iceCandidate = makeRef(*iceCandidate),
promise = WTFMove(promise)] (auto&& result) mutable {

Nit: I think "auto" is not a good choice here because the only way for someone
that isn't already familiar with the code to figure out "result"'s type is to
dig through the libWebRTCProvider header.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:295
> +		       // FIXME: Log error.

It is probably worth logging the error to the console in this patch, even if
that isn't the long term solution.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:435
> +	   // FIXME: We might need to clear al pending candidates when setting
again local description.

Nits: "al" => "all"
"when setting again local description" => "when setting local description
again"

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:467
> +    provider.registerMDNSName(document, ipAddress, [peerConnection =
makeRef(m_peerConnection), this, ipAddress] (auto&& result) {

Ditto the "auto" comment above.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:473
> +	       // FIXME: Log error.

Ditto the comment above about logging the error.

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:78
> +    virtual void unregisterMDNSNames(Document&) { }
> +
> +    virtual void registerMDNSName(Document& document, const String&
ipAddress, CompletionHandler<void(MDNSNameOrError&&)>&& callback)

It is a layering violation to use Document here. Can you just use the
identifier?

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:80
> +	  
request->connection->send(Messages::WebMDNSRegister::FinishedRegisteringMDNSNam
e { request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) },
0);

Why not log the error here, or pass the error value to be logged in the web
process?

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:92
> +	      
m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMD
NSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:96
> +	      
m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMD
NSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:104
> +    String baseName = createCanonicalUUIDString();

Nit: is the local variable necessary?

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:109
> +    if (ip == ( in_addr_t)(-1)) {

Nit: unnecessary space after the "(".

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:110
> +	  
m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMD
NSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto the comment above about logging or passing a meaningful error to the web
process.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:129
> +	  
m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMD
NSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:154
> +	  
connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName {
requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:173
> +	  
request->connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName
{ request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:181
> +	  
request->connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName
{ request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:193
> +	  
m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNS
Name { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:207
> +	  
m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNS
Name { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:214
> +	  
m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNS
Name { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/WebProcess/Network/webrtc/WebMDNSRegister.cpp:88
> +	   finishedRegisteringMDNSName(m_pendingRequestsIdentifier,
makeUnexpected(MDNSRegisterError::General));

Ditto.

> Source/WebKit/WebProcess/Network/webrtc/WebMDNSRegister.cpp:97
> +	   finishedResolvingMDNSName(m_pendingRequestsIdentifier,
makeUnexpected(MDNSRegisterError::General));

Ditto.