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 42 matching lines @@
 RemoteFontFaceSource::RemoteFontFaceSource(FontResource* font,
                                            CSSFontSelector* fontSelector,
                                            FontDisplay display)
     : m_font(font),
       m_fontSelector(fontSelector),
       m_display(display),
       m_period(display == FontDisplaySwap ? SwapPeriod : BlockPeriod),
       m_histograms(font->url().protocolIsData()
                        ? FontLoadHistograms::FromDataURL
                        : font->isLoaded() ? FontLoadHistograms::FromMemoryCache
-                                          : FontLoadHistograms::FromUnknown),
+                                          : FontLoadHistograms::FromUnknown,
+                   m_display),
       m_isInterventionTriggered(false) {
   ThreadState::current()->registerPreFinalizer(this);
   m_font->addClient(this);
 
   if (shouldTriggerWebFontsIntervention()) {
     m_isInterventionTriggered = true;
     m_period = SwapPeriod;
     m_fontSelector->document()->addConsoleMessage(ConsoleMessage::create(
         OtherMessageSource, InfoMessageLevel,
         "Slow network is detected. Fallback font will be used while loading: " +
@@ skipping 31 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->isCORSFailed(),
+                          m_font->getStatus() == Resource::LoadError,
+                          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->isCORSFailed(),
+                                 m_font->getStatus() == Resource::LoadError,
+                                 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(
+    bool isCorsFailed,

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

Probably we can just have one argument to indicate any error here.

[tbansal1, 2016/10/21 06:58:13]

I am still using two different errors. It seems more readable this way, but I
can change if you feel strongly about this.

+    bool loadError,
     bool isInterventionTriggered) {
-  if (!m_isLongLimitExceeded)
-    recordInterventionResult(isInterventionTriggered);
+  if (!m_isLongLimitExceeded) {

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

Can we check error here?

if (!m_isLongLimitExceeded  & !error)
  recordInterventionResult(isInterventionTriggered);

[tbansal1, 2016/10/21 06:58:13]

Done.

+    recordInterventionResult(isCorsFailed, loadError, isInterventionTriggered);
+  }
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::longLimitExceeded(
+    bool isCorsFailed,

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

These two arguments look curious to me, because this method is called when long
timeout is fired and loading is still in progress without any error.

If we want to exclude cases that long limit is exceeded and some network errors
happen, we need to stop recording intervention result here and report it only
when the load is finished. But it causes another issue that we fail to report
metrics in cases user takes another navigation while loading the font.

[tbansal1, 2016/10/21 06:58:13]

Done.

+    bool loadError,
     bool isInterventionTriggered) {
   m_isLongLimitExceeded = true;
   maySetDataSource(FromNetwork);
-  recordInterventionResult(isInterventionTriggered);
+  recordInterventionResult(isCorsFailed, loadError, 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(
+    bool isCorsFailed,
+    bool loadError,
     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/20 10:12:44]

Do you have any reason why you omit only missedCacheInterventionHistogram here?

IMO, as names implies, it would be better not to have other differences between
WebFont.InterventionResult and WebFont.InterventionResult.MissedCache than cache
condition.

[tbansal1, 2016/10/21 06:58:13]

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 && !isCorsFailed &&
+      m_fontDisplay == FontDisplayAuto && !loadError) {
     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