Currently in the plist and database, -1 is used to indicate no user interaction. We should handle this in ResourceLoadStatistics when considering minimum user interaction time.
<rdar://problem/63696470>
Created attachment 400406 [details] Patch
Comment on attachment 400406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400406&action=review > Source/WebKit/ChangeLog:10 > + http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction.html Should we have a test for the case where cookies are removed, too? This seems to be testing the 7-day non-cookie data removal, but what about the 30-day cookie removal? > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2542 > +Optional<WallTime> ResourceLoadStatisticsDatabaseStore::mostRecentUserInteractionTime(DomainData& statistic) Could this be passed as a const DomainData&? > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2572 > + if (mostRecentUserInteractionTime) I think this could just be: if (auto mostRecentInteractionTime = this->mostRecentInteractionTime(statistic)) > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596 > + if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) How long has that been wrong! Yikes. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:971 > +Optional<WallTime> ResourceLoadStatisticsMemoryStore::mostRecentUserInteractionTime(ResourceLoadStatistics& statistic) Seems like this could be "const ResourceLoadStatistics&" > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:1021 > + if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) :-(
Comment on attachment 400406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400406&action=review Looks good! See comment on change log. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544 > + if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0) Good that you include 0 here. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596 > + if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) 😳 > LayoutTests/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). Please add a comment on that the case of data deletion are already tested and that these new tests only need to check for the absence of deletion. Also add a comment on which test case(s) started failing once you corrected the issue, if any.
(In reply to Brent Fulgham from comment #3) > Comment on attachment 400406 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400406&action=review > > > Source/WebKit/ChangeLog:10 > > + http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction.html > > Should we have a test for the case where cookies are removed, too? > > This seems to be testing the 7-day non-cookie data removal, but what about > the 30-day cookie removal? > Since the flawed logic only affects non-cookie website data, the 30 day all website data removal case is covered by operating-dates-all-website-data-removed.html and operating-dates-all-website-data-removed-database.html. Good thought though, I can explain this in the ChangeLog. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2542 > > +Optional<WallTime> ResourceLoadStatisticsDatabaseStore::mostRecentUserInteractionTime(DomainData& statistic) > > Could this be passed as a const DomainData&? > Yes, good catch. I'll change that. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2572 > > + if (mostRecentUserInteractionTime) > > I think this could just be: > > if (auto mostRecentInteractionTime = > this->mostRecentInteractionTime(statistic)) > Nice, will change. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596 > > + if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) > > How long has that been wrong! Yikes. :( > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:971 > > +Optional<WallTime> ResourceLoadStatisticsMemoryStore::mostRecentUserInteractionTime(ResourceLoadStatistics& statistic) > > Seems like this could be "const ResourceLoadStatistics&" Yes, good catch. I'll change that. > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:1021 > > + if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) > > :-(
(In reply to John Wilander from comment #4) > Comment on attachment 400406 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400406&action=review > > Looks good! See comment on change log. > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544 > > + if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0) > > Good that you include 0 here. > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596 > > + if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) > > 😳 > > > LayoutTests/ChangeLog:7 > > + Reviewed by NOBODY (OOPS!). > > Please add a comment on that the case of data deletion are already tested > and that these new tests only need to check for the absence of deletion. > Also add a comment on which test case(s) started failing once you corrected > the issue, if any. Yes, doing this I realized these new tests can replace the old 7-day deletion tests because they cover that test case already. Patch-for-landing will remove the old ones.
Created attachment 400508 [details] Patch for landing
Thanks for the feedback Brent and John!
(In reply to katherine_cheney from comment #6) > (In reply to John Wilander from comment #4) > > Comment on attachment 400406 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=400406&action=review > > > > Looks good! See comment on change log. > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544 > > > + if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0) > > > > Good that you include 0 here. > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596 > > > + if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) > > > > 😳 > > > > > LayoutTests/ChangeLog:7 > > > + Reviewed by NOBODY (OOPS!). > > > > Please add a comment on that the case of data deletion are already tested > > and that these new tests only need to check for the absence of deletion. > > Also add a comment on which test case(s) started failing once you corrected > > the issue, if any. > > Yes, doing this I realized these new tests can replace the old 7-day > deletion tests because they cover that test case already. Patch-for-landing > will remove the old ones. That was not what I meant. I thought the old tests check the case when data *should* be deleted. But maybe there are *also* previous tests that data was retained?
(In reply to John Wilander from comment #9) > (In reply to katherine_cheney from comment #6) > > (In reply to John Wilander from comment #4) > > > Comment on attachment 400406 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=400406&action=review > > > > > > Looks good! See comment on change log. > > > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544 > > > > + if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0) > > > > > > Good that you include 0 here. > > > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596 > > > > + if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) > > > > > > 😳 > > > > > > > LayoutTests/ChangeLog:7 > > > > + Reviewed by NOBODY (OOPS!). > > > > > > Please add a comment on that the case of data deletion are already tested > > > and that these new tests only need to check for the absence of deletion. > > > Also add a comment on which test case(s) started failing once you corrected > > > the issue, if any. > > > > Yes, doing this I realized these new tests can replace the old 7-day > > deletion tests because they cover that test case already. Patch-for-landing > > will remove the old ones. > > That was not what I meant. I thought the old tests check the case when data > *should* be deleted. But maybe there are *also* previous tests that data was > retained? I think I see what you're saying. This is just about the non-cookie website data deletion.
(In reply to John Wilander from comment #10) > (In reply to John Wilander from comment #9) > > (In reply to katherine_cheney from comment #6) > > > (In reply to John Wilander from comment #4) > > > > Comment on attachment 400406 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=400406&action=review > > > > > > > > Looks good! See comment on change log. > > > > > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544 > > > > > + if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0) > > > > > > > > Good that you include 0 here. > > > > > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596 > > > > > + if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) > > > > > > > > 😳 > > > > > > > > > LayoutTests/ChangeLog:7 > > > > > + Reviewed by NOBODY (OOPS!). > > > > > > > > Please add a comment on that the case of data deletion are already tested > > > > and that these new tests only need to check for the absence of deletion. > > > > Also add a comment on which test case(s) started failing once you corrected > > > > the issue, if any. > > > > > > Yes, doing this I realized these new tests can replace the old 7-day > > > deletion tests because they cover that test case already. Patch-for-landing > > > will remove the old ones. > > > > That was not what I meant. I thought the old tests check the case when data > > *should* be deleted. But maybe there are *also* previous tests that data was > > retained? > > I think I see what you're saying. This is just about the non-cookie website > data deletion. All tests check for proper data deletion for expired statistics. The new 7-day test checks for deletion when including a statistic with -1 as the mostRecentUserInteraction time and with parameters().isRunningTest disabled. This covered the other case, and it didn't make sense to keep the other 7-day data deletion test because it doesn't test actual behavior (because of the parameters().isRunningTest()).
(In reply to katherine_cheney from comment #11) > (In reply to John Wilander from comment #10) > > (In reply to John Wilander from comment #9) > > > (In reply to katherine_cheney from comment #6) > > > > (In reply to John Wilander from comment #4) > > > > > Comment on attachment 400406 [details] > > > > > Patch > > > > > > > > > > View in context: > > > > > https://bugs.webkit.org/attachment.cgi?id=400406&action=review > > > > > > > > > > Looks good! See comment on change log. > > > > > > > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544 > > > > > > + if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0) > > > > > > > > > > Good that you include 0 here. > > > > > > > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596 > > > > > > + if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval) > > > > > > > > > > 😳 > > > > > > > > > > > LayoutTests/ChangeLog:7 > > > > > > + Reviewed by NOBODY (OOPS!). > > > > > > > > > > Please add a comment on that the case of data deletion are already tested > > > > > and that these new tests only need to check for the absence of deletion. > > > > > Also add a comment on which test case(s) started failing once you corrected > > > > > the issue, if any. > > > > > > > > Yes, doing this I realized these new tests can replace the old 7-day > > > > deletion tests because they cover that test case already. Patch-for-landing > > > > will remove the old ones. > > > > > > That was not what I meant. I thought the old tests check the case when data > > > *should* be deleted. But maybe there are *also* previous tests that data was > > > retained? > > > > I think I see what you're saying. This is just about the non-cookie website > > data deletion. > > All tests check for proper data deletion for expired statistics. The new > 7-day test checks for deletion when including a statistic with -1 as the > mostRecentUserInteraction time and with parameters().isRunningTest disabled. > This covered the other case, and it didn't make sense to keep the other > 7-day data deletion test because it doesn't test actual behavior (because of > the parameters().isRunningTest()). In fact, the other test was giving us a false-positive, which is probably worse.
Committed r262265: <https://trac.webkit.org/changeset/262265> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400508 [details].