[webkit-reviews] review granted: [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 granted  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 #20 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

okay, looks good!

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

Okay, good.

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

Got it. Thanks for clarifying.

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

Seems reasonable.

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

Agreed.