[webkit-reviews] review denied: [Bug 187773] Add Web API Statistics Collection : [Attachment 348976] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Woodrow Wang
<wwang153 at stanford.edu>'s request for review:
Bug 187773: Add Web API Statistics Collection
https://bugs.webkit.org/show_bug.cgi?id=187773

Attachment 348976: Patch

https://bugs.webkit.org/attachment.cgi?id=348976&action=review




--- Comment #80 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 348976
  --> https://bugs.webkit.org/attachment.cgi?id=348976
Patch

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

> Source/WebCore/css/CSSFontFaceSource.cpp:187
> +		   if (auto document = fontSelector->document())

nit: we prefer auto* assuming its returns a raw pointer.

> Source/WebCore/css/CSSFontSelector.cpp:403
> +    RefPtr<Font> font =
FontCache::singleton().fontForFamily(fontDescription,
m_document->settings().pictographFontFamily());

nit: auto

> Source/WebCore/css/CSSFontSelector.cpp:405
> +	   if (m_document)

Unnecessary null check given that it is dereferenced right above.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:369
> +	   HTMLCanvasElement& canvas = this->canvas();

auto&

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:463
> +	   HTMLCanvasElement& canvas = this->canvas();

unnecessary local variable.

> Source/WebCore/loader/CanvasActivityRecord.cpp:29
> +const unsigned textCapacity = 10;

I think the naming is not clear. Maybe something like maximumTextLengthToRecord
?

> Source/WebCore/loader/CanvasActivityRecord.cpp:36
> +    if (textWritten.size() < textCapacity &&
textWritten.add(text).isNewEntry)

Seems to me like this could get out of hand pretty quickly for some use cases.
We should probably have a max number of entries in the HashSet as well.

> Source/WebCore/loader/ResourceLoadObserver.cpp:248
> +    if (!shouldLog(document.sessionID().isEphemeral()) || !document.frame())

We can probably drop the !document.frame() check if you use topDocument().

> Source/WebCore/loader/ResourceLoadObserver.cpp:254
> +    statistics.fontsLoaded.add(familyName);

Is it intentional that the font gets added to both fontsFailedToLoad and
fontsLoaded when it fails to load?

> Source/WebCore/loader/ResourceLoadObserver.cpp:255
> +    auto mainFrameRegistrableDomain =
primaryDomain(document.frame()->mainFrame().document()->url());

document.frame()->mainFrame().document()->url() -> document.topDocument().url()

> Source/WebCore/loader/ResourceLoadObserver.cpp:262
> +    if (!shouldLog(document.sessionID().isEphemeral()) || !document.frame())

We can probably drop the !document.frame() check if you use topDocument().

> Source/WebCore/loader/ResourceLoadObserver.cpp:266
> +    auto mainFrameRegistrableDomain =
primaryDomain(document.frame()->mainFrame().document()->url());

topDocument()

> Source/WebCore/loader/ResourceLoadObserver.cpp:274
> +    if (!shouldLog(document.sessionID().isEphemeral()) || !document.frame())

We can probably drop the !document.frame() check if you use topDocument().

> Source/WebCore/loader/ResourceLoadObserver.cpp:279
> +    auto mainFrameRegistrableDomain =
primaryDomain(document.frame()->mainFrame().document()->url());

topDocument()

> Source/WebCore/loader/ResourceLoadObserver.cpp:290
> +    if (!shouldLog(document.sessionID().isEphemeral()) || !document.frame())

We can probably drop the !document.frame() check if you use topDocument().

> Source/WebCore/loader/ResourceLoadObserver.cpp:295
> +    if (!statistics.navigatorFunctionsAccessed.contains(functionName)) {

Unnecessary double lookup? Can't use use to AddResult.isNewEntry?

> Source/WebCore/loader/ResourceLoadObserver.cpp:299
> +    auto mainFrameRegistrableDomain =
primaryDomain(document.frame()->mainFrame().document()->url());

topDocument()

> Source/WebCore/loader/ResourceLoadObserver.cpp:308
> +    if (!shouldLog(document.sessionID().isEphemeral()) || !document.frame())

We can probably drop the !document.frame() check if you use topDocument().

> Source/WebCore/loader/ResourceLoadObserver.cpp:313
> +    if (!statistics.screenFunctionsAccessed.contains(functionName)) {

Unnecessary double lookup?

> Source/WebCore/loader/ResourceLoadObserver.cpp:317
> +    auto mainFrameRegistrableDomain =
primaryDomain(document.frame()->mainFrame().document()->url());

topDocument()

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

Wouldn't it be clearer with hashSet / key parameters reversed?

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

Not sure this utility function brings much. Call sites could be updated to pass
"origin".

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

Call sites could be updated and this could be dropped.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:162
> +static void decodeOriginHashSet(KeyedDecoder& decoder, const String& label,
HashSet<String>& hashSet)

Call sites could be updated and this be dropped.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:172
> +    optionSet.add(OptionSet<T>::fromRaw(optionSetBitMask));

doesn't this work?
optionSet = OptionSet<T>::fromRaw(optionSetBitMask);

> Source/WebCore/loader/ResourceLoadStatistics.cpp:175
> +static void decodeFontHashSet(KeyedDecoder& decoder, const String& label,
HashSet<String>& hashSet)

Call sites could be updated and this be dropped.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:309
> +static String navigatorEnumToString(ResourceLoadStatistics::NavigatorAPI
navigatorEnum)

Bad naming IMO. maybe navigatorAPIEnumToString? or simply
navigatorAPIToString().

> Source/WebCore/loader/ResourceLoadStatistics.cpp:313
> +	   return "javaEnabled";

return ASCIILiteral("javaEnabled");

Ditto for other literal strings below.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:324
> +    default:

Do we really need a default case? We usually omit it so that we get a compiling
error if we fail to handle one of the enum values.

Ditto for other switch cases below.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:329
> +static String screenEnumToString(ResourceLoadStatistics::ScreenAPI
screenEnum)

Same comment about naming.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:353
> +static void appendNavigatorOptionSet(StringBuilder& builder, const
OptionSet<ResourceLoadStatistics::NavigatorAPI>& optionSet)

NavigatorAPIOptionSet ? Omitting "API" makes things confusing.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:357
> +    builder.appendLiteral("	  ");

builder.appendLiteral("    navigatorFunctionsAccessed:\n");

> Source/WebCore/loader/ResourceLoadStatistics.cpp:358
> +    builder.append("navigatorFunctionsAccessed:");

Not needed.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:359
> +    builder.append('\n');

Not needed.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:367
> +static void appendScreenOptionSet(StringBuilder& builder, const
OptionSet<ResourceLoadStatistics::ScreenAPI>& optionSet)

Same comment about naming.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:371
> +    builder.appendLiteral("	  ");

Please do all 3 in one append, as suggested above.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:374
> +    for (OptionSet<ResourceLoadStatistics::ScreenAPI>::iterator it =
optionSet.begin(); it != optionSet.end(); ++it) {

Any reason you cannot use a modern C++ range loop? for (auto& screenAPI :
optionSet)

> Source/WebCore/loader/ResourceLoadStatistics.h:108
> +    enum class NavigatorAPI : uint64_t {

Why uint64_t ?

> Source/WebCore/loader/ResourceLoadStatistics.h:116
> +    enum class ScreenAPI : uint64_t {

Why uint64_t ?