[webkit-reviews] review denied: [Bug 185715] Conversion between SecurityOriginData and DatabaseIdentifier is asymmetric when port is null : [Attachment 340672] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 185715: Conversion between SecurityOriginData and DatabaseIdentifier is
asymmetric when port is null
https://bugs.webkit.org/show_bug.cgi?id=185715

Attachment 340672: Patch

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




--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 340672
  --> https://bugs.webkit.org/attachment.cgi?id=340672
Patch

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

Note that you could have written an API test specific to SecurityOriginData,
instead of a higher level one, since you change is specific to
SecurityOriginData. See SecurityOrigin.cpp in our API test folder for existing
SecurityOrigin API tests.

> Source/WebCore/ChangeLog:8
> +	   Fixed the issue of null port when coverting between
SecurityOriginData and DatabaseIdentifier.

Typo: coverting

> Source/WebCore/page/SecurityOriginData.cpp:118
> +	   return SecurityOriginData {databaseIdentifier.substring(0,
separator1), databaseIdentifier.substring(separator1 + 1, separator2 -
separator1 - 1), std::nullopt};

We may want to store databaseIdentifier.substring(0, separator1) and
databaseIdentifier.substring(separator1 + 1, separator2 - separator1 - 1) in
local variables with meaningful names, to avoid duplication and improve
readability.

We could also use ", port ? make_optional<uint16_t>(port) : std::nullopt)"
below. This may actually be nicer.

Also, I know you copied the code below but we normally have spaces between the
curly brackets. return SecurityOriginData { a, b, c };

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:153
> +    auto array = [[NSMutableArray alloc] init];

initWithCapacity since we know the size in advance?

Also, may be nice to use NSMutableArray<NSString *> instead of auto here. I
think auto ends up being NSMutableArray which is not as strongly typed.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:155
> +	   [array addObject:[[NSString alloc]
initWithCString:origin.toString().utf8().data()
encoding:NSUTF8StringEncoding]];

a WTF String can implicitly be converted into a NSString. I think this should
work:
[array addObject:origin.toString()];
If not, maybe:
[array addObject:(NSString *)origin.toString()];

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:156
> +    return array;

I suck at objc but aren't we supposed to autorelease here? since we transfer
ownership.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecordPrivate.h:49
> +- (NSArray<NSString *> *)_originsString WK_API_AVAILABLE(macosx(10.13.4),
ios(11.3));

You always want this for new API:
WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

> Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageDatabaseTracker.mm:60
> +    configuration.get().websiteDataStore = [WKWebsiteDataStore
defaultDataStore];

Isn't this the default? If so, do we even need to pass in a configuration?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageDatabaseTracker.mm:62
> +    webView.get().UIDelegate = [[LocalStorageUIDelegate alloc] init];

Isn't this leaking? We may want to store [[LocalStorageUIDelegate alloc] init]
in a local RetainPtr<>, then then assign localStorageUIDelegate.get() here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageDatabaseTracker.mm:67
> +    TestWebKitAPI::Util::sleep(1);

Why is this needed? Are we sure this is not going to be flaky?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageDatabaseTracker.mm:75
> +	   EXPECT_TRUE([[origins objectAtIndex:0]
isEqual:@"http://localhost"]);

Is there a way to make EXPECT_STREQ() work here? It would make the output more
useful when failing.