Report frequency of single page app navigations to UMA

third_party/WebKit/Source/core/loader/FrameLoader.cpp

Index: third_party/WebKit/Source/core/loader/FrameLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index 23b058dc70155aed38401d5d15f0860ebd8d6adb..46e2dc304755d048719b736d737f532babceb1c6 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -79,6 +79,7 @@
 #include "core/probe/CoreProbes.h"
 #include "core/svg/graphics/SVGImage.h"
 #include "core/xml/parser/XMLDocumentParser.h"
+#include "platform/Histogram.h"
 #include "platform/InstanceCounters.h"
 #include "platform/PluginScriptForbiddenScope.h"
 #include "platform/ScriptForbiddenScope.h"
@@ -484,6 +485,45 @@ void FrameLoader::UpdateForSameDocumentNavigation(
     HistoryScrollRestorationType scroll_restoration_type,
     FrameLoadType type,
     Document* initiating_document) {
+  // This enum is used to index different kinds of single-page-application

[Nate Chapin, 2017/06/21 18:05:24]

Could we put this all of this logging in a helper? It seems like it's
complicated enough it deserves its own function.

[Liquan (Max) Gu, 2017/06/26 14:52:21]

By 'all of this logging' do you mean the big switch and
UMA_HISTOGRAM_ENUMERATION()? It looks weird to me to mix the logging logic and
categorizing logic in a helper function. Helper function with only categorizing
logic seems more normal? Let's say, a single_page_application_helper class with
FindSinglePageAppNavigationType()?

+  // navigations for UMA enum histogram. New enum values can be added, but
+  // existing enums must never be renumbered or deleted and reused.
+  // This enum should be consistent with SinglePageAppNavigationType in
+  // tools/metrics/histograms/enums.
+  //
+  // |SinglePageAppNavigationType| falls into this grid according to different
+  // combinations of |FrameLoadType| and |SameDocumentNavigationSource|:
+  //
+  //                              HistoryApi    Default
+  //  kFrameLoadTypeBackForward    illegal         1
+  // !kFrameLoadTypeBackForward       0            2
+  enum SinglePageAppNavigationType {

[tdresser, 2017/06/21 14:45:00]

We'd normally define the enum outside of this method. This would let you refer
to the enum values in the test for example, which will make the test a bit
clearer.

[Liquan (Max) Gu, 2017/06/26 14:52:21]

Good idea.

+    kSPANavTypeHistoryPushStateOrReplaceState = 0,
+    kSPANavTypeSameDocumentBackwardOrForward = 1,
+    kSPANavTypeOtherFragmentNavigation = 2,
+    kSPANavTypeCount
+  };
+  SinglePageAppNavigationType single_page_app_navigation_type;
+  switch (same_document_navigation_source) {
+    case kSameDocumentNavigationDefault:
+      if (type == kFrameLoadTypeBackForward) {
+        single_page_app_navigation_type =
+            kSPANavTypeSameDocumentBackwardOrForward;
+      } else {
+        single_page_app_navigation_type = kSPANavTypeOtherFragmentNavigation;
+      }
+      break;
+    case kSameDocumentNavigationHistoryApi:
+      // It's illegal to have both kSameDocumentNavigationHistoryApi and
+      // kFrameLoadTypeBackForward.
+      DCHECK(type != kFrameLoadTypeBackForward);
+      single_page_app_navigation_type =
+          kSPANavTypeHistoryPushStateOrReplaceState;
+  }
+  UMA_HISTOGRAM_ENUMERATION(
+      "RendererScheduler.UpdateForSameDocumentNavigationCount",
+      single_page_app_navigation_type, kSPANavTypeCount);
+
   TRACE_EVENT1("blink", "FrameLoader::updateForSameDocumentNavigation", "url",
                new_url.GetString().Ascii().data());