[Webkit-unassigned] [Bug 213319] WIP: 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 #2 from John Wilander <wilander at apple.com> ---
Comment on attachment 402162
  --> https://bugs.webkit.org/attachment.cgi?id=402162
Patch

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

Great to see some code! We should make sure to enable the associated tests which will probably involve touching the test expectation files.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

As long as we make sure that the associated tests are run you can change this to "No new tests. Enabled existing tests."

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

Was this something that was removed earlier? I'm surprised this is needed.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:77
> +

Even if we find whitespace errors, we try not to change them unless we are changing the code adjacent to it. Otherwise it makes the revision history hard to read because it looks like you changed something here, for instance deleted code, where in fact you just removed whitespace.

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

Oh, so this was the reason for the addition of the encodeHashSet() function? This is something you want to call out in the change log – both that you did it and why you did it. "Changed encodeHashCountedSet() to encodeHashSet() because …"

I'm curious, did you change from counted set to just set?

> Source/WebCore/loader/ResourceLoadStatistics.cpp:198
> +IGNORE_WARNINGS_BEGIN("unused-result")

Yeah, this is a result of our sparse storage. We decided early on to not store empty data structures which means that an empty return in decoding should not be interpreted as an error but an empty (sub)structure.

> Source/WebCore/loader/ResourceLoadStatistics.h:126
> +    HashSet<RegistrableDomain> topFrameRegistrableDomainsWhichAccessedWebAPIs;

Interesting! We apparently introduced RegistrableDomain after this code was landed.

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