It should be possible to use the cookie manager also for ephemeral sessions.
Created attachment 301344 [details] Patch
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
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.
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?
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?
(In reply to comment #5) > Can this be tested? Aha! This is definitely related to https://bugs.webkit.org/show_bug.cgi?id=168230
Comment on attachment 301344 [details] Patch r=me, fix Windows. Let's get rid of CookiesStrategy, too!
(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!
(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.
(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 :(
(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.
Created attachment 301478 [details] Patch for landing
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.
Committed r212283: <http://trac.webkit.org/changeset/212283>
(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?
(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`
(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.
(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.
I guess it caused bug #168375.