[webkit-reviews] review granted: [Bug 171295] Resource Load Statistics: Introduce shorter time-to-live for cookie partition whitelisting : [Attachment 308162] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 171295: Resource Load Statistics: Introduce shorter time-to-live for cookie
partition whitelisting
https://bugs.webkit.org/show_bug.cgi?id=171295

Attachment 308162: Patch

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




--- Comment #3 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 308162
  --> https://bugs.webkit.org/attachment.cgi?id=308162
Patch

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

Looks good. Please double-check the mix of doubles and integer math is doing
what you expect, and consider using a method to hide the complexity of that one
predicate. Otherwise r=me.

> Source/WebCore/loader/ResourceLoadObserver.cpp:52
> +static auto timestampResolution = 3600;

I think this gives an integer value...

> Source/WebCore/loader/ResourceLoadObserver.cpp:306
>      return std::floor(seconds / timestampResolution) * timestampResolution;

... so doesn't this do integer division? Maybe the floor doesn't do anything
here?

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:192
> +	       || currentTime() > resourceStatistic.mostRecentUserInteraction +
timeToLiveCookiePartitionFree);

I think this would read a little more cleanly as a local static method:

static bool shouldPartitionCookie(...)