[webkit-reviews] review granted: [Bug 223726] Implement PCM SQLite changes based on spec review : [Attachment 424362] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted katherine_cheney at apple.com's
request for review:
Bug 223726: Implement PCM SQLite changes based on spec review
https://bugs.webkit.org/show_bug.cgi?id=223726

Attachment 424362: Patch

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




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

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

r=me. I have a few minor suggestions, but otherwise looks good.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:274
> +static const Vector<String> expectedUnattributedColumns()

Since this is a never-destroyed object, I would return a const reference to
avoid making a copy. Also, why not use the ExpectedColumns type alias?

static const ExpectedColumns& expectedUnattributedColumns()

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:280
> +static const Vector<String> expectedAttributedColumns()

Ditto "const ExpectedColumns&"

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:444
> +    auto currentSchema = tableSchema(table);

Maybe:
auto currentSchema = tableSchema("AttributedPrivateClickMeasurement"_s)

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:456
> +    auto oldSchema = tableSchema(table);

I wonder if the compiler could do a better job if you didn't stack allocate
'table' every time you hit this call.

Maybe:
auto oldSchema = tableSchema("TopFrameUniqueRedirectsTo"_s);

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:536
> +    Vector<String> columns;

Depending on how many columns table tend to have, there might be benefit to
hinting at the capacity of 'columns'. (Vector::reserveCapacity)

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:571
> +    auto attributedTableName = "AttributedPrivateClickMeasurement"_s;

I would suggest making these 'const'.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:600
> +    auto foundAttributedTableColumns = columnsForTable(attributedTableName);

I think all of the above could be 'const'.