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

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Woodrow Wang
<wwang153 at stanford.edu>'s request for review:
Bug 187773: Added Web API Statistics Collection
https://bugs.webkit.org/show_bug.cgi?id=187773

Attachment 345617: Patch

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




--- Comment #16 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 345617
  --> https://bugs.webkit.org/attachment.cgi?id=345617
Patch

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

> Source/WebCore/css/CSSFontSelector.cpp:306
>  {
>      // If this ASSERT() fires, it usually means you forgot a
document.updateStyleIfNeeded() somewhere.
>      ASSERT(!m_buildIsUnderway || m_computingRootStyleFontCount);
> +    
> +#if ENABLE(WEB_API_STATISTICS)

>From the name of this function CSSFontSelector::fontRangesForFamily() it is not
obvious whether this is the most direct way to know whether a font loaded or
failed. Where does this function fall in the flow to support the Font Loading
API events <https://drafts.csswg.org/css-font-loading/#FontFaceSet-events>?

> Source/WebCore/css/CSSFontSelector.cpp:308
> +	   ResourceLoadObserver::shared().logFontLoad(*m_document, familyName,
true);

It is not obvious what the purpose of the last argument is without looking at
the prototype of ResourceLoadObserver::logFontLoad(). I suggest we use an enum
class instead of a boolean to make the purpose of the last argument clear at
the call site.

> Source/WebCore/loader/ResourceLoadObserver.cpp:292
> +    String registrableDomainString = primaryDomain(document.url());

I do not see the value in having the suffix "String" in the name of this
variable. I suggest we would remove this suffix and call this variable
registrableDomain. I also suggest we change the data type of this variable from
String to auto and move this definition to be above line 295 so as to be closer
to where this variable is used.  In general, it is good programming practice to
place variables as close to where they are used as possible both for
readability as well as to avoid computation until needed.

> Source/WebCore/loader/ResourceLoadObserver.cpp:293
> +    const URL& mainFrameURL =
document.frame()->mainFrame().document()->url();

This variable is only referenced in the line below and I do not find its name
any more descriptive than its value. I suggest we inline its value into the
line below and remove this variable.

> Source/WebCore/loader/ResourceLoadObserver.cpp:294
> +    auto mainFrameRegistrableDomain = primaryDomain(mainFrameURL);

I suggest that we move this variable to be above line 299 (right above the line
that uses it).

> Source/WebCore/loader/ResourceLoadObserver.cpp:317
> +void ResourceLoadObserver::logCanvasRead(const Document& document, const
unsigned height, const unsigned width)
> +{
> +    if (!shouldLog(document.page()))
> +	   return;
> +    const URL& mainFrameURL =
document.frame()->mainFrame().document()->url();
> +    String registrableDomainString = primaryDomain(document.url());
> +    auto mainFrameRegistrableDomain = primaryDomain(mainFrameURL);
> +    auto& statistics =
ensureResourceStatisticsForPrimaryDomain(registrableDomainString);
> +    statistics.mainFrameURL.add(mainFrameRegistrableDomain);
> +    statistics.canvasLog.maxHeight =
std::max(statistics.canvasLog.maxHeight, height);
> +    statistics.canvasLog.maxWidth = std::max(statistics.canvasLog.maxWidth,
width);
> +    statistics.canvasLog.canvasDataRead = true;
> +    if
(statistics.canvasUnderTopFrameOrigins.add(mainFrameRegistrableDomain).isNewEnt
ry)
> +	   scheduleNotificationIfNeeded();
> +}

Please simplify this code using the suggestions I gave for
ResourceLoadObserver::logFontLoad()?

> Source/WebCore/loader/ResourceLoadObserver.cpp:337
> +void ResourceLoadObserver::logCanvasWriteOrMeasure(const Document& document,
const unsigned height, const unsigned width, const String& textWritten)
> +{
> +    if (!shouldLog(document.page()))
> +	   return;
> +    bool shouldCallNotificationCallback = false;
> +    const URL& mainFrameURL =
document.frame()->mainFrame().document()->url();
> +    String registrableDomainString = primaryDomain(document.url());
> +    auto mainFrameRegistrableDomain = primaryDomain(mainFrameURL);
> +    auto& statistics =
ensureResourceStatisticsForPrimaryDomain(registrableDomainString);
> +    statistics.mainFrameURL.add(mainFrameRegistrableDomain);
> +    statistics.canvasLog.maxHeight =
std::max(statistics.canvasLog.maxHeight, height);
> +    statistics.canvasLog.maxWidth = std::max(statistics.canvasLog.maxWidth,
width);
> +    if (statistics.canvasLog.textWritten.add(textWritten))
> +	   shouldCallNotificationCallback = true;
> +    if
(statistics.canvasUnderTopFrameOrigins.add(mainFrameRegistrableDomain).isNewEnt
ry)
> +	   shouldCallNotificationCallback = true;
> +    if (shouldCallNotificationCallback)
> +	   scheduleNotificationIfNeeded();
> +}

Ditto.

> Source/WebCore/loader/ResourceLoadObserver.cpp:353
> +void ResourceLoadObserver::logNavigatorLoad(const Frame* frame, const
String& functionName)
> +{
> +    if (!frame)
> +	   return;
> +    Document* document = frame->document();
> +    if (!shouldLog(document->page()))
> +	   return;
> +    String registrableDomainString = primaryDomain(document->url());
> +    const URL& mainFrameURL = frame->mainFrame().document()->url();
> +    auto mainFrameRegistrableDomain = primaryDomain(mainFrameURL);
> +    auto& statistics =
ensureResourceStatisticsForPrimaryDomain(registrableDomainString);
> +    statistics.navigatorFunctionsAccessed.add(functionName);
> +    if (statistics.mainFrameURL.add(mainFrameRegistrableDomain).isNewEntry)
> +	   scheduleNotificationIfNeeded();
> +}

Ditto.

This function mostly works with the document. I suggest it take a const
Document& instead of a const Frame*.

> Source/WebCore/loader/ResourceLoadObserver.cpp:369
> +void ResourceLoadObserver::logScreenLoad(const Frame* frame, const String&
functionName)
> +{
> +    if (!frame)
> +	   return;
> +    Document* document = frame->document();
> +    if (!shouldLog(document->page()))
> +	   return;
> +    String registrableDomainString = primaryDomain(document->url());
> +    const URL& mainFrameURL = frame->mainFrame().document()->url();
> +    auto mainFrameRegistrableDomain = primaryDomain(mainFrameURL);
> +    auto& statistics =
ensureResourceStatisticsForPrimaryDomain(registrableDomainString);
> +    statistics.screenFunctionsAccessed.add(functionName);
> +    if (statistics.mainFrameURL.add(mainFrameRegistrableDomain).isNewEntry)
> +	   scheduleNotificationIfNeeded();
> +}

Ditto.

This function mostly works with the document. I suggest it take a const
Document& instead of a const Frame*.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:65
> +    encoder.encodeObject(label, canvasLog, [](KeyedEncoder& encoderInner,
const CanvasLog log) {

This lambda is unnecessarily copying the passed CanvasLog. We should change
"const CanvasLog" to "const CanvasLog&"  to just pass a const lvalue reference
to the CanvasLog and avoid the copy. Please put a space between the capture
list ([]) and the parameter list.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:69
> +	   encoderInner.encodeObjects("textWritten", log.textWritten.begin(),
log.textWritten.end(), [](KeyedEncoder& encoderInner2, const String text) {

This lambda cause ref-count churn with the pass string. We should change const
String to "const String&" to avoid it. Please put a space between the capture
list ([]) and the parameter list.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:153
> +    decoder.decodeObject(label, canvasLog, [](KeyedDecoder& decoderInner,
CanvasLog& canvasLog) {

Please put a space between the capture list ([]) and the parameter list.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:161
> +	   decoderInner.decodeObjects("textWritten", ignore,
[&canvasLog](KeyedDecoder& decoderInner2, String text) {

Please put a space between the capture list and the parameter list. "String
text"?

> Source/WebCore/loader/ResourceLoadStatistics.cpp:165
> +	       const String canvasText = text;

We're making an unnecessary copy here.

> Source/WebCore/loader/ResourceLoadStatistics.cpp:418
> +    canvasLog.maxHeight =  std::max(otherCanvasLog.maxHeight,
canvasLog.maxHeight);
> +    canvasLog.maxWidth =  std::max(otherCanvasLog.maxWidth,
canvasLog.maxWidth);

Nit: Extra space character to the right of the '='.

> Source/WebCore/page/Navigator.cpp:78
> +    ResourceLoadObserver::shared().logNavigatorLoad(frame(), "appVersion");

We should only call this function when we have a non-null frame. Otherwise, we
are performing an unnecessary function call because
ResourceLoadObserver::logNavigatorLoad() does nothing when a null frame is
passed to it.

> Source/WebCore/page/Navigator.cpp:91
> +    ResourceLoadObserver::shared().logNavigatorLoad(frame(), "userAgent");

Ditto.

> Source/WebCore/page/Navigator.cpp:106
> +    ResourceLoadObserver::shared().logNavigatorLoad(frame(), "plugins");

Ditto.

> Source/WebCore/page/Navigator.cpp:116
> +    ResourceLoadObserver::shared().logNavigatorLoad(frame(), "mimeTypes");

Ditto.

> Source/WebCore/page/Navigator.cpp:126
> +    ResourceLoadObserver::shared().logNavigatorLoad(frame(),
"cookieEnabled");

Ditto.

> Source/WebCore/page/Navigator.cpp:144
> +    ResourceLoadObserver::shared().logNavigatorLoad(frame(), "javaEnabled");

Ditto.

> Source/WebCore/page/Navigator.cpp:162
> +    ResourceLoadObserver::shared().logNavigatorLoad(frame(), "standAlone");

Ditto.

> Source/WebCore/page/Navigator.cpp:172
> +    ResourceLoadObserver::shared().logNavigatorLoad(frame(),
"getStorageUpdates");

Ditto.

> Source/WebCore/page/Screen.cpp:52
> +#if ENABLE(WEB_API_STATISTICS)
> +    ResourceLoadObserver::shared().logScreenLoad(frame(), "height");
> +#endif

We should only call this function when we have a non-null frame. Otherwise, we
are performing an unnecessary function call because
ResourceLoadObserver::logScreenLoad() does nothing when a null frame is passed
to it.

> Source/WebCore/page/Screen.cpp:62
> +    ResourceLoadObserver::shared().logScreenLoad(frame(), "width");

Ditto.

> Source/WebCore/page/Screen.cpp:73
> +    ResourceLoadObserver::shared().logScreenLoad(frame(), "colorDepth");

Ditto.

> Source/WebCore/page/Screen.cpp:83
> +    ResourceLoadObserver::shared().logScreenLoad(frame(), "pixelDepth");

Ditto.

> Source/WebCore/page/Screen.cpp:93
> +    ResourceLoadObserver::shared().logScreenLoad(frame(), "availLeft");

Ditto.

> Source/WebCore/page/Screen.cpp:103
> +    ResourceLoadObserver::shared().logScreenLoad(frame(), "availTop");

Ditto.

> Source/WebCore/page/Screen.cpp:113
> +    ResourceLoadObserver::shared().logScreenLoad(frame(), "availHeight");

Ditto.

> Source/WebCore/page/Screen.cpp:123
> +    ResourceLoadObserver::shared().logScreenLoad(frame(), "availWidth");

Ditto.