[webkit-reviews] review granted: [Bug 199081] Make HTTPCookieAcceptPolicy an enum class : [Attachment 372847] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 199081: Make HTTPCookieAcceptPolicy an enum class
https://bugs.webkit.org/show_bug.cgi?id=199081

Attachment 372847: Patch

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




--- Comment #8 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 372847
  --> https://bugs.webkit.org/attachment.cgi?id=372847
Patch

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

> Source/WebKit/ChangeLog:10
> +	   WKPreferencesGetIncrementalRenderingSuppressionTimeout was using its
toAPI function to convert a double to a double because HTTPCookieAcceptPolicy
used to be an unsigned integer.
> +	   toAPI(WebCore::MouseButton) was also using the
toAPI(HTTPCookieAcceptPolicy) because HTTPCookieAcceptPolicy used to be an
unsigned integer.

Wow

> Source/WebKit/NetworkProcess/Cookies/curl/WebCookieManagerCurl.cpp:49
> -    case HTTPCookieAcceptPolicyOnlyFromMainDocumentDomain:
> +    case HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain:
>	   curlPolicy = CookieAcceptPolicy::OnlyFromMainDocumentDomain;
>	   break;
> -    case HTTPCookieAcceptPolicyExclusivelyFromMainDocumentDomain:
> +    case HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain:

Of course these names are nonsense and now would be a good time to rename them
(discussed in bug #193458).

> Source/WebKit/NetworkProcess/Cookies/mac/WebCookieManagerMac.mm:40
> +    return static_cast<CFHTTPCookieStorageAcceptPolicy>(policy);

Seems fragile... we won't think to update this if we reorder the members of
HTTPCookieAcceptPolicy, for instance. A switch would be safer, like you used
below.

> Source/WebKit/UIProcess/WebCookieManagerProxy.h:81
> +    void setHTTPCookieAcceptPolicySynchronouslyForTesting(PAL::SessionID,
HTTPCookieAcceptPolicy);

Oops, looks like this was WIP and should be deleted before landing? It's not
implemented.