Bug 174942

Created attachment 331552 [details] Patch

Comment on attachment 331552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331552&action=review I feel like this patch has a lot of parsers. Are there not existing parsers for these things? > Source/WebCore/ChangeLog:3 > + [Curl] Use SQLite database in cookie jar implementation for Curl port Cool! > Source/WebCore/platform/network/curl/CookieJarCurlDatabase.cpp:89 > + CookieJarDB* cookieJarDB = CookieJarDB::getInstance(); This should be implemented by NetworkStorageSession instead of having a global singleton. > Source/WebCore/platform/network/curl/CookieJarDB.cpp:76 > +CookieJarDB* CookieJarDB::getInstance() This could return a reference. > Source/WebCore/platform/network/curl/CookieJarDB.cpp:80 > + CookieJarDB::s_instance = new CookieJarDB(); This should use smart pointers or NeverDestroyed to avoid explicit new/delete calls. > Source/WebCore/platform/network/curl/CookieJarDB.cpp:84 > +void CookieJarDB::releaseInstance() This is never called. > Source/WebCore/platform/network/curl/CookieJarDB.cpp:191 > +void CookieJarDB::detectDatabaseCorruption() Does this really detect anything? > Source/WebCore/platform/network/curl/CookieUtil.cpp:82 > +static bool parseExpires(const char* expires, time_t& time) This could return std::optional<time_t>

(In reply to Alex Christensen from comment #2) > Comment on attachment 331552 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331552&action=review > > I feel like this patch has a lot of parsers. Are there not existing parsers > for these things? I wasn't able to find any parsers for cookie strings within WebCore for other network layers. They all seem to be using a library for parsing. CookieJarSoup uses soup_cookie_parse CookieJarCF uses CFHTTPCookieCreateWithResponseHeaderFields CookieJarMac uses NSHTTPCookie's cookiesWithResponseHeaderFields getNetscapeCookieFormat in CookieJarCurl does some similar parsing but it can't be used directly since it's just returning a string in a different format. I can pull out that parsing code and share it with the cookie jar if that's preferred. > > Source/WebCore/ChangeLog:3 > > + [Curl] Use SQLite database in cookie jar implementation for Curl port > > Cool! > > > Source/WebCore/platform/network/curl/CookieJarCurlDatabase.cpp:89 > > + CookieJarDB* cookieJarDB = CookieJarDB::getInstance(); > > This should be implemented by NetworkStorageSession instead of having a > global singleton. > I'll look into using that. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:76 > > +CookieJarDB* CookieJarDB::getInstance() > > This could return a reference. > Will do. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:80 > > + CookieJarDB::s_instance = new CookieJarDB(); > > This should use smart pointers or NeverDestroyed to avoid explicit > new/delete calls. > Will do. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:84 > > +void CookieJarDB::releaseInstance() > > This is never called. > I'll take that out. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:191 > > +void CookieJarDB::detectDatabaseCorruption() > > Does this really detect anything? > flagDatabaseCorruption seems like a more suitable name. > > Source/WebCore/platform/network/curl/CookieUtil.cpp:82 > > +static bool parseExpires(const char* expires, time_t& time) > > This could return std::optional<time_t> Will do. Thanks for the feedback.

Created attachment 332812 [details] Patch Updated patch based on Alex's feedback. I also added a flag to use the old backend with ENABLE(CURL_COOKIE_STORAGE). I think we'd want to solely use the CookieJarDB backend in curl and remove handling cookies with libcurl, but I'm not sure if the libcurl cookie backend should be removed yet.

Attachment 332812 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] ERROR: Source/WebCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] Total errors found: 5 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 332814 [details] Patch Fixing ChangeLog style issues

Comment on attachment 332814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332814&action=review > Source/WebCore/platform/network/NetworkStorageSession.h:165 > + std::unique_ptr<CookieJarCurl> m_cookieStorage; These could be UniqueRef<CookieJarCurl> or just CookieJarCurl. > Source/WebCore/platform/network/NetworkStorageSession.h:166 > +#if !ENABLE(CURL_COOKIE_STORAGE) Is there a reason we want to leave the code from both cookie storage formats? Isn't the database format better? > Source/WebCore/platform/network/curl/CookieJarDB.cpp:260 > + std::unique_ptr<SQLiteStatement> pstmt; > + pstmt = std::make_unique<SQLiteStatement>(m_database, sql); You can assign to it on the same line.

(In reply to Alex Christensen from comment #7) > Comment on attachment 332814 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332814&action=review > > > Source/WebCore/platform/network/NetworkStorageSession.h:165 > > + std::unique_ptr<CookieJarCurl> m_cookieStorage; > > These could be UniqueRef<CookieJarCurl> or just CookieJarCurl. Will do. > > Source/WebCore/platform/network/NetworkStorageSession.h:166 > > +#if !ENABLE(CURL_COOKIE_STORAGE) > > Is there a reason we want to leave the code from both cookie storage > formats? Isn't the database format better? We don't need the old format in, I'll take it out. I was just worried that other projects using curl might not want to use the new format just yet. > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:260 > > + std::unique_ptr<SQLiteStatement> pstmt; > > + pstmt = std::make_unique<SQLiteStatement>(m_database, sql); > > You can assign to it on the same line. Will do.

(In reply to Christopher Reid from comment #8) > (In reply to Alex Christensen from comment #7) > > Comment on attachment 332814 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=332814&action=review > > > > > Source/WebCore/platform/network/NetworkStorageSession.h:165 > > > + std::unique_ptr<CookieJarCurl> m_cookieStorage; > > > > These could be UniqueRef<CookieJarCurl> or just CookieJarCurl. > > Will do. Actually, I'm not sure if I can do this. PlatformCookieJar.h dispatcher functions use a const NetworkStorageSession& and I would have to make all the calls to CookieJarDB const which doesn't seem ideal. I thought UniqueRef might allow modifying the underlying data in a const call but that doesn't seem to be the case. Is there any way around that or should I just stick with unique_ptr?

Created attachment 332865 [details] Patch I removed all the old curl cookie code and ended up using mutable to avoid using unique_ptr in NetworkStorageSession.

Created attachment 332866 [details] Patch Had the wrong line endings in the last patch.

Comment on attachment 332866 [details] Patch Clearing flags on attachment: 332866 Committed r227987: <https://trac.webkit.org/changeset/227987>

All reviewed patches have been landed. Closing bug.

<rdar://problem/37135651>

*** Bug 14730 has been marked as a duplicate of this bug. ***