[webkit-reviews] review denied: [Bug 104370] [GTK] Cookies' storage path and policy are not set when the WebProcess is killed and relaunched : [Attachment 178222] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Joaquim Rocha
<jrocha at igalia.com>'s request for review:
Bug 104370: [GTK] Cookies' storage path and policy are not set when the
WebProcess is killed and relaunched
https://bugs.webkit.org/show_bug.cgi?id=104370

Attachment 178222: Patch
https://bugs.webkit.org/attachment.cgi?id=178222&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=178222&action=review


I've noticed that WIN has WebContext::setInitialHTTPCookieAcceptPolicy() maybe
we could reuse that and save the values in the context instead of the cookies
proxy.

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:83
> +    encoder << cookieAcceptPolicy;

I think you should use encoder.encodeEnum() for enums.

> Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:140
> +#if USE(SOUP)
> +    setCookieAcceptPolicy(policy);
> +#endif

You can simply do m_cookieAcceptPolicy = policy;

> Source/WebKit2/UIProcess/WebCookieManagerProxy.h:75
> +    void setCookieAcceptPolicy(const HTTPCookieAcceptPolicy&);

This method is confusing, because there's setHTTPCookieAcceptPolicy and
setCookieAcceptPolicy, but it's not needed it at all, since we don't expect it
to be used by others.

> Source/WebKit2/UIProcess/WebCookieManagerProxy.h:101
> +    HTTPCookieAcceptPolicy m_cookieAcceptPolicy;
> +    pair<String, uint32_t> m_cookiePersistentStorage;

These should be initialized in the constructor.

> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:49
> +    const pair<String, uint32_t>& storage =
m_cookieManagerProxy->cookiePersistentStorage();
> +    parameters.cookiePersistentStoragePath = storage.first;
> +    parameters.cookiePersistentStorageType = storage.second;

Maybe this could be simpler if we had a method returning two parameters instead
of pair? something like:

m_cookieManagerProxy->getCookiePersistentStorage(parameters.cookiePersistentSto
ragePath, parameters.cookiePersistentStorageType);

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:93
> +    const pair<String, uint32_t>& storage =
m_cookieManagerProxy->cookiePersistentStorage();
> +    parameters.cookiePersistentStoragePath = storage.first;
> +    parameters.cookiePersistentStorageType = storage.second;

Ditto.

> Source/WebKit2/UIProcess/soup/WebCookieManagerProxySoup.cpp:54
> +const pair<String, uint32_t>&
WebCookieManagerProxy::cookiePersistentStorage() const
> +{
> +    return m_cookiePersistentStorage;
> +}
> +
> +void WebCookieManagerProxy::setCookieAcceptPolicy(const
HTTPCookieAcceptPolicy& policy)
> +{
> +    m_cookieAcceptPolicy = policy;
> +}
> +
> +const HTTPCookieAcceptPolicy& WebCookieManagerProxy::cookieAcceptPolicy()
const
> +{
> +    return m_cookieAcceptPolicy;

These could be in the header.

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:173
> +   
WebCookieManager::shared().setCookiePersistentStorage(parameters.cookiePersiste
ntStoragePath, parameters.cookiePersistentStorageType);

You should check cookiePersistentStoragePath is not empty before calling
setCookiePersistentStorage(), as it will happen the first time the web process
is launched.