Bug 208612

<rdar://problem/60060905>

Created attachment 392506 [details]
Patch

Comment on attachment 392506 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:86
> +constexpr auto insertResourceLoadCountRowQuery = "INSERT OR IGNORE INTO ResourceLoadCount VALUES(?, 0)"_s;

INSERT OR IGNORE calls might require creating a unique index. Maybe not though because ResourceLoadCount has a primary key. Did you test that repeat attempts don't create a new row?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:356
> +            if (strncmp(table, "ResourceLoadCount", strlen(table))) {

You can just use == here.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:359
> +            } else if (!createResourceLoadCountTable()) {

Hmm, this seems strange. createResourceLoadCountTable() will be called for every other table iteration besides ResourceLoadCount. It should probably only be called once.

I think this could be re-written in a way to  make it easier to add new tables in the future. Maybe maintain a list of missing tables, and add them at the end?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:602
> +

You can use resetStatement() here if you want. One less line of code.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:620
> +    ASSERT_UNUSED(resetResult, resetResult == SQLITE_OK);

ditto

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:645
> +

ditto

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1346
> +        buildList(WTF::IteratorRange<Vector<unsigned>::iterator>(julianDaysInRange.begin(), julianDaysInRange.end())), ") ORDER BY day"));

I think theres a static buildList function already in the class you can use.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2862
>      

Maybe I'm out of the loop, but what does "Transient stats" mean here?

