| Summary: | Remove grandfathering from ResourceLoadStatistics | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||
| Status: | NEW --- | ||||||||||
| Severity: | Normal | CC: | ahmad.saleem792, cdumez, ews-watchlist, japhet, sam, wilander | ||||||||
| Priority: | P2 | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Alex Christensen
2020-04-18 16:11:12 PDT
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! |