[Webkit-unassigned] [Bug 213319] We should resurrect the older patch that collects some statistics of web API calls

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=213319

--- Comment #19 from Umar Iqbal <uiqbal at apple.com> ---
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

>> 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.

We also have a runtime flag to turn it on/off from the settings menu (in internal features) and it is set to false in RuntimeEnabledFeatures.h by default.

>> 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.

It is needed by WebCore::ResourceLoadStatistics::fontsFailedToLoad which stores font family as String. Since RegistrableDomains is designed to store domains, and not font family, we should keep it to encode string HashSet used by fontsFailedToLoad.

>> 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?

In an earlier patch (https://bugs.webkit.org/show_bug.cgi?id=195071, check the changelog) it was mentioned that the counts were never used and the patch changed the other HashCountedSets to HashSets (e.g. WebCore::ResourceLoadStatistics::topFrameUniqueRedirectsTo). I followed the same principle and updated HashCountedSets to HashSet for WebCore::ResourceLoadStatistics::topFrameRegistrableDomainsWhichAccessedWebAPIs

>> 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.

Same reason as explained earlier for encodeHashSet

>> 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.

Since we turn the feature on/off from the settings menu. We can keep it.

-- 
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/20200624/848b3985/attachment.htm>