Bug 82598 – [GTK] Add WebKitCookieManager::changed signal to WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

To notify when cookies have changed.

[Carlos Garcia Campos]

Created attachment 134552 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 134552 [details]
Patch

looks great to me

[Martin Robinson]

Comment on attachment 134552 [details]
Patch

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

Seems good to me, but I have the following doubt:

> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:58
> +    WKCookieManagerStartObservingCookieChanges(priv->wkCookieManager.get());

Did you mean to write WKCookieManagerStopObservingCookieChanges here?

[Carlos Garcia Campos]

Comment on attachment 134552 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.cpp:58
>> +    WKCookieManagerStartObservingCookieChanges(priv->wkCookieManager.get());
> 
> Did you mean to write WKCookieManagerStopObservingCookieChanges here?

Indeed, copy paste error, good catch! :-P

[Carlos Garcia Campos]

Created attachment 135292 [details]
Updated patch

Stop observing changes in finalize.

[Philippe Normand]

Comment on attachment 135292 [details]
Updated patch

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

LGTM

> Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:36
> +void setCookieStoragePrivateBrowsingEnabled(bool enabled)
> +{
> +    notImplemented();

Would it make sense to start/stop observing cookies there? depending on the enabled value.
I can't find this function used in WebKit2 btw, is it part of some DerivedSources or was it removed from the other ports?

[Carlos Garcia Campos]

(In reply to comment #6)
> (From update of attachment 135292 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135292&action=review
> 
> LGTM
> 
> > Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:36
> > +void setCookieStoragePrivateBrowsingEnabled(bool enabled)
> > +{
> > +    notImplemented();
> 
> Would it make sense to start/stop observing cookies there? depending on the enabled value.
> I can't find this function used in WebKit2 btw, is it part of some DerivedSources or was it removed from the other ports?

This function is declared in CookieStorage.h so we need to provide an implementation even if it's empty. When private browsing is enabled, the cookie storage used for private browsing will be observed for changes, so I don't think we need to do anything here.

[Carlos Garcia Campos]

Committed r115057: <http://trac.webkit.org/changeset/115057>