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..b81b68a5d608ffae3470a873ccf418e8233b69b0 100644
--- a/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
+++ b/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
@@ -112,7 +112,7 @@ void RemoteFontFaceSource::notifyFinished(Resource*) {
                                     ? 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.
@@ -151,7 +151,7 @@ void RemoteFontFaceSource::fontLoadLongLimitExceeded(FontResource*) {
   else if (m_display == FontDisplayFallback)
     switchToFailurePeriod();
 
-  m_histograms.longLimitExceeded(m_isInterventionTriggered);
+  m_histograms.longLimitExceeded(m_font, m_display, m_isInterventionTriggered);
 }
 
 void RemoteFontFaceSource::switchToSwapPeriod() {
@@ -259,16 +259,20 @@ void RemoteFontFaceSource::FontLoadHistograms::fallbackFontPainted(
 }
 
 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(
@@ -387,6 +391,8 @@ void RemoteFontFaceSource::FontLoadHistograms::recordLoadTimeHistogram(
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::recordInterventionResult(
+    const FontResource* font,
+    const FontDisplay display,
     bool isTriggered) {
   CHECK_NE(FromUnknown, m_dataSource);
 
@@ -403,8 +409,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/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