Add more UMA in CopylessPasteExtractor

third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp

 // Copyright 2017 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 "modules/document_metadata/CopylessPasteExtractor.h"
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 
@@ skipping 229 matching lines @@
 
 void extractEntityFromTopLevelObject(const JSONObject& val,
                                      Vector<EntityPtr>& entities) {
   const JSONArray* graph = val.GetArray(kJSONLDKeyGraph);
   if (graph) {
     extractEntitiesFromArray(*graph, entities);
   }
   extractTopLevelEntity(val, entities);
 }
 
-bool extractMetadata(const Element& root, Vector<EntityPtr>& entities) {
+// kCount must be the last entry.

[Ilya Sherman, 2017/04/12 23:25:55]

nit: Please document that this enum is used to back an UMA histogram, and should
therefore be treated as append-only.

[wychen, 2017/04/13 00:47:54]

Done.

+enum ExtractionStatus { kOK, kEmpty, kParseFailure, kWrongType, kCount };

[Ilya Sherman, 2017/04/12 23:25:55]

nit: Could this be an enum class?

[wychen, 2017/04/12 23:40:04]

Great suggestion. The UMA API in base/ supports enum class, but the one in Blink
doesn't. static_cast'ing all over doesn't look nice.
I heard this would change soon, but I don't want to block this CL.

+
+ExtractionStatus extractMetadata(const Element& root,
+                                 Vector<EntityPtr>& entities) {
   for (Element& element : ElementTraversal::DescendantsOf(root)) {
     if (element.HasTagName(HTMLNames::scriptTag) &&
         element.getAttribute(HTMLNames::typeAttr) == "application/ld+json") {
       std::unique_ptr<JSONValue> json = ParseJSON(element.textContent());
       if (!json) {
         LOG(ERROR) << "Failed to parse json.";
-        return false;
+        return kParseFailure;
       }
       switch (json->GetType()) {
         case JSONValue::ValueType::kTypeArray:
           extractEntitiesFromArray(*(JSONArray::Cast(json.get())), entities);
           break;
         case JSONValue::ValueType::kTypeObject:
           extractEntityFromTopLevelObject(*(JSONObject::Cast(json.get())),
                                           entities);
           break;
         default:
-          return false;
+          return kWrongType;
       }
     }
   }
-  return !entities.IsEmpty();
+  if (entities.IsEmpty()) {
+    return kEmpty;
+  }
+  return kOK;
 }
 
 }  // namespace
 
 WebPagePtr CopylessPasteExtractor::extract(const Document& document) {
   TRACE_EVENT0("blink", "CopylessPasteExtractor::extract");
 
   if (!document.GetFrame() || !document.GetFrame()->IsMainFrame())
     return nullptr;
 
   Element* html = document.documentElement();
   if (!html)
     return nullptr;
 
-  double start_time = MonotonicallyIncreasingTime();
-
   WebPagePtr page = WebPage::New();
 
   // Traverse the DOM tree and extract the metadata.
-  if (!extractMetadata(*html, page->entities))
+  double start_time = MonotonicallyIncreasingTime();
+  ExtractionStatus status = extractMetadata(*html, page->entities);
+  double elapsed_time = MonotonicallyIncreasingTime() - start_time;
+
+  DEFINE_STATIC_LOCAL(EnumerationHistogram, status_histogram,
+                      ("CopylessPaste.ExtractionStatus", kCount));
+  status_histogram.Count(status);
+
+  if (status != kOK) {
+    DEFINE_STATIC_LOCAL(CustomCountHistogram, extractionHistogram,
+                        ("CopylessPaste.ExtractionFailedUs", 1, 1000000, 50));

[Ilya Sherman, 2017/04/12 23:25:55]

Optional nit: Could you please write 1000 * 1000 instead of 1000000, for ease of
visual parsing?  Applies below as well.

[wychen, 2017/04/13 00:47:54]

Done.

+    extractionHistogram.Count(1e6 * elapsed_time);
     return nullptr;
+  }
+  DEFINE_STATIC_LOCAL(CustomCountHistogram, extractionHistogram,
+                      ("CopylessPaste.ExtractionUs", 1, 1000000, 50));
+  extractionHistogram.Count(1e6 * elapsed_time);
+
   page->url = document.Url();
   page->title = document.title();
-
-  double elapsed_time = MonotonicallyIncreasingTime() - start_time;
-
-  DEFINE_STATIC_LOCAL(CustomCountHistogram, extractionHistogram,
-                      ("CopylessPaste.ExtractionUs", 1, 1000000, 50));
-  extractionHistogram.Count(static_cast<int>(1e6 * elapsed_time));
   return page;
 }
 
 }  // namespace blink