Created attachment 396869 [details]
Patch

Comment on attachment 396869 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadStatistics.cpp:88
> +    encoder.encodeBool("grandfathered"_s, false);

Probably worth adding a comment explaining why this is here.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:314
> +    bool unused;
> +    if (!decoder.decodeBool("grandfathered", unused))
>          return false;

Same here.

Comment on attachment 396869 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Grandfathering was needed for the initial release.
> +        Now it's just a maintenance burden that doesn't do anything.

Would be good to explain what Grandfathering in this context was and why it can be removed now here.

Created attachment 397015 [details]
Patch

Created attachment 397023 [details]
Patch

Sorry, I was out for a few days. Unfortunately, I don’t think we can get rid of grandfathering yet. The reason is that time limited history deletion is not yet supported. So when a user deletes history for one hour or one day, we delete all ITP data. In those cases, there will be lots of website data left and grandfathering kicks in to retain that data until ITP has had a chance to again learn which websites the user interacts with.

Support for time limited deletion is tricky. We’d have to keep a series of time stamps for every ITP data entry so that we can effectively roll ITP back in time one hour, one day, two days, or one week.

What we could do if we want to get rid of grandfathering (which I agree is a pain) is to keep a series of time stamps only for user interaction. That is the only piece of ITP data needed to be able to retain the right website data after a partial history deletion.

Checking via BugID, it seems this r+ patch didn't landed and also checking via code. Example:

https://github.com/WebKit/WebKit/blob/11f7471c70b0d9aff5c0bfd95d5b4175e5bf2f2d/Source/WebCore/loader/ResourceLoadStatistics.cpp#L98

Do we need this? Thanks!