[Webkit-unassigned] [Bug 208612] Resource Load Statistics: Keep a count of resource loads per day

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=208612

--- Comment #5 from mmokary at apple.com ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200305/9aac0063/attachment.htm>