[webkit-reviews] review granted: [Bug 205121] Add timeStamp to ITP database : [Attachment 385925] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted katherine_cheney at apple.com's
request for review:
Bug 205121: Add timeStamp to ITP database
https://bugs.webkit.org/show_bug.cgi?id=205121

Attachment 385925: Patch

https://bugs.webkit.org/attachment.cgi?id=385925&action=review




--- Comment #12 from John Wilander <wilander at apple.com> ---
Comment on attachment 385925
  --> https://bugs.webkit.org/attachment.cgi?id=385925
Patch

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

I think we're good here. Please see my comments and questions before landing.

> Source/WebKit/ChangeLog:13
> +	   _getResourceLoadStatisticsDataSummaryAPI.

Missing space before API.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:664
> +	   if (insertRelationshipStatement.bindDouble(2,
WallTime::now().secondsSinceEpoch().value()) != SQLITE_OK) {

Just double checking:
1) Does this cover all the types of data ITP collects? Subresource, sub frame,
web sockets (which map to subresource)?
2) Is this timestamp double-keyed so that it's for the third-party under a
specific first-party?

I was expecting a timestamp capture where we capture the loads but you can
absolutely do it at the sink too.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1859
> +    double mostRecentlyUpdatedTimestamp =
m_getMostRecentlyUpdatedTimestampStatement.getColumnDouble(0);

Much clearer variable name.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:1
08
> +    WebResourceLoadStatisticsStore::ThirdPartyDataForSpecificFirstParty
thirdPartyDataForSpecificFirstParty { firstPartyDomain,
thirdPartyHasStorageAccess, Seconds { NO_EXISTING_TIMESTAMP }};

Thanks for introducing a constant. See my comment on what its definition
though.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:42
> +#define NO_EXISTING_TIMESTAMP -1

I think this should instead be …

static constexpr Seconds NoExistingTimestamp = { -1 };

… in the ResourceLoadStatistics namespace and then used as
ResourceLoadStatistics::NoExistingTimestamp. Note that you then don't have to
instantiate new Seconds objects in all those places.

See for instance LoggedInStatus::TimeToLiveShort and WatchDog::noTimeLimit.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:140
> +	       return WTF::nullopt;

Will this lead to us dumping all the existing data? Because it won't have these
timestamps. In the case of ResourceLoadStatistics.cpp, there's a modelVersion
to check for and a graceful import of data when the source is less than the new
model.

> Tools/ChangeLog:10
> +	   in the Resource Load Statistics database backend.

You're mixing Resource Load Statistics and ITP in this entry. Let's stick to
one.

> Tools/ChangeLog:11
> +	   This also adds an api test case using the ITP database store.

api -> API

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:901
> +    // Teach ITP about bad origins:

Change colon to period.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:923
> +    // capture time for comparison later

Capital c and period.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:926
> +    

Remove double linefeed.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:942
> +

Remove double linefeed.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1020
> +    // it appears under or redirects to

Period.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1049
> +	   // check timestamp for evil3 is reported as being within the correct
range

Start with capital C and end with a period.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1083
> +	   // check timestamp for evil1 is reported as being within the correct
range

Start with capital C and end with a period.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1108
> +	   // check timestamp for evil2 is reported as being within the correct
range

Start with capital C and end with a period.