Log web fonts intervention result more accurately

third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/css/RemoteFontFaceSource.h"
 
 #include "core/css/CSSCustomFontData.h"
 #include "core/css/CSSFontFace.h"
 #include "core/css/CSSFontSelector.h"
 #include "core/dom/Document.h"
@@ skipping 94 matching lines @@
 
 bool RemoteFontFaceSource::isValid() const {
   return !m_font->errorOccurred();
 }
 
 void RemoteFontFaceSource::notifyFinished(Resource*) {
   m_histograms.maySetDataSource(m_font->response().wasCached()
                                     ? FontLoadHistograms::FromDiskCache
                                     : FontLoadHistograms::FromNetwork);
   m_histograms.recordRemoteFont(m_font.get());
-  m_histograms.fontLoaded(m_isInterventionTriggered);
+  m_histograms.fontLoaded(m_font, m_display, m_isInterventionTriggered);
 
   m_font->ensureCustomFontData();
   // FIXME: Provide more useful message such as OTS rejection reason.
   // See crbug.com/97467
   if (m_font->getStatus() == Resource::DecodeError &&
       m_fontSelector->document()) {
     m_fontSelector->document()->addConsoleMessage(ConsoleMessage::create(
         OtherMessageSource, WarningMessageLevel,
         "Failed to decode downloaded font: " + m_font->url().elidedString()));
     if (m_font->otsParsingMessage().length() > 1)
@@ skipping 18 matching lines @@
     switchToFailurePeriod();
 }
 
 void RemoteFontFaceSource::fontLoadLongLimitExceeded(FontResource*) {
   if (m_display == FontDisplayBlock ||
       (!m_isInterventionTriggered && m_display == FontDisplayAuto))
     switchToSwapPeriod();
   else if (m_display == FontDisplayFallback)
     switchToFailurePeriod();
 
-  m_histograms.longLimitExceeded(m_isInterventionTriggered);
+  m_histograms.longLimitExceeded(m_font, m_display, m_isInterventionTriggered);
 }
 
 void RemoteFontFaceSource::switchToSwapPeriod() {
   ASSERT(m_period == BlockPeriod);
   m_period = SwapPeriod;
 
   pruneTable();
   if (m_face) {
     m_fontSelector->fontFaceInvalidated();
     m_face->didBecomeVisibleFallback(this);
@@ skipping 87 matching lines @@
     m_loadStartTime = currentTimeMS();
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::fallbackFontPainted(
     DisplayPeriod period) {
   if (period == BlockPeriod && !m_blankPaintTime)
     m_blankPaintTime = currentTimeMS();
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::fontLoaded(
+    const FontResource* font,
+    const FontDisplay display,
     bool isInterventionTriggered) {
   if (!m_isLongLimitExceeded)
-    recordInterventionResult(isInterventionTriggered);
+    recordInterventionResult(font, display, isInterventionTriggered);
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::longLimitExceeded(
+    const FontResource* font,
+    const FontDisplay display,
     bool isInterventionTriggered) {
   m_isLongLimitExceeded = true;
   maySetDataSource(FromNetwork);
-  recordInterventionResult(isInterventionTriggered);
+  recordInterventionResult(font, display, isInterventionTriggered);
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::recordFallbackTime(
     const FontResource* font) {
   if (m_blankPaintTime <= 0)
     return;
   int duration = static_cast<int>(currentTimeMS() - m_blankPaintTime);
   DEFINE_STATIC_LOCAL(CustomCountHistogram, blankTextShownTimeHistogram,
                       ("WebFont.BlankTextShownTime", 0, 10000, 50));
   blankTextShownTimeHistogram.count(duration);
@@ skipping 98 matching lines @@
                       ("WebFont.DownloadTime.4.Over1MB", 0, 10000, 50));
   DEFINE_STATIC_LOCAL(
       CustomCountHistogram, missedCacheOver1mbHistogram,
       ("WebFont.MissedCache.DownloadTime.4.Over1MB", 0, 10000, 50));
   over1mbHistogram.count(duration);
   if (m_dataSource == FromNetwork)
     missedCacheOver1mbHistogram.count(duration);
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::recordInterventionResult(
+    const FontResource* font,
+    const FontDisplay display,
     bool isTriggered) {
   CHECK_NE(FromUnknown, m_dataSource);
 
   // interventionResult takes 0-3 values.
   int interventionResult = 0;
   if (m_isLongLimitExceeded)
     interventionResult |= 1 << 0;
   if (isTriggered)
     interventionResult |= 1 << 1;
   const int boundary = 1 << 2;
 
   DEFINE_STATIC_LOCAL(EnumerationHistogram, interventionHistogram,
                       ("WebFont.InterventionResult", boundary));
   DEFINE_STATIC_LOCAL(EnumerationHistogram, missedCacheInterventionHistogram,
                       ("WebFont.InterventionResult.MissedCache", boundary));
   interventionHistogram.count(interventionResult);
-  if (m_dataSource == FromNetwork)
+

[Takashi Toyoshima, 2016/10/19 06:36:52]

As you changed, this method is called from two places.
- fontLoaded()
- longLimitExceeded()

For the latter case, font->X() won't work correctly because loading is still
under working. So, can you have this error state check in caller side? We may
want to have this check only in fontLoaded() case, in notifyFinished().

That would omit to record WebFont.InterventionResult too at line 411, but I
think it's fine.

[tbansal1, 2016/10/19 08:27:56]

I need to check this to make sure, but when this method is called in case of
longLimitExceeded, then should not resource status be pending? I am skipping the
UMA only if the status is LoadError. So, the UMA should still be recorded in the
case of longLimitExceeded. Right?

[Takashi Toyoshima, 2016/10/19 10:16:28]

That's true for the current implementation. But we do not want to have
unnecessary assumptions here against Resource internal design. It could be
changed frequently, and would be a root of regression bugs. E.g., cache-aware
loading would have a retry logic, and it isn't trivial if the internal state
continues to be always pending there in the future.

Also passing |font| instance to m_histograms results in exposing too much
information from RemoteFontFaceSource to FontLoadHistograms. That's my another
concern.

[tbansal1, 2016/10/20 00:10:58]

Agreed. That;s why I am not checking for pending state, but instead for error
state which seems more reliable (since it is a clear signal that browser was not
able to fetch the resource). 

This would help in reducing false positives when the linked web font points to a
URL that does not exist. Right now, all those cases are incorrectly recorded as
false positives.
Done

+  // Do not record the result if the font failed to load since the failure
+  // could have happened either due to slow connection or unavailability of the
+  // server.
+  if (m_dataSource == FromNetwork && !font->isCORSFailed() &&
+      display == FontDisplayAuto && font->getStatus() != Resource::LoadError) {

[Takashi Toyoshima, 2016/10/19 06:36:52]

I slightly prefer to pass the |display| at m_histograms' constructor and hold it
inside the instance rather than passing it on demand.

Note: |display| should be always FontDisplayAuto unless users enable
chrome://flags/#enable-experimental-web-platform-features. So I thinks this
check won't affect results so much.

[tbansal1, 2016/10/20 00:10:58]

Done.

[Takashi Toyoshima, 2016/10/20 10:12:44]

Thanks!

     missedCacheInterventionHistogram.count(interventionResult);
+  }
 }
 
 RemoteFontFaceSource::FontLoadHistograms::CacheHitMetrics
 RemoteFontFaceSource::FontLoadHistograms::dataSourceMetricsValue() {
   switch (m_dataSource) {
     case FromDataURL:
       return DataUrl;
     case FromMemoryCache:
       return MemoryHit;
     case FromDiskCache:
       return DiskHit;
     case FromNetwork:
       return Miss;
     case FromUnknown:
     // Fall through.
     default:
       NOTREACHED();
   }
   return Miss;
 }
 
 }  // namespace blink