[webkit-reviews] review denied: [Bug 173499] Resource Load Statistics: Add telemetry : [Attachment 313161] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied John Wilander <wilander at apple.com>'s
request for review:
Bug 173499: Resource Load Statistics: Add telemetry
https://bugs.webkit.org/show_bug.cgi?id=173499

Attachment 313161: Patch

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




--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 313161
  --> https://bugs.webkit.org/attachment.cgi?id=313161
Patch

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

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:244
> +	   m_fireTelementryHandler();

typo: m_fireTelementryHandler -> m_fireTelemetryHandler

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:344
> +	  
page.value->postMessageToInjectedBundle("ResourceLoadStatisticsTelemetryFinishe
d", messageBody);

Why doesn't this send an IPC to the WebProcess and have it iterate over the
pages? This would reduce the amount of IPC.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:65
> +    m_telemetryTimer.start(5_s, WTF::Seconds::fromHours(24));

24_h ?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:202
> +	   m_telemetryTimer.startOneShot(WTF::Seconds::fromMilliseconds(100));

100_ms ?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:113
> +    WebCore::Timer m_telemetryTimer;

We should not be using WebCore::Timer in the UIProcess. RunLoop::Timer is the
one you want I believe.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:42
> +static unsigned minimumPrevalentResourcesForTelemetry = 3;

const unsigned minimumPrevalentResourcesForTelemetry = 3;

Should be const and does not need the static.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:46
> +    unsigned numberOfTimesDataRecordsRemoved;

I believe these members should have inline initializers.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:53
> +static inline Vector<PrevalentResourceTelemetry>
sortPrevalentResources(Vector<WebCore::ResourceLoadStatistics>&
resourceLoadStatistics)

Seems big to inline. The name is a bit weird because it does more than sorting.
Maybe toSortedPrevalentResources()?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:55
> +    Vector<PrevalentResourceTelemetry> sorted;

You can reverseInitialCapacity(resourceLoadStatistics.size());

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:57
> +	   sorted.append(PrevalentResourceTelemetry {

You can uncheckedAppend().

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:67
> +	   bool operator()(PrevalentResourceTelemetry a,
PrevalentResourceTelemetry b) const

Shouldn't these be const PrevalentResourceTelemetry& ?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:79
> +static inline unsigned
withUserInteraction(Vector<PrevalentResourceTelemetry> v, unsigned begin,
unsigned end)

Why are all these functions inline?

I doubt we should be passing the Vector by value here. Should likely be a const
Vector&.

v is a bad name.

The name withUserInteraction() is really unclear. What does this method do?

Shouldn't we babe using size_t for those begin / end (same comment below).

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:85
> +    for (unsigned i = begin; i < end; i++) {

++i.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:86
> +	   if (v.at(i).hasHadUserInteraction)

We prefer [I] when the vector is not a pointer.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:87
> +	       result++;

++result;

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:93
> +static inline unsigned median(Vector<unsigned> v)

Again passing Vector by value.. and inline

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:98
> +	   return v.at(0);

[0]

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:104
> +    return (v.at(middle - 1) + v.at(middle)) / 2;

[middle -1]

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:107
> +static inline unsigned median(Vector<PrevalentResourceTelemetry> v, unsigned
begin, unsigned end, std::function<unsigned(const PrevalentResourceTelemetry
telemetry)>&& statisticGetter)

Again passing Vector by value.. and inline.

Please use WTF::Function instead of std::function.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:113
> +    for (unsigned i = begin; i <= end; i++)

++i

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:114
> +	   part.append(statisticGetter(v.at(i)));

[I]