Comment on attachment 392506 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1307
> +    postTask([this, numberOfDays = WTFMove(numberOfDays), completionHandler = WTFMove(completionHandler)]() mutable {

No need to WTFMove() an unsigned

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1317
> +            postTaskReply([resourceLoadCountPerDay = WTFMove(resourceLoadCountPerDay), completionHandler = WTFMove(completionHandler)]() mutable {

ditto

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1318
> +                completionHandler(WTFMove(resourceLoadCountPerDay));

ditto

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2756
> +    // Transient stats

WebKit comments need to end with a period.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2859
> +    // Transient stats

WebKit comments need to end with a period.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:552
> +    completionHandler();

Dont you need to pass a parameter here?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:887
> +    if (!canSendMessage()) {

Should not be needed.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1526
> +            process->getResourceLoadCountPerDayForPastDays(m_sessionID, numberOfDays, WTFMove(completionHandler));

use after move of completionHandler if there is more than one process pool with a network process.

(In reply to katherine_cheney from comment #3)
> Comment on attachment 392506 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392506&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:86
> > +constexpr auto insertResourceLoadCountRowQuery = "INSERT OR IGNORE INTO ResourceLoadCount VALUES(?, 0)"_s;
> 
> INSERT OR IGNORE calls might require creating a unique index. Maybe not
> though because ResourceLoadCount has a primary key. Did you test that repeat
> attempts don't create a new row?

The primary key acts as a unique index in this (ON CONFLICT IGNORE) case, so this doesn't create new rows.

> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:356
> > +            if (strncmp(table, "ResourceLoadCount", strlen(table))) {
> 
> You can just use == here.

table is a char*[], it doesn't like == 

> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:359
> > +            } else if (!createResourceLoadCountTable()) {
> 
> Hmm, this seems strange. createResourceLoadCountTable() will be called for
> every other table iteration besides ResourceLoadCount. It should probably
> only be called once.
> 
> I think this could be re-written in a way to  make it easier to add new
> tables in the future. Maybe maintain a list of missing tables, and add them
> at the end?
> 

Since the strncmp is effectively a != (rather than ==) this will only be reached when table is "ResourceLoadCount". I agree this doesn't lend well to adding new similar tables in the future, and reads poorly.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:602
> > +
> 
> You can use resetStatement() here if you want. One less line of code.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:620
> > +    ASSERT_UNUSED(resetResult, resetResult == SQLITE_OK);
> 
> ditto
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:645
> > +
> 
> ditto
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1346
> > +        buildList(WTF::IteratorRange<Vector<unsigned>::iterator>(julianDaysInRange.begin(), julianDaysInRange.end())), ") ORDER BY day"));
> 
> I think theres a static buildList function already in the class you can use.
> 

This uses the static buildList function

> > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2862
> >      
> 
> Maybe I'm out of the loop, but what does "Transient stats" mean here?

It was just a name I came up with, since the field frequently restarts from 0 and isn't persisted. I'll rename it to something more clear.

(In reply to Chris Dumez from comment #4)
> Comment on attachment 392506 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392506&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1307
> > +    postTask([this, numberOfDays = WTFMove(numberOfDays), completionHandler = WTFMove(completionHandler)]() mutable {
> 
> No need to WTFMove() an unsigned
> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1317
> > +            postTaskReply([resourceLoadCountPerDay = WTFMove(resourceLoadCountPerDay), completionHandler = WTFMove(completionHandler)]() mutable {
> 
> ditto

In this case it's a Vector<unsigned>

> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1318
> > +                completionHandler(WTFMove(resourceLoadCountPerDay));
> 
> ditto

Same here, Vector<unsigned>

> 
> > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2756
> > +    // Transient stats
> 
> WebKit comments need to end with a period.
> 
> > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:2859
> > +    // Transient stats
> 
> WebKit comments need to end with a period.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:552
> > +    completionHandler();
> 
> Dont you need to pass a parameter here?
> 
> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:887
> > +    if (!canSendMessage()) {
> 
> Should not be needed.

All adjacent methods do this. If it's not needed it should be a separate task to update them all IMO.

> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1526
> > +            process->getResourceLoadCountPerDayForPastDays(m_sessionID, numberOfDays, WTFMove(completionHandler));
> 
> use after move of completionHandler if there is more than one process pool
> with a network process.

Changing this to RELEASE_ASSERT(processPool().size() == 1)

Created attachment 392530 [details]
Patch

(In reply to mmokary from comment #5)
> (In reply to katherine_cheney from comment #3)
> > Comment on attachment 392506 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=392506&action=review
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:86
> > > +constexpr auto insertResourceLoadCountRowQuery = "INSERT OR IGNORE INTO ResourceLoadCount VALUES(?, 0)"_s;
> > 
> > INSERT OR IGNORE calls might require creating a unique index. Maybe not
> > though because ResourceLoadCount has a primary key. Did you test that repeat
> > attempts don't create a new row?
> 
> The primary key acts as a unique index in this (ON CONFLICT IGNORE) case, so
> this doesn't create new rows.
> 

Great! I assumed so because of the test case, but wanted to double check :) 

> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:356
> > > +            if (strncmp(table, "ResourceLoadCount", strlen(table))) {
> > 
> > You can just use == here.
> 
> table is a char*[], it doesn't like == 
> 
> > 

You can probably use String(table)

> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:359
> > > +            } else if (!createResourceLoadCountTable()) {
> > 
> > Hmm, this seems strange. createResourceLoadCountTable() will be called for
> > every other table iteration besides ResourceLoadCount. It should probably
> > only be called once.
> > 
> > I think this could be re-written in a way to  make it easier to add new
> > tables in the future. Maybe maintain a list of missing tables, and add them
> > at the end?
> > 
> 
> Since the strncmp is effectively a != (rather than ==) this will only be
> reached when table is "ResourceLoadCount". I agree this doesn't lend well to
> adding new similar tables in the future, and reads poorly.
> 

Ah okay, this makes more sense now. I do think the strncmp makes this code harder to understand though. IMO I think converting the char* to a WTFString then doing a if (String(table) != "ResourceLoadCount"_s) would read better.

Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1346
> > > +        buildList(WTF::IteratorRange<Vector<unsigned>::iterator>(julianDaysInRange.begin(), julianDaysInRange.end())), ") ORDER BY day"));
> > 
> > I think theres a static buildList function already in the class you can use.
> > 
> 
> This uses the static buildList function

Awesome! Not sure what I thought I saw the first time around...

Created attachment 392627 [details]
Patch