Bug 171295 – Resource Load Statistics: Introduce shorter time-to-live for cookie partition whitelisting

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

Introduce shorter time-to-live for cookie partition whitelisting.

[Radar WebKit Bug Importer]

<rdar://problem/31823818>

[John Wilander]

Created attachment 308162 [details]
Patch

[Brent Fulgham]

Comment on attachment 308162 [details]
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(...)

[John Wilander]

Created attachment 308177 [details]
Patch for landing

[John Wilander]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 308162 [details]
> 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?

I tested it and the call to floor() does make a difference:
Without floor 1493167842.243571
With floor 1493164800.000000

> > 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(...)

Agreed and fixed.

Thanks for the review, Brent!

[WebKit Commit Bot]

Comment on attachment 308177 [details]
Patch for landing

Clearing flags on attachment: 308177

Committed r215782: <http://trac.webkit.org/changeset/215782>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.