[webkit-reviews] review canceled: [Bug 191406] [Curl] Reject entire cookie if the domain fails a tailmatch. : [Attachment 354369] PATCH

[bugzilla-daemon at webkit.org]

Basuke Suzuki <Basuke.Suzuki at sony.com> has canceled Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 191406: [Curl] Reject entire cookie if the domain fails a tailmatch.
https://bugs.webkit.org/show_bug.cgi?id=191406

Attachment 354369: PATCH

https://bugs.webkit.org/attachment.cgi?id=354369&action=review




--- Comment #9 from Basuke Suzuki <Basuke.Suzuki at sony.com> ---
Comment on attachment 354369
  --> https://bugs.webkit.org/attachment.cgi?id=354369
PATCH

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

Separate the refactoring and initial preparation of the unit test file to
https://bugs.webkit.org/show_bug.cgi?id=191493

>> Source/WebCore/platform/network/curl/CookieJarDB.cpp:460
>>  int CookieJarDB::setCookie(const String& url, const String& cookie, bool
fromJavaScript)
> 
> Could it return a bool instead?
> Or are we passing some meaningful error codes?

bool must be fine.

>> Source/WebCore/platform/network/curl/CookieUtil.cpp:140
>> +bool parseCookieHeader(const String& cookieLine, Cookie& result)
> 
> Might be a follow-up patch but we now like to do things like:
> std::optional<Cookie> parseCookieHeader(const String& cookieLine);
> or ExceptionOr<Cookie> if there is a need to provide more accurate error
handling.

Good to catch this improvement on this patch. Thanks

>> Source/WebCore/platform/network/curl/CookieUtil.h:37
>> +bool parseCookieHeader(const String& cookieLine, Cookie& result);
> 
> s/ result//

Might change using optional as above.

>> Source/WebCore/platform/win/FileSystemWin.cpp:379
>> +static String generateTemporaryPath(Function<bool(const String&)> action)
> 
> Can action be a const Function&?

I try. Let me see.

>> Source/WebCore/platform/win/FileSystemWin.cpp:412
>> +	String proposedPath = generateTemporaryPath([&handle](const String
proposedPath) {
> 
> s/const String/const String&/

Right.

>> Source/WebCore/platform/win/FileSystemWin.cpp:563
>> +String createTemporaryDirectory(String directoryPrefix)
> 
> directoryPrefix does not seem to be used.
> Also it is a String, should it be a const String&.

Right.

>> Source/WebCore/platform/win/FileSystemWin.cpp:569
>> +	return proposedPath;
> 
> Maybe we could remove this temporary variable and directly write 'return
generate...'
> This would remove the use of two different 'proposedPath' variables in the
same function.

Right.

>> Tools/ChangeLog:1
>> +2018-11-08	Christopher Reid  <chris.reid at sony.com>
> 
> Is it a patch from Christopher, from Basuke or a joint work?

This is Chris's work and tested by me.

>> Tools/TestWebKitAPI/Tests/WebCore/curl/Cookies.cpp:66
>> +	EXPECT_EQ(0, m_cookieJar->setCookie("http://example.com", "foo=bar;
Domain=example.com", false));
> 
> If we return a boolean, we would write it:
> EXPECT_TRUE(m_cookieJar->setCookie("http://example.com", "foo=bar;
Domain=example.com", false));
> which seems a bit more readable.
> 
> Also false is not super readable.
> We could write it as:
> bool fromJavaScript = false here or use an enumeration instead of a bool.
> 
> There are no tests for setCookie with fromJavaScript = true.
> Should it be tested as well?

It should be. I'll change the code using Enum.