Bug 217063

Created attachment 409913 [details] Patch

Created attachment 409922 [details] Patch

Created attachment 409923 [details] Patch

Comment on attachment 409923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409923&action=review Looks ok to me. Thanks for doing this Alex. > Source/WTF/ChangeLog:3 > + Remove plist-based ResourceLoadStatistics storage, which has been replaced by database-based storage yay! > Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:81 > TEST(ResourceLoadStatistics, GrandfatherCallbackDatabase) You could remove the *Database from the names of these API tests.

Comment on attachment 409923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409923&action=review Kate, do you know what happens if a user has an old plist format? How well do we handle that? There was special migration code between model version numbers. I'd like to wait for Kate's response to my comments, especially the failing LocalStorage + IDB test case. > LayoutTests/http/tests/resourceLoadStatistics/aggregate-sorted-data-no-storage-access-expected.txt:79 > + {{ Has been granted storage access under topframe1: 0; Has been seen under topframe1 in the last 24 hours: 1 },{ Has been granted storage access under topframe4: 0; Has been seen under topframe4 in the last 24 hours: 1 },{ Has been granted storage access under topframe3: 0; Has been seen under topframe3 in the last 24 hours: 1 },{ Has been granted storage access under topframe2: 0; Has been seen under topframe2 in the last 24 hours: 1 },} It's worrisome that these differ. Kate, do you happen to know why? Is it the spareness of the plist data that made it be 0? > LayoutTests/http/tests/resourceLoadStatistics/aggregate-sorted-data-no-storage-access-expected.txt:81 > + {{ Has been granted storage access under topframe1: 0; Has been seen under topframe1 in the last 24 hours: 1 },{ Has been granted storage access under topframe2: 0; Has been seen under topframe2 in the last 24 hours: 1 },} Ditto. > LayoutTests/http/tests/resourceLoadStatistics/aggregate-sorted-data-no-storage-access-expected.txt:83 > + {{ Has been granted storage access under topframe1: 0; Has been seen under topframe1 in the last 24 hours: 1 },} Ditto. > LayoutTests/http/tests/resourceLoadStatistics/dont-count-third-party-image-as-third-party-script-expected.txt:14 > + dataRecordsRemoved: 0 I wonder if this is also an effect of sparse data in the plist case? > LayoutTests/http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction-expected.txt:12 > +After statistics processing: IDB entry does exist. This seems wrong. Kate, to me this looks like the DB version not working. > LayoutTests/http/tests/resourceLoadStatistics/telemetry-generation-expected.txt:9 > +FAIL testResult.top3SubframeUnderTopFrameOrigins should be 0 (of type number). Was undefined (of type undefined). This seems wrong. Telemetry doesn't matter though since I believe we're removing it too. > LayoutTests/http/tests/resourceLoadStatistics/telemetry-generation-expected.txt:13 > +FAIL testResult.top3SubframeUnderTopFrameOrigins should be 4 (of type number). Was undefined (of type undefined). Ditto. > LayoutTests/http/tests/resourceLoadStatistics/telemetry-generation-expected.txt:15 > +Some tests failed. Ditto.

Comment on attachment 409923 [details] Patch We should understand the differences, but this patch does not change the behavior. It just brings our attention to existing things we should understand that are maybe bugs we should fix in separate patches. The diffs in the expectations are just from effectively copying the database expectations to what is now the only expectations.

Comment on attachment 409923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409923&action=review The only thing I think should be changed is http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction-database-expected.txt that John noticed, I filed a radar to look into that. Everything else is expected changes because there are differences between the plist and database. In response to John's comment -- the database takes whatever data is in the memory store and migrates it into the database. So as long as the memory store handles various plist versions, that won't change with this patch. >> LayoutTests/http/tests/resourceLoadStatistics/aggregate-sorted-data-no-storage-access-expected.txt:79 >> + {{ Has been granted storage access under topframe1: 0; Has been seen under topframe1 in the last 24 hours: 1 },{ Has been granted storage access under topframe4: 0; Has been seen under topframe4 in the last 24 hours: 1 },{ Has been granted storage access under topframe3: 0; Has been seen under topframe3 in the last 24 hours: 1 },{ Has been granted storage access under topframe2: 0; Has been seen under topframe2 in the last 24 hours: 1 },} > > It's worrisome that these differ. Kate, do you happen to know why? Is it the spareness of the plist data that made it be 0? The plist does not support sorting/aggregation of ITP data for the privacy report, so this change makes sense. >> LayoutTests/http/tests/resourceLoadStatistics/aggregate-sorted-data-no-storage-access-expected.txt:81 >> + {{ Has been granted storage access under topframe1: 0; Has been seen under topframe1 in the last 24 hours: 1 },{ Has been granted storage access under topframe2: 0; Has been seen under topframe2 in the last 24 hours: 1 },} > > Ditto. Again, this is fine. >> LayoutTests/http/tests/resourceLoadStatistics/aggregate-sorted-data-no-storage-access-expected.txt:83 >> + {{ Has been granted storage access under topframe1: 0; Has been seen under topframe1 in the last 24 hours: 1 },} > > Ditto. Same, this is ok. >> LayoutTests/http/tests/resourceLoadStatistics/dont-count-third-party-image-as-third-party-script-expected.txt:14 >> + dataRecordsRemoved: 0 > > I wonder if this is also an effect of sparse data in the plist case? I don't recall exactly why this differs, but since 127.0.0.1 appears as a top frame domain in localhost's SubresourceUnderTopFrameDomains data, it seems correct that it appears in this output. Probably a plist bug in the toString() code (the database has its own dump function). >> LayoutTests/http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction-expected.txt:12 >> +After statistics processing: IDB entry does exist. > > This seems wrong. Kate, to me this looks like the DB version not working. Agreed, this does not seem correct, and does not match the expectations currently in http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction-database-expected.txt. I think this file should not be rebased. I filed a radar to look into the test rdar://69748720. Good catch John. >> LayoutTests/http/tests/resourceLoadStatistics/telemetry-generation-expected.txt:9 >> +FAIL testResult.top3SubframeUnderTopFrameOrigins should be 0 (of type number). Was undefined (of type undefined). > > This seems wrong. Telemetry doesn't matter though since I believe we're removing it too. I think some of this is related to the changes made in https://bugs.webkit.org/show_bug.cgi?id=215514. Regardless I think we should address this in another patch (aka remove telemetry and these tests - rdar://problem/69748988).

