Cleanup Cookie database implementation to make them testable better. Then add unit tests.
Created attachment 354405 [details] PATCH
Separate the initial test preparation and refactoring of code from the patch. https://bugs.webkit.org/show_bug.cgi?id=191406
Attachment 354405 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/CookieJarDB.cpp:152: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 354410 [details] FIX Style
Comment on attachment 354410 [details] FIX Style View in context: https://bugs.webkit.org/attachment.cgi?id=354410&action=review > Source/WebCore/platform/FileSystem.h:190 > +#if OS(WINDOWS) There's already a PLATFORM(WIN) just above this. > Tools/TestWebKitAPI/Tests/WebCore/curl/Cookies.cpp:35 > +using namespace WebCore; Could we put this inside the namespace in case we unify these sources, too?
Comment on attachment 354410 [details] FIX Style View in context: https://bugs.webkit.org/attachment.cgi?id=354410&action=review Thanks for review! >> Source/WebCore/platform/FileSystem.h:190 >> +#if OS(WINDOWS) > > There's already a PLATFORM(WIN) just above this. I meant this for both AppleWin and WinCairo because there's not WinCairo specific code. >> Tools/TestWebKitAPI/Tests/WebCore/curl/Cookies.cpp:35 >> +using namespace WebCore; > > Could we put this inside the namespace in case we unify these sources, too? Ah, okay.
Created attachment 354582 [details] Fix Move tests into its own namespace and make utility functions in FileSystem into PLATFORM(WIN_CAIRO)
Comment on attachment 354582 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=354582&action=review > Source/WebCore/platform/network/curl/CookieJarCurlDatabase.cpp:51 > + for (auto cookie : *result) { s/auto/auto& > Source/WebCore/platform/network/curl/CookieJarDB.cpp:434 > + SQLiteStatement* statement = getPrepareStatement(SET_COOKIE_SQL); s/SQLiteStatement/auto/ Also maybe getPrepareStatement should return a SQLiteStatement& > Source/WebCore/platform/network/curl/CookieJarDB.h:43 > + enum class Source : uint8_t { Do we need " : uint8_t"? > Source/WebCore/platform/network/curl/CookieUtil.cpp:147 > + return std::nullopt; You can also write it { }, which is shorter. Not exactly sure what is the recommended way.
Comment on attachment 354582 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=354582&action=review Thanks! >> Source/WebCore/platform/network/curl/CookieJarCurlDatabase.cpp:51 >> + for (auto cookie : *result) { > > s/auto/auto& Sure. >> Source/WebCore/platform/network/curl/CookieJarDB.cpp:434 >> + SQLiteStatement* statement = getPrepareStatement(SET_COOKIE_SQL); > > s/SQLiteStatement/auto/ > Also maybe getPrepareStatement should return a SQLiteStatement& Got it. Also will rename getPrepareStatement to preparedStatement. >> Source/WebCore/platform/network/curl/CookieJarDB.h:43 >> + enum class Source : uint8_t { > > Do we need " : uint8_t"? Not specifically. Will remove it. >> Source/WebCore/platform/network/curl/CookieUtil.cpp:147 >> + return std::nullopt; > > You can also write it { }, which is shorter. > Not exactly sure what is the recommended way. Oh, I didn't know that. Thanks
Created attachment 354592 [details] PATCH
(In reply to youenn fablet from comment #8) > Comment on attachment 354582 [details] > Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=354582&action=review > > > Source/WebCore/platform/network/curl/CookieJarCurlDatabase.cpp:51 > > + for (auto cookie : *result) { > > s/auto/auto& > > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:434 > > + SQLiteStatement* statement = getPrepareStatement(SET_COOKIE_SQL); > > s/SQLiteStatement/auto/ > Also maybe getPrepareStatement should return a SQLiteStatement& > > > Source/WebCore/platform/network/curl/CookieJarDB.h:43 > > + enum class Source : uint8_t { > > Do we need " : uint8_t"? We are preferring this when possible in case they get stored someday. In fact, you can't send an enum class without a storage class over IPC now. > > > Source/WebCore/platform/network/curl/CookieUtil.cpp:147 > > + return std::nullopt; > > You can also write it { }, which is shorter. > Not exactly sure what is the recommended way. std::nullopt is preferred in WebKit. It's more explicit.
(In reply to Alex Christensen from comment #11) > (In reply to youenn fablet from comment #8) > > Comment on attachment 354582 [details] > > Fix > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=354582&action=review > > > > > Source/WebCore/platform/network/curl/CookieJarCurlDatabase.cpp:51 > > > + for (auto cookie : *result) { > > > > s/auto/auto& > > > > > Source/WebCore/platform/network/curl/CookieJarDB.cpp:434 > > > + SQLiteStatement* statement = getPrepareStatement(SET_COOKIE_SQL); > > > > s/SQLiteStatement/auto/ > > Also maybe getPrepareStatement should return a SQLiteStatement& > > > > > Source/WebCore/platform/network/curl/CookieJarDB.h:43 > > > + enum class Source : uint8_t { > > > > Do we need " : uint8_t"? > We are preferring this when possible in case they get stored someday. In > fact, you can't send an enum class without a storage class over IPC now. Got it. I couldn't find any reason at this moment for this, but it may be contained in IPC someday. > > > > > Source/WebCore/platform/network/curl/CookieUtil.cpp:147 > > > + return std::nullopt; > > > > You can also write it { }, which is shorter. > > Not exactly sure what is the recommended way. > std::nullopt is preferred in WebKit. It's more explicit. Oh, okay. Thanks.
Created attachment 354594 [details] PATCH AGAIN
Comment on attachment 354594 [details] PATCH AGAIN Clearing flags on attachment: 354594 Committed r238111: <https://trac.webkit.org/changeset/238111>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46005812>