[webkit-reviews] review granted: [Bug 194867] Use a SQLite database to hold the ResourceLoadStatistics data : [Attachment 363513] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 194867: Use a SQLite database to hold the ResourceLoadStatistics data
https://bugs.webkit.org/show_bug.cgi?id=194867

Attachment 363513: Patch

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




--- Comment #14 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 363513
  --> https://bugs.webkit.org/attachment.cgi?id=363513
Patch

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

r=me with changes.

> Source/WebCore/page/RuntimeEnabledFeatures.h:152
> +    bool iTPDatabaseEnabled() const { return m_isITPDatabaseEnabled; }

typo: iTPDatabaseEnabled -> isTPDatabaseEnabled

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:145
> +    this->workQueue()->dispatch([this, path =
m_storageDirectoryPath.isolatedCopy()] {

this->workQueue()->dispatch() could be workQueue.dispatch(). But really, why
are we dispatching here? We are already on the background queue.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:155
> +	   if (!m_database.tableExists("ObservedDomains")) {

"ObservedDomains"_s

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:182
> +	   result = m_observedDomainCount.getColumnInt(0) ? false : true;

Personally, I think all these ternaries would look better like so:
result = !m_observedDomainCount.getColumnInt(0);

But your call.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:190
> +constexpr auto createObservedDomain = "CREATE TABLE ObservedDomains ("

Could we move those to the top of the file with the others?

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:329
> +{

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:363
> +    if (statement.bindInt(1, firstDomainID) != SQLITE_OK

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:371
> +    bool result = statement.getColumnInt(0) ? true : false;

I think it would look better like so: bool relationShipExists =
!!statement.getColumnInt(0);

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:381
> +    if (statement.bindInt(1, domainID) != SQLITE_OK

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:419
> +    unsigned domainID = 0;

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:438
> +    auto registrableDomainID = domainID(loadStatistics.registrableDomain);

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:463
> +    workQueue()->dispatch([this, weakMemoryStore = makeWeakPtr(memoryStore)]
{

Why are we dispatching? I believe this is already called on the background
queue since all methods of the stores are supposed to be always called on the
background queue. On a related note, we should assert we are on the right
queue.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:506
> +    SQLiteStatement domainsToUpdateStatement(m_database, makeString("UPDATE
ObservedDomains SET dataRecordsRemoved = dataRecordsRemoved + 1 WHERE
registrableDomain IN (", domainsToString(domains), ")"));

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:539
> +	       auto newDomain = findTopFrames.getColumnInt(0);

isNewDomain? or more likely newDomainID?

The name is confusing considering this is an int.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:594
> +    HashMap<unsigned, NotVeryPrevalentResources> results;

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:674
> +    ensurePrevalentResourcesForDebugMode();

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:680
> +{

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:685
> +{

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:822
> +{

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:833
> +    if (!debugModeEnabled())

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:942
> +    if (m_mostRecentUserInteractionStatement.bindInt(1, hadUserInteraction)

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1041
> +    SQLiteStatement domainsToUpdateStatement(m_database, makeString("UPDATE
ObservedDomains SET isPrevalent = 1 WHERE domainID IN (",
buildList(WTF::IteratorRange<StdSet<unsigned>::iterator>(domains.begin(),
domains.end())), ")"));

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1055
> +    ASSERT_NOT_REACHED(0);

ASSERT_NOT_REACHED(0); -> ASSERT_NOT_REACHED();

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1268
> +    SQLiteStatement statement(m_database, makeString("SELECT isPrevalent,
hadUserInteraction FROM ObservedDomains WHERE registrableDomain = ",
domain.string()));

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1284
> +    auto firstPartyPrimaryDomainID = domainID(firstPartyDomain);

ASSERT(!RunLoop::isMain());

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:7
9
> +    this->workQueue()->dispatchAfter(5_s, [weakThis = makeWeakPtr(*this)] {

workQueue.dispatchAfter() ?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:219
> +    Vector<OperatingDate>& operatingDates() { return m_operatingDates; }

Please add a proper setter instead.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:221
> +    WallTime& endOfGrandfatheringTimestamp() { return
m_endOfGrandfatheringTimestamp; }

Please add a proper setter instead: setEndOfGrandfatheringTimestamp(WallTime);