Introduce shorter time-to-live for cookie partition whitelisting.
<rdar://problem/31823818>
Created attachment 308162 [details] Patch
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(...)
Created attachment 308177 [details] Patch for landing
(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!
Comment on attachment 308177 [details] Patch for landing Clearing flags on attachment: 308177 Committed r215782: <http://trac.webkit.org/changeset/215782>
All reviewed patches have been landed. Closing bug.