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

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted 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 349314: Patch

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




--- Comment #90 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 349314
  --> https://bugs.webkit.org/attachment.cgi?id=349314
Patch

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

I think the patch looks good, with a few small adjustments. Also, please
double-check that you meant to use 'familyForLookup' in
CSSFontSelector::fontRangesForFamily

> Source/WebCore/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=187773

Please include a Radar number here.

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

Should this be familyForLookup? Couldn't it have been changed by
resolveGenericFamily?

Also: Do we really want to log what WebKit figured out was the closest match to
the script's request? Why not log the actual requested font -- that seems like
it might have more signal than the value WebKit found as the closest match.

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

Personal preference, but I would only grab the pictographFontFamily once and
use it in both places. But it probably doesn't really matter.

> Source/WebCore/loader/CanvasActivityRecord.cpp:38
> +    return false;

Maybe this should be:

if (textWritten.size() >= maximumNumberOfStringsToRecord)
    return false;

return textWritten.add(text).isNewEntry;

I don't like how the important part of the function (textWritten.add(...)) is
kind of hidden at the end of the size check.