Bug 191493 – [Curl] Add API Test for Curl cookie backend.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

Cleanup Cookie database implementation to make them testable better. Then add unit tests.

[Basuke Suzuki]

Created attachment 354405 [details]
PATCH

[Basuke Suzuki]

Separate the initial test preparation and refactoring of code from the patch. https://bugs.webkit.org/show_bug.cgi?id=191406

[EWS Watchlist]

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.

[Basuke Suzuki]

Created attachment 354410 [details]
FIX Style

[Alex Christensen]

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?

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 354582 [details]
Fix

Move tests into its own namespace and make utility functions in FileSystem into PLATFORM(WIN_CAIRO)

[youenn fablet]

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.

[Basuke Suzuki]

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

[Basuke Suzuki]

Created attachment 354592 [details]
PATCH

[Alex Christensen]

(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.

[Basuke Suzuki]

(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.

[Basuke Suzuki]

Created attachment 354594 [details]
PATCH AGAIN

[WebKit Commit Bot]

Comment on attachment 354594 [details]
PATCH AGAIN

Clearing flags on attachment: 354594

Committed r238111: <https://trac.webkit.org/changeset/238111>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46005812>