[webkit-reviews] review denied: [Bug 187773] Added Web API Statistics Collection : [Attachment 345578] 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 345578: Patch

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




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

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

Woody and I talked about this patch today. Looking at the canvas logic, it
seems sufficient to define at least two new ResourceLoadObserver functions:
logCanvasRead() and logCanvasMeasureOrWrite() to log any operation that reads
the pixel data from the canvas and text measurement and draw operations,
respectively. The logCanvasRead() function could take an HTMLCanvasElement (or
just the height and width) and the logCanvasMeasureOrWrite() function could
take  an HTMLCanvasElement (or just the height and width) and the string that
was measured or drawn.

If we make the above change, then we should move the CanvasLog struct to be a
private struct of ResourceLoadStatistics since that this only code that makes
use of it.

> Source/WebCore/css/CSSFontSelector.cpp:328
> +#if ENABLE(WEB_API_STATISTICS)
> +	   auto font = FontCache::singleton().fontForFamily(fontDescription,
familyForLookup);
> +	   if (font == nullptr) {
> +	       // Log font failed to load
> +	       ResourceLoadObserver::shared().logFontLoad(*m_document,
familyName, false);
> +	   }
> +	   return FontRanges((RefPtr<Font>&&)font);
> +#else
>	   return
FontRanges(FontCache::singleton().fontForFamily(fontDescription,
familyForLookup));
> +#endif

Can we share more code?

> Source/WebCore/html/HTMLCanvasElement.cpp:1050
> +    m_textWritten.clear();

m_textWritten always holds exactly one string. It is unnecessary for
m_textWritten to be a HashSet.

> Source/WebCore/loader/CanvasLog.h:32
> +    HashSet<String> m_fontsUsed;

We are not making meaningful use of this. We should remove it.

> Source/WebCore/loader/CanvasLog.h:37
> +struct CanvasLog {
> +    HashSet<String> m_textWritten;
> +    HashSet<String> m_fontsUsed;
> +    
> +    unsigned m_maxWidth { 0 };
> +    unsigned m_maxHeight { 0 };
> +    
> +    bool m_canvasDataRead { false };

We should make this a private class of ResourceLoadStatistics and remove all
"m_" prefixes from ivars.

> Source/WebCore/loader/ResourceLoadObserver.cpp:315
> +    if
(statistics.canvasLog.m_textWritten.add(canvasLogForUpdate.m_textWritten.begin(
), canvasLogForUpdate.m_textWritten.end()))

What is the lifetime of ResourceLoadStatistics? Better question, can
statistics.canvasLog.m_textWritten grow unbounded?