[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 #8 from katherine_cheney at apple.com ---
(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...

-- 
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/fa45d4ac/attachment.htm>