Bug 194075 – [SOUP] Move cookiePersistentStoragePath and cookiePersistentStorageType from NetworkProcess to NetworkSession

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

Move cookiePersistentStoragePath and cookiePersistentStorageType from NetworkProcess to NetworkSession to reduce globals. This removes two of the six soup-specific variables in NetworkProcessCreationParameters. More to come....

[Michael Catanzaro]

Created attachment 360687 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 360687 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:169
> +    auto sessionID = webkitWebsiteDataManagerGetDataStore(manager->priv->dataManager).websiteDataStore().sessionID();

We should not allow to set a persistent cookie storage in ephemeral sessions. Return early here if sessionID is ephemeral. That's the reason why we used the default session ID unconditionally, for now in soup based ports all sessions but the default one are ephemeral.

> Source/WebKit/UIProcess/soup/WebCookieManagerProxySoup.cpp:44
> +    auto pair = m_cookiePersistentStorageMap.get(sessionID);
> +    storagePath = pair.first;
> +    storageType = static_cast<uint32_t>(pair.second);

You should check the return value in case the sessionID is not present in the map. Or add an assert if it should always be.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #3)
> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:169
> > +    auto sessionID = webkitWebsiteDataManagerGetDataStore(manager->priv->dataManager).websiteDataStore().sessionID();
> 
> We should not allow to set a persistent cookie storage in ephemeral
> sessions. Return early here if sessionID is ephemeral. That's the reason why
> we used the default session ID unconditionally, for now in soup based ports
> all sessions but the default one are ephemeral.

OK.

> > Source/WebKit/UIProcess/soup/WebCookieManagerProxySoup.cpp:44
> > +    auto pair = m_cookiePersistentStorageMap.get(sessionID);
> > +    storagePath = pair.first;
> > +    storageType = static_cast<uint32_t>(pair.second);
> 
> You should check the return value in case the sessionID is not present in
> the map. Or add an assert if it should always be.

If it's not present then an empty value gets used (I think empty string and the first storage type policy) so the end result would be the same, right?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> If it's not present then an empty value gets used (I think empty string and
> the first storage type policy) so the end result would be the same, right?

Verified it returns empty string for path, and 0 (SoupCookiePersistentStorageText) for type.

[Michael Catanzaro]

Committed r240785: <https://trac.webkit.org/changeset/240785>