Log web fonts intervention result more accurately

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

Index: third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
diff --git a/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp b/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
index 9c3f564d0932a2bd7700a03c479c1a80b3f92954..632ad446970f8f49b3d74c888017b36c6d3e5c8d 100644
--- a/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
+++ b/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
@@ -60,7 +60,8 @@ RemoteFontFaceSource::RemoteFontFaceSource(FontResource* font,
       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);
@@ -112,7 +113,9 @@ void RemoteFontFaceSource::notifyFinished(Resource*) {
                                     ? 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.
@@ -151,7 +154,9 @@ void RemoteFontFaceSource::fontLoadLongLimitExceeded(FontResource*) {
   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() {
@@ -259,16 +264,21 @@ void RemoteFontFaceSource::FontLoadHistograms::fallbackFontPainted(
 }
 
 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(
@@ -387,6 +397,8 @@ void RemoteFontFaceSource::FontLoadHistograms::recordLoadTimeHistogram(
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::recordInterventionResult(
+    bool isCorsFailed,
+    bool loadError,
     bool isTriggered) {
   CHECK_NE(FromUnknown, m_dataSource);
 
@@ -403,8 +415,14 @@ void RemoteFontFaceSource::FontLoadHistograms::recordInterventionResult(
   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