(In reply to katherine_cheney from comment #7) > Comment on attachment 409923 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409923&action=review > > The only thing I think should be changed is > http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed- > statistics-with-no-user-interaction-database-expected.txt that John noticed, > I filed a radar to look into that. Everything else is expected changes > because there are differences between the plist and database. > > In response to John's comment -- the database takes whatever data is in the > memory store and migrates it into the database. So as long as the memory > store handles various plist versions, that won't change with this patch. > > >> LayoutTests/http/tests/resourceLoadStatistics/aggregate-sorted-data-no-storage-access-expected.txt:79 > >> + {{ Has been granted storage access under topframe1: 0; Has been seen under topframe1 in the last 24 hours: 1 },{ Has been granted storage access under topframe4: 0; Has been seen under topframe4 in the last 24 hours: 1 },{ Has been granted storage access under topframe3: 0; Has been seen under topframe3 in the last 24 hours: 1 },{ Has been granted storage access under topframe2: 0; Has been seen under topframe2 in the last 24 hours: 1 },} > > > > It's worrisome that these differ. Kate, do you happen to know why? Is it the spareness of the plist data that made it be 0? > > The plist does not support sorting/aggregation of ITP data for the privacy > report, so this change makes sense. > > >> LayoutTests/http/tests/resourceLoadStatistics/aggregate-sorted-data-no-storage-access-expected.txt:81 > >> + {{ Has been granted storage access under topframe1: 0; Has been seen under topframe1 in the last 24 hours: 1 },{ Has been granted storage access under topframe2: 0; Has been seen under topframe2 in the last 24 hours: 1 },} > > > > Ditto. > > Again, this is fine. > > >> LayoutTests/http/tests/resourceLoadStatistics/aggregate-sorted-data-no-storage-access-expected.txt:83 > >> + {{ Has been granted storage access under topframe1: 0; Has been seen under topframe1 in the last 24 hours: 1 },} > > > > Ditto. > > Same, this is ok. > > >> LayoutTests/http/tests/resourceLoadStatistics/dont-count-third-party-image-as-third-party-script-expected.txt:14 > >> + dataRecordsRemoved: 0 > > > > I wonder if this is also an effect of sparse data in the plist case? > > I don't recall exactly why this differs, but since 127.0.0.1 appears as a > top frame domain in localhost's SubresourceUnderTopFrameDomains data, it > seems correct that it appears in this output. Probably a plist bug in the > toString() code (the database has its own dump function). > > >> LayoutTests/http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction-expected.txt:12 > >> +After statistics processing: IDB entry does exist. > > > > This seems wrong. Kate, to me this looks like the DB version not working. > > Agreed, this does not seem correct, and does not match the expectations > currently in > http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed- > statistics-with-no-user-interaction-database-expected.txt. I think this file > should not be rebased. I filed a radar to look into the test > rdar://69748720. Good catch John. Thanks, Kate! Alex, please don't rebase this test expectation. Instead make the test as [ Pass Fail ] (which I believe is possible) with a comment that rdar://69748720 is tracking an investigation. Marking r+ with that comment. > >> LayoutTests/http/tests/resourceLoadStatistics/telemetry-generation-expected.txt:9 > >> +FAIL testResult.top3SubframeUnderTopFrameOrigins should be 0 (of type number). Was undefined (of type undefined). > > > > This seems wrong. Telemetry doesn't matter though since I believe we're removing it too. > > I think some of this is related to the changes made in > https://bugs.webkit.org/show_bug.cgi?id=215514. Regardless I think we should > address this in another patch (aka remove telemetry and these tests - > rdar://problem/69748988).

Comment on attachment 409923 [details] Patch See comment above about test case expectation.

<rdar://problem/69753755>