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

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

-- 
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/3fcb3d25/attachment.htm>