[webkit-reviews] review denied: [Bug 32085] WebSocket should block the same ports that are blocked for resource loading : [Attachment 44191] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 32085: WebSocket should block the same ports that are blocked for resource
loading
https://bugs.webkit.org/show_bug.cgi?id=32085

Attachment 44191: proposed patch
https://bugs.webkit.org/attachment.cgi?id=44191&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   Move isDefaultPortForProtocol() to KURL, because that's a better
place for it (SecurityOrigin
> +	   is not even in WebCore/platform directory).

I think KURL.h is a good source file for this. But I would slightly prefer a
free function to a static member function.

> +    typedef HashMap<String, unsigned> DefaultPortsMap;
> +    DEFINE_STATIC_LOCAL(DefaultPortsMap, defaultPorts, ());
> +    if (defaultPorts.isEmpty()) {
> +	   defaultPorts.set("http", 80);
> +	   defaultPorts.set("https", 443);
> +	   defaultPorts.set("ftp", 21);
> +	   defaultPorts.set("ftps", 990);
> +    }

Is it safe to use a case-sensitive map for this? Do callers all lowercase the
protocol first? Should we assert that the passed in string has no uppercase
ASCII letters in it?

> +bool KURL::portAllowed(const KURL& url)

This should be an ordinary member function, not a static member function. Or
alternatively it could be a free function.

> +    // If the port is not in the blocked port list, allow it.
> +    if (!std::binary_search(blockedPortList, blockedPortListEnd, port))
> +	   return true;

I'd like to see "using namespace std" rather than "std::binary_search".

I'd like to see an assertion that the list is sorted properly, because it seems
it would be easy to accidentally add a new port and not keep the list sorted.

I'm going to say review- because I think you should do at least one of the
things I suggest above. If you don't agree, feel free to put this same patch up
for review again.