[webkit-reviews] review denied: [Bug 195088] Implement Telemetry and Dumping Routines for SQLite backend : [Attachment 377926] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Katherine_cheney at apple.com's request for
review:
Bug 195088: Implement Telemetry and Dumping Routines for SQLite backend
https://bugs.webkit.org/show_bug.cgi?id=195088

Attachment 377926: Patch

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




--- Comment #33 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 377926
  --> https://bugs.webkit.org/attachment.cgi?id=377926
Patch

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

Getting real close.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:516
> +static const String joinSubStatisticsForSorting()

This should return either a StringView or const char*.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:566
> +    auto[queryStart, queryEnd] = buildQueryStartAndEnd(statistic);

space after auto / before the [.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:609
> +static Optional<unsigned>
getMedianOfPrevalentResourcesWithUserInteraction(SQLiteDatabase& database,
unsigned prevalentResourcesWithUserInteractionCount)
> +{
> +    SQLiteStatement medianDaysSinceUIStatement =
makeMedianWithUIQuery(database);
> +    unsigned median;
> +
> +    if (medianDaysSinceUIStatement.prepare() == SQLITE_OK) {
> +	   if (medianDaysSinceUIStatement.bindInt(1, 1) != SQLITE_OK
> +	       || medianDaysSinceUIStatement.bindInt(2,
(prevalentResourcesWithUserInteractionCount / 2) != SQLITE_OK)) {
> +	       RELEASE_LOG_ERROR(Network,
"ResourceLoadStatisticsDatabaseStore::getMedianOfPrevalentResourcesWithUserInte
raction, error message: %{public}s", database.lastErrorMsg());
> +	       ASSERT_NOT_REACHED();
> +	       return WTF::nullopt;
> +	   }
> +	   if (medianDaysSinceUIStatement.step() == SQLITE_ROW) {
> +	       double rawSeconds =
medianDaysSinceUIStatement.getColumnDouble(0);
> +	       WallTime wallTime = WallTime::fromRawSeconds(rawSeconds);
> +	       median = wallTime <= WallTime() ? 0 :
std::floor((WallTime::now() - wallTime) / 24_h);
> +	   }
> +    }
> +
> +    if (prevalentResourcesWithUserInteractionCount & 1)
> +	   return median;
> +
> +    SQLiteStatement lowerMedianDaysSinceUIStatement =
makeMedianWithUIQuery(database);
> +    if (lowerMedianDaysSinceUIStatement.prepare() == SQLITE_OK) {
> +	   if (lowerMedianDaysSinceUIStatement.bindInt(1, 1) != SQLITE_OK
> +	       || lowerMedianDaysSinceUIStatement.bindInt(2,
((prevalentResourcesWithUserInteractionCount - 1) / 2)) != SQLITE_OK) {
> +	       RELEASE_LOG_ERROR(Network,
"ResourceLoadStatisticsDatabaseStore::getMedianOfPrevalentResourcesWithUserInte
raction, error message: %{public}s", database.lastErrorMsg());
> +	       ASSERT_NOT_REACHED();
> +	       return WTF::nullopt;
> +	   }
> +	   if (lowerMedianDaysSinceUIStatement.step() == SQLITE_ROW) {
> +	       double rawSecondsLower =
lowerMedianDaysSinceUIStatement.getColumnDouble(0);
> +	       WallTime wallTimeLower =
WallTime::fromRawSeconds(rawSecondsLower);
> +	       return ((wallTimeLower <= WallTime() ? 0 :
std::floor((WallTime::now() - wallTimeLower) / 24_h)) + median) / 2;
> +	   }
> +    }
> +
> +    return WTF::nullopt;
> +}

I think this would read cleaner and avoid the uninitialized unsigned would be
something like this:

static Optional<unsigned>
getMedianOfPrevalentResourcesWithUserInteraction(SQLiteDatabase& database,
unsigned prevalentResourcesWithUserInteractionCount)
{
    SQLiteStatement medianDaysSinceUIStatement =
makeMedianWithUIQuery(database);

    // Prepare
    if (medianDaysSinceUIStatement.prepare() != SQLITE_OK) {
	RELEASE_LOG_ERROR(Network,
"ResourceLoadStatisticsDatabaseStore::getMedianOfPrevalentResourcesWithUserInte
raction, error message: %{public}s", database.lastErrorMsg());
	ASSERT_NOT_REACHED();
	return WTF::nullopt;
    }

    // Bind
    if (medianDaysSinceUIStatement.bindInt(1, 1) != SQLITE_OK ||
medianDaysSinceUIStatement.bindInt(2,
(prevalentResourcesWithUserInteractionCount / 2) != SQLITE_OK)) {
	RELEASE_LOG_ERROR(Network,
"ResourceLoadStatisticsDatabaseStore::getMedianOfPrevalentResourcesWithUserInte
raction, error message: %{public}s", database.lastErrorMsg());
	ASSERT_NOT_REACHED();
	return WTF::nullopt;
    }

    // Step
    if (medianDaysSinceUIStatement.step() != SQLITE_ROW) {
	RELEASE_LOG_ERROR(Network,
"ResourceLoadStatisticsDatabaseStore::getMedianOfPrevalentResourcesWithUserInte
raction, error message: %{public}s", database.lastErrorMsg());
	ASSERT_NOT_REACHED();
	return WTF::nullopt;
    }

    double rawSeconds = medianDaysSinceUIStatement.getColumnDouble(0);
    WallTime wallTime = WallTime::fromRawSeconds(rawSeconds);
    unsigned median = wallTime <= WallTime() ? 0 : std::floor((WallTime::now()
- wallTime) / 24_h);

    if (prevalentResourcesWithUserInteractionCount & 1)
	return median;

    SQLiteStatement lowerMedianDaysSinceUIStatement =
makeMedianWithUIQuery(database);

    // Prepare
    if (lowerMedianDaysSinceUIStatement.prepare() != SQLITE_OK) {
	RELEASE_LOG_ERROR(Network,
"ResourceLoadStatisticsDatabaseStore::getMedianOfPrevalentResourcesWithUserInte
raction, error message: %{public}s", database.lastErrorMsg());
	ASSERT_NOT_REACHED();
	return WTF::nullopt;
    }

    // Bind
    if (lowerMedianDaysSinceUIStatement.bindInt(1, 1) != SQLITE_OK ||
lowerMedianDaysSinceUIStatement.bindInt(2,
((prevalentResourcesWithUserInteractionCount - 1) / 2)) != SQLITE_OK) {
	RELEASE_LOG_ERROR(Network,
"ResourceLoadStatisticsDatabaseStore::getMedianOfPrevalentResourcesWithUserInte
raction, error message: %{public}s", database.lastErrorMsg());
	ASSERT_NOT_REACHED();
	return WTF::nullopt;
    }

    // Step
    if (lowerMedianDaysSinceUIStatement.step() != SQLITE_ROW) {
	RELEASE_LOG_ERROR(Network,
"ResourceLoadStatisticsDatabaseStore::getMedianOfPrevalentResourcesWithUserInte
raction, error message: %{public}s", database.lastErrorMsg());
	ASSERT_NOT_REACHED();
	return WTF::nullopt;
    }

    double rawSecondsLower =
lowerMedianDaysSinceUIStatement.getColumnDouble(0);
    WallTime wallTimeLower = WallTime::fromRawSeconds(rawSecondsLower);
    return ((wallTimeLower <= WallTime() ? 0 : std::floor((WallTime::now() -
wallTimeLower) / 24_h)) + median) / 2;
}

What do you think? You would want to use the same pattern in
getTopPrevelentResourceDaysSinceUI and
getMedianOfPrevalentResourceWithoutUserInteraction.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:642
> +static Optional<unsigned>
getMedianOfPrevalentResourceWithoutUserInteraction(SQLiteDatabase& database,
int range, PrevalentResourceDatabaseTelemetry::Statistic statistic, unsigned
numberOfPrevalentResourcesWithoutUI)

I think range and median below should both be unsigned.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:648
> +    SQLiteStatement Statistic = makeMedianWithoutUIQuery(database,
statistic);

Statistic should start with a lowercase s.

>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsTelemetry.cpp:
268
> +static String makeDescription(PrevalentResourceDatabaseTelemetry::Statistic
statistic)

This should return either a StringView or const char*.