[webkit-reviews] review granted: [Bug 189933] Cap lifetime of persistent cookies created client-side through document.cookie : [Attachment 350709] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted John Wilander <wilander at apple.com>'s
request for review:
Bug 189933: Cap lifetime of persistent cookies created client-side through
document.cookie
https://bugs.webkit.org/show_bug.cgi?id=189933

Attachment 350709: Patch

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




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

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

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:284
> +	   if (![cookie isSessionOnly]) {

Don't we need to update the CFNet implementation as well?

> Source/WebCore/testing/Internals.cpp:4769
> +	   return { { } };

Why not return { };

> Source/WebCore/testing/Internals.cpp:4773
> +    auto cookieData = WTF::map(cookies, [](auto& cookie) {

you can return the result of WTF::map() right away. We do not really need this
local variable.

> Source/WebCore/testing/Internals.h:765
> +	   CookieData(Cookie cookie)

Maybe a blank line before this?

> Source/WebCore/testing/Internals.h:778
> +	   CookieData()

Do we really need this? With the return { }; I suggested earlier, this may no
longer be needed.

> LayoutTests/ChangeLog:11
> +	   1) Cookies are available to JavaScript by default via
document.cookie, which

You do not need to repeat the whole changelog in the Layout tests one. This
changelog should merely be something like "Added layout test coverage.".

> LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:39
> +    const overTwoDaysInSeconds = twoDaysInSeconds + 10;

Can 10 lead to flakiness on slow bots? Why not 30? (I think this is the default
timeout value).

> LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:40
> +    const overOneWeekInSeconds = oneWeekInSeconds + 10;

ditto

> LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:43
> +	   if (!cookies)

if (!cookies.length) no?