Bug 168229 – CookieManager only works with the default session

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

It should be possible to use the cookie manager also for ephemeral sessions.

[Carlos Garcia Campos]

Created attachment 301344 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Attachment 301344 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/CookieStorage.h:34:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:34:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:38:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:56:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:68:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:77:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:113:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:44:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:46:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:96:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:136:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:31:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:33:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:43:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 301344 [details]
Patch

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

Nice work! I trust you'll find an owner to review it.

Any reason you used std::function instead of the preferred WTF::Function?

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:46
> +    static NeverDestroyed<HashMap<CFHTTPCookieStorageRef, std::function<void ()>>> map;

You need to fix Windows:

C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\network\cf\CookieStorageCFNet.cpp(46): error C2079: 'map' uses undefined class 'WTF::NeverDestroyed<WTF::HashMap<CFHTTPCookieStorageRef,std::function<void (void)>,WTF::PtrHash<P *>,WTF::HashTraits<KeyArg>,WTF::HashTraits<MappedArg>>>' [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
          with
          [
              P=OpaqueCFHTTPCookieStorage,
              KeyArg=CFHTTPCookieStorageRef,
              MappedArg=std::function<void (void)>
          ]

Probably a missing #include?

[Alex Christensen]

Comment on attachment 301344 [details]
Patch

I like the direction this code is going in.  Definitely fix the Windows build.  WTF::Functions are not copyable, and that seems preferable in this case.  Can this be tested?

[Alex Christensen]

(In reply to comment #5)
> Can this be tested?
Aha!  This is definitely related to https://bugs.webkit.org/show_bug.cgi?id=168230

[Alex Christensen]

Comment on attachment 301344 [details]
Patch

r=me, fix Windows.
Let's get rid of CookiesStrategy, too!

[Michael Catanzaro]

(In reply to comment #6)
> (In reply to comment #5)
> > Can this be tested?
> Aha!  This is definitely related to
> https://bugs.webkit.org/show_bug.cgi?id=168230

Yeah, it's because we just exposed ephemeral mode. See https://lists.webkit.org/pipermail/webkit-gtk/2017-February/002911.html. Thanks for reviewing it quickly, because we want to freeze API for the next release now!

[Carlos Garcia Campos]

(In reply to comment #4)
> Comment on attachment 301344 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301344&action=review
> 
> Nice work! I trust you'll find an owner to review it.
> 
> Any reason you used std::function instead of the preferred WTF::Function?

Because a non copyable function can't be used as a value of a HashMap.
 
> > Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:46
> > +    static NeverDestroyed<HashMap<CFHTTPCookieStorageRef, std::function<void ()>>> map;
> 
> You need to fix Windows:
> 
> C:
> \cygwin\home\buildbot\WebKit\Source\WebCore\platform\network\cf\CookieStorage
> CFNet.cpp(46): error C2079: 'map' uses undefined class
> 'WTF::NeverDestroyed<WTF::HashMap<CFHTTPCookieStorageRef,std::function<void
> (void)>,WTF::PtrHash<P
> *>,WTF::HashTraits<KeyArg>,WTF::HashTraits<MappedArg>>>'
> [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.
> vcxproj]
>           with
>           [
>               P=OpaqueCFHTTPCookieStorage,
>               KeyArg=CFHTTPCookieStorageRef,
>               MappedArg=std::function<void (void)>
>           ]
> 
> Probably a missing #include?

Yes.

[Alex Christensen]

(In reply to comment #9)
> > Any reason you used std::function instead of the preferred WTF::Function?
> 
> Because a non copyable function can't be used as a value of a HashMap.
You're right :(

[Carlos Garcia Campos]

(In reply to comment #5)
> Comment on attachment 301344 [details]
> Patch
> 
> I like the direction this code is going in.  Definitely fix the Windows
> build.  WTF::Functions are not copyable, and that seems preferable in this
> case.  Can this be tested?

I started using Function but had to change it to std::function to be able to use it in HashMap value.

[Carlos Garcia Campos]

Created attachment 301478 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 301478 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/CookieStorage.h:34:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:34:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:38:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:56:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:68:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:77:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:113:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:46:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:48:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:71:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:96:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:136:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:31:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:33:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:43:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Committed r212283: <http://trac.webkit.org/changeset/212283>

[Brady Eidson]

(In reply to comment #11)
> (In reply to comment #5)
> > Comment on attachment 301344 [details]
> > Patch
> > 
> > I like the direction this code is going in.  Definitely fix the Windows
> > build.  WTF::Functions are not copyable, and that seems preferable in this
> > case.  Can this be tested?
> 
> I started using Function but had to change it to std::function to be able to
> use it in HashMap value.

Nothing about a Function precludes it from being a HashMap value; We use Functions in HashMap values all the time.

What was preventing you from doing so?

Could you please revisit this?

[Brady Eidson]

(In reply to comment #15)
> (In reply to comment #11)
> > (In reply to comment #5)
> > > Comment on attachment 301344 [details]
> > > Patch
> > > 
> > > I like the direction this code is going in.  Definitely fix the Windows
> > > build.  WTF::Functions are not copyable, and that seems preferable in this
> > > case.  Can this be tested?
> > 
> > I started using Function but had to change it to std::function to be able to
> > use it in HashMap value.
> 
> Nothing about a Function precludes it from being a HashMap value; We use
> Functions in HashMap values all the time.

For hints, search WebKit2 for:
`HashMap<uint64_t, Function`

[Carlos Garcia Campos]

(In reply to comment #15)
> (In reply to comment #11)
> > (In reply to comment #5)
> > > Comment on attachment 301344 [details]
> > > Patch
> > > 
> > > I like the direction this code is going in.  Definitely fix the Windows
> > > build.  WTF::Functions are not copyable, and that seems preferable in this
> > > case.  Can this be tested?
> > 
> > I started using Function but had to change it to std::function to be able to
> > use it in HashMap value.
> 
> Nothing about a Function precludes it from being a HashMap value; We use
> Functions in HashMap values all the time.
> 
> What was preventing you from doing so?
> 
> Could you please revisit this?

You are right, now I remember exactly the issue. You can indeed use Function as a value of a HshMap, but you can't get it, because that always requires a copy, at least with the current HashMap implementation, I'm not sure if it's possible to implement a git without copying.

[Carlos Garcia Campos]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #11)
> > > (In reply to comment #5)
> > > > Comment on attachment 301344 [details]
> > > > Patch
> > > > 
> > > > I like the direction this code is going in.  Definitely fix the Windows
> > > > build.  WTF::Functions are not copyable, and that seems preferable in this
> > > > case.  Can this be tested?
> > > 
> > > I started using Function but had to change it to std::function to be able to
> > > use it in HashMap value.
> > 
> > Nothing about a Function precludes it from being a HashMap value; We use
> > Functions in HashMap values all the time.
> 
> For hints, search WebKit2 for:
> `HashMap<uint64_t, Function`

I did it when I found the issue, and all those were cases where function is added once and then taken, like completion handlers. In that case there's no problem because take doesn't need to copy, it just moves. Here we need to set once and get multiple times. We could take and set on every get, but I don't think it's worth it.

[Michael Catanzaro]

I guess it caused bug #168375.