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,

[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.
   // 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 130 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,
+    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(
     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 138 matching lines @@
       return Miss;
     case FromUnknown:
     // Fall through.
     default:
       NOTREACHED();
   }
   return Miss;
 }
 
 }  // namespace blink