[webkit-reviews] review denied: [Bug 195474] Resource Load Statistics: Make it possible exclude localhost from classification : [Attachment 364055] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 195474: Resource Load Statistics: Make it possible exclude localhost from
classification
https://bugs.webkit.org/show_bug.cgi?id=195474

Attachment 364055: Patch

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




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

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

Looks like you neglected to update the SQLite code paths, so r- to fix that.

It also seems like you adjusted a number of test cases. Was that done because
it's needed for this localhost behavior? Or did you notice the cause of test
flakiness and decide to fix them at the same time?

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:7
2
> +    , m_shouldIncludeLocalhost { shouldIncludeLocalhost }

It seems like this should be part of the base class, rather than defining and
setting in the two children.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:1
80
> +}

This whole method should be in the base class.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:1
86
> +	       continue;

You should be making these same changes in the
ResourceLoadStatisticsDatabaseStore class, too.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:4
71
> +	   return;

Ditto.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:5
02
> +	   return false;

Ditto.