[webkit-reviews] review denied: [Bug 219274] ICE does not resolve for `turns` relay candidates rooted in LetsEncrypt CA : [Attachment 415015] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied youenn fablet
<youennf at gmail.com>'s request for review:
Bug 219274: ICE does not resolve for `turns` relay candidates rooted in
LetsEncrypt CA
https://bugs.webkit.org/show_bug.cgi?id=219274

Attachment 415015: Patch

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




--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 415015
  --> https://bugs.webkit.org/attachment.cgi?id=415015
Patch

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

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:66
> +    if (size < 4)
> +	   return std::make_pair(0, 0);

Should this be nullopt because it doesn't have enough bytes to make a size?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:146
> +	       dispatch_data_apply(content, makeBlockPtr([&](dispatch_data_t,
size_t, const void* buffer, size_t size) {
> +		   processData(static_cast<const uint8_t*>(buffer), size);

Since we're putting all the data into a Vector anyways, I think it would make
the code easier to reason about and less error prone if we just did that here. 
Then we could also use dispatch_data_get_size and reserve the size up front and
use an unchecked append function.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:220
> +void NetworkRTCSocketSocketCocoa::setOption(int, int)

Should this have FIXME: implement. ?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:246
> +	   std::memset(buffer.data() + size, 0, messageLengths.second -
messageLengths.first);

Can the first be greater than the second?
I think a struct with named members would make this easier to understand than a
std::pair

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCSocketSocketCocoa.mm:262
> +    if (size > 65536)

Should this be >= ?
Also, std::numeric_limits<uint16_t> might be nice.