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 0d5f323799f2cf30939d9da36dda534df5e8b7d5..45ce1f6e3951a4b4a73d4f48ebc429d701ad063e 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,

[kinuko, 2016/10/21 17:07:11]

nit: if these two are only used in &&'ed bool check, shall we change
fontLoaded() to just take two params, say, loadError and
isInterventionTriggered, and just pass "m_font->isCORSFailed() ||
m_font->getStatus() == Resource::LoadError" here?

(either works for me though)

[tbansal1, 2016/10/21 17:14:32]

I thought about this, but concluded that it might be a bit confusing. If we use
|loadError| as the variable name, then |loadError| would now mean either a CORS
failed error, or an actual resource load error. This would be confusing.

If we pass in a single variable, then other options for the variable name are
|corsOrLoadError| or |error|. The former is clearer but mouthful. The latter is
a bit vague.

So, I decided to just use two different variables to be clearer.

+                          m_isInterventionTriggered);
 
   m_font->ensureCustomFontData();
   // FIXME: Provide more useful message such as OTS rejection reason.
@@ -263,16 +266,21 @@ void RemoteFontFaceSource::FontLoadHistograms::fallbackFontPainted(
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::fontLoaded(
+    bool isCorsFailed,
+    bool loadError,
     bool isInterventionTriggered) {
-  if (!m_isLongLimitExceeded)
+  if (!m_isLongLimitExceeded && m_fontDisplay == FontDisplayAuto &&
+      !isCorsFailed && !loadError) {
     recordInterventionResult(isInterventionTriggered);
+  }
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::longLimitExceeded(
     bool isInterventionTriggered) {
   m_isLongLimitExceeded = true;
   maySetDataSource(FromNetwork);
-  recordInterventionResult(isInterventionTriggered);
+  if (m_fontDisplay == FontDisplayAuto)
+    recordInterventionResult(isInterventionTriggered);
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::recordFallbackTime(