[webkit-reviews] review denied: [Bug 213319] We should resurrect the older patch that collects some statistics of web API calls : [Attachment 402612] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Umar Iqbal <uiqbal at apple.com>'s
request for review:
Bug 213319: We should resurrect the older patch that collects some statistics
of web API calls
https://bugs.webkit.org/show_bug.cgi?id=213319

Attachment 402612: Patch

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




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

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

I think this is headed in the right direction, but seems to re-introduce code
that uses Strings instead of RegistrableDomains, which is not what we want. I
also don't understand the reason to switch the existing code from
HashCountedSet to HashSet, and would like some explanation of that.

Finally, because this could impact performance, I do not support turning it on
by default, everywhere as this current patch does. We should have a runtime
flag to turn it on/off that can be used for testing before we enabled this to
compile and run at all times.

> Source/WebKitLegacy/mac/ChangeLog:8
> +	   + Enabled ENABLE_WEB_API_STATISTICS flag

Do we actually want this turned on for all users, all the time? I would expect
this to either be controlled by a debug menu flag, or for this to be off
everywhere and only turned on when building to work on this feature. I do not
support turning this on by default yet.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:63
> +static void encodeHashSet(KeyedEncoder& encoder, const String& label,  const
String& key, const HashSet<String>& hashSet)

This method appears to already exist in my code, outside of this ENABLE()
block. Indeed, if I look up a few dozen lines in this tool I can see the
official 'encodeHashSet' method. It is also based on the correct, modern
RegistrableDomain code. I think you can entirely delete this new function.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:129
> +    encodeHashSet(encoder, "topFrameRegistrableDomainsWhichAccessedWebAPIs",
"domain", topFrameRegistrableDomainsWhichAccessedWebAPIs);

Can you clarify why you are changing from a HashCounted set to a HashSet?

> Source/WebCore/loader/ResourceLoadStatistics.cpp:177
> +static void decodeHashSet(KeyedDecoder& decoder, const String& label, const
String& key, HashSet<String>& hashSet)

Likewise, this already exists but uses RegistrableDomain instead of String. I
don't think we want to use this code.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:342
> +	   decodeHashSet(decoder,
"topFrameRegistrableDomainsWhichAccessedWebAPIs", "domain",
topFrameRegistrableDomainsWhichAccessedWebAPIs);

Ditto regarding change from HashCounted to Hash

> Source/WebCore/loader/ResourceLoadStatistics.cpp:377
> +static void appendHashSet(StringBuilder& builder, const String& label, const
HashSet<String>& hashSet)

Why is this needed, when there is an existing 'appendHashSet' that uses
RegistrableDomain?

> LayoutTests/platform/ios-wk2/TestExpectations:72
> +http/tests/webAPIStatistics [ Pass ]

I would leave this Skipped until we have a toggle to turn the feature on/off
dynamically.

> LayoutTests/platform/mac-wk2/TestExpectations:73
> +http/tests/webAPIStatistics [ Pass ]

Ditto.