[webkit-reviews] review denied: [Bug 185041] [WKHTTPCookieStore getAllCookies] returns inconsistent creation time : [Attachment 339212] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 185041: [WKHTTPCookieStore getAllCookies] returns inconsistent creation
time
https://bugs.webkit.org/show_bug.cgi?id=185041

Attachment 339212: Patch

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




--- Comment #6 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 339212
  --> https://bugs.webkit.org/attachment.cgi?id=339212
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:348
> +	   *cookiesPtr = [nsCookies retain];

RetainPtr is a smart pointer <https://en.wikipedia.org/wiki/Smart_pointer> that
automatically retains and releases the pointer it holds. The act of assignment
is an implicit retain of the new value and release of the old value, so please
remove this explicit retain, which is an extra retain, and therefore a memory
leak.

Also, you can just lambda capture &cookies and use "cookies" by name, rather
than capturing cookiesPtr = &cookies. I think that's simpler.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:356
> +    sleep(1_s);

What is the resolution of creation time? If it's milliseconds, then you can
reduce this sleep to something like 10ms.

It's good to make tests as fast as possible because we have tens of thousands
of them.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:359
> +	   *cookiesPtr = [nsCookies retain];

Ditto.