[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 bugzilla-daemon at webkit.org
Thu May 17 19:38:45 PDT 2018

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

Attachment 340672: Patch


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

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

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]

a WTF String can implicitly be converted into a NSString. I think this should
[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

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

You always want this for new API:

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

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]

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

More information about the webkit-reviews mailing list