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

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham 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 377597: Patch

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




--- Comment #3 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 377597
  --> https://bugs.webkit.org/attachment.cgi?id=377597
Patch

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

I think this looks good overall, but could be made better by factoring out some
of the common SQL logic into 'make' functions that would allow for easier
maintenance in the future. r- to fix that.

> Source/WebKit/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=195088

Please always include the radar number here underneath the Bugzilla line:
<rdar://problem/54213407>

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:518
> +    if ((prevalentResourcesWithUserInteractionCount & 1)) {

What is the significance of the bitwise & here? Are you checking for an odd
number?

If this is just a check for non-zero we would typically write this as "if
(prevalentResourcesWIthUserInteractionCount)"

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:531
> +	   SQLiteStatement upperMedianDaysSinceUIStatement(m_database,
makeString("SELECT mostRecentUserInteractionTime FROM ObservedDomains INNER
JOIN (SELECT ", pre, " AND hadUserInteraction = 1 ", post, ") as q ON
ObservedDomains.domainID = q.domainID LIMIT 1 OFFSET ",
String::number((prevalentResourcesWithUserInteractionCount+1) / 2)));

The three SQL statements in this section are almost identical. I suggest you
write a generator function that returns the appropriate SQLLiteStatement based
on inputs:

static SQLiteStatement makeMedianQuery(SQLDatabase& database, int count, const
String& pre, const String& post) {
    return SQLiteStatement(database, makeString("SELECT
mostRecentUserInteractionTime FROM ObservedDomains INNER JOIN (SELECT ", pre, "
AND hadUserInteraction = 1 ", post, ") as q ON ObservedDomains.domainID =
q.domainID LIMIT 1 OFFSET ", String::number(count)));
}

Then you can just say:

SQLiteStatement medianDaysSinceUIStatement =
makeMedianQuery(prevalentResourcesWithUserInteractionCount / 2, pre, post);

and

SQLiteStatement lowerMedianDaysSinceUIStatement =
makeMedianQuery((prevalentResourcesWithUserInteractionCount - 1) / 2 pre,
post);

and

SQLiteStatement upperMedianDaysSinceUIStatement =
makeMedianQuery((prevalentResourcesWithUserInteractionCount + 1) / 2 pre,
post);

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:538
> +		   double rawSecondsUpper =
upperMedianDaysSinceUIStatement.getColumnDouble(0);

Very nice!

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:624
> +	   SQLiteStatement upperMedianStatistic(m_database,
makeString(preamble, pre, " AND hadUserInteraction = 0 ", post, end,
String::number((range+1) / 2)));

Again, I think these three median statements could be converted into. a 'make'
function.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:680
> +    data.medianDaysSinceUserInteractionPrevalentResourceWithUserInteraction
=
getMedianOfPrevalentResourcesWithUserInteraction(data.numberOfPrevalentResource
sWithUserInteraction, joinPrevalentResourcesQueryPre,
joinPrevalentResourcesQueryPost);

Someday we need to rename these to shorter labels.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:-508
> -    // FIXME(195088): Implement for Database version.

Great!