[Manifest] Add metrics recording.

content/renderer/manifest/manifest_uma_util.cc

Index: content/renderer/manifest/manifest_uma_util.cc
diff --git a/content/renderer/manifest/manifest_uma_util.cc b/content/renderer/manifest/manifest_uma_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..bb8a1c4c2bb7e3301d6f04aa34aaafc4699769cd
--- /dev/null
+++ b/content/renderer/manifest/manifest_uma_util.cc
@@ -0,0 +1,68 @@
+// 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 "content/renderer/manifest/manifest_uma_util.h"
+
+#include "base/metrics/histogram.h"
+#include "content/public/common/manifest.h"
+
+namespace content {
+
+namespace {
+
+enum ManifestFetchResultType {

[Mark P, 2014/10/03 19:57:49]

Please put the standard warning here that this enum is recorded in UMA logs and
hence you cannot delete or reorder or renumber any elements; new elements should
only be placed at the end.

Also please explicitly do = whatever for each element.

[mlamouri (slow - plz ping), 2014/10/07 11:11:39]

Done.

+  MANIFEST_FETCH_SUCCESS = 0,
+  MANIFEST_FETCH_ERROR_EMPTY_URL,
+  MANIFEST_FETCH_ERROR_UNSPECIFIED,
+  MANIFEST_FETCH_RESULT_TYPE_COUNT
+};
+
+} // anonymous namespace
+
+void ManifestUmaUtil::ParseSucceeded(const Manifest& manifest) {
+  UMA_HISTOGRAM_BOOLEAN("Manifest.ParseSuccess", true);
+  UMA_HISTOGRAM_BOOLEAN("Manifest.IsEmpty", manifest.IsEmpty());
+  if (manifest.IsEmpty())
+    return;
+
+  UMA_HISTOGRAM_BOOLEAN("Manifest.name", !manifest.name.is_null());
+  UMA_HISTOGRAM_BOOLEAN("Manifest.short_name", !manifest.short_name.is_null());
+  UMA_HISTOGRAM_BOOLEAN("Manifest.start_url", !manifest.start_url.is_empty());
+  UMA_HISTOGRAM_BOOLEAN("Manifest.display",
+      manifest.display != Manifest::DISPLAY_MODE_UNSPECIFIED);
+  UMA_HISTOGRAM_BOOLEAN("Manifest.orientation",
+      manifest.orientation != blink::WebScreenOrientationLockDefault);
+  UMA_HISTOGRAM_BOOLEAN("Manifest.icons", !manifest.icons.empty());
+  UMA_HISTOGRAM_BOOLEAN("Manifest.gcm_sender_id",
+      !manifest.gcm_sender_id.is_null());
+}
+
+void ManifestUmaUtil::ParseFailed() {
+  UMA_HISTOGRAM_BOOLEAN("Manifest.ParseSuccess", false);

[Mark P, 2014/10/03 19:57:49]

You use this name in two places.  Consider making it a local-file const char [].

[mlamouri (slow - plz ping), 2014/10/07 11:11:39]

Done.

+}
+
+void ManifestUmaUtil::FetchSucceeded() {
+  UMA_HISTOGRAM_ENUMERATION("Manifest.FetchResult",

[Mark P, 2014/10/03 19:57:49]

ditto for this histogram name.

[mlamouri (slow - plz ping), 2014/10/07 11:11:38]

Done.

+                            MANIFEST_FETCH_SUCCESS,
+                            MANIFEST_FETCH_RESULT_TYPE_COUNT);
+}
+
+void ManifestUmaUtil::FetchFailed(FetchFailureReason reason) {
+  ManifestFetchResultType fetch_result_type = MANIFEST_FETCH_RESULT_TYPE_COUNT;
+  switch (reason) {
+    case FETCH_EMPTY_URL:
+      fetch_result_type = MANIFEST_FETCH_ERROR_EMPTY_URL;

[Miguel Garcia, 2014/10/02 15:56:42]

why do you want two enums? one for uma and one externally? can't you simply move
this one to the .h file and use it instead? that way you avoid the switch here

[mlamouri (slow - plz ping), 2014/10/02 16:10:55]

One enum is the UMA enum, it's an implementation detail. Consumers of that
helper class shouldn't have to worry about it. The UMA enums has SUCCESS/FAILURE
while the exposed enum only has the failures information.

+      break;
+    case FETCH_UNSPECIFIED_REASON:
+      fetch_result_type = MANIFEST_FETCH_ERROR_UNSPECIFIED;
+      break;
+  }
+  DCHECK_NE(fetch_result_type, MANIFEST_FETCH_RESULT_TYPE_COUNT);
+
+  UMA_HISTOGRAM_ENUMERATION("Manifest.FetchResult",
+                            fetch_result_type,
+                            MANIFEST_FETCH_RESULT_TYPE_COUNT);
+}
+
+} // namespace content