[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

Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #402612|review-                     |review+, commit-queue+
              Flags|                            |

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

-- 
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/c998aeb6/attachment-0001.htm>