Bug 82598 - [GTK] Add WebKitCookieManager::changed signal to WebKit2 GTK+ API
Summary: [GTK] Add WebKitCookieManager::changed signal to WebKit2 GTK+ API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 82441
Blocks: 83016
  Show dependency tree / graph
 
Reported: 2012-03-29 04:54 PDT by Carlos Garcia Campos
Modified: 2012-04-24 08:19 PDT (History)
2 users (show)

See Also:

Attachments
Patch (11.33 KB, patch)
2012-03-29 05:02 PDT, Carlos Garcia Campos 		no flags Details | Formatted Diff | Diff
Updated patch (11.48 KB, patch)
2012-04-03 01:50 PDT, Carlos Garcia Campos 		pnormand: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-03-29 04:54:29 PDT 
To notify when cookies have changed.
Comment 1 Carlos Garcia Campos 2012-03-29 05:02:22 PDT 
Created attachment 134552 [details]
Patch
Comment 2 Sergio Villar Senin 2012-03-29 05:06:58 PDT 
Comment on attachment 134552 [details]
Patch

looks great to me
Comment 3 Martin Robinson 2012-03-29 08:44:51 PDT 
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?
Comment 4 Carlos Garcia Campos 2012-03-29 08:47:55 PDT 
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
Comment 5 Carlos Garcia Campos 2012-04-03 01:50:45 PDT 
Created attachment 135292 [details]
Updated patch

Stop observing changes in finalize.
Comment 6 Philippe Normand 2012-04-24 06:46:11 PDT 
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?
Comment 7 Carlos Garcia Campos 2012-04-24 08:15:05 PDT 
(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.
Comment 8 Carlos Garcia Campos 2012-04-24 08:19:03 PDT 
Committed r115057: <http://trac.webkit.org/changeset/115057>