[M90 Merge] Make stability data from last session have correct CCT st…

…ate. Saves the CCT state from each session to Local State and uses it in the initial stability log next session. Also cleans up some redundant tracking of custom tab state, since it is entirely derivable from activity type. (cherry picked from commit 43fd90d) Bug: 1183468 Change-Id: Ie2c4be9aeb2b0aaf9dfc3d23f30758ee1e390f17 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2725765 Reviewed-by: Steven Holte <holte@chromium.org> Reviewed-by: Peter Conn <peconn@chromium.org> Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#859519} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2742794 Bot-Commit: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/branch-heads/4430@{#230} Cr-Branched-From: e5ce7dc-refs/heads/master@{#857950}
chromium · Mar 8, 2021 · 087b81b · 087b81b
1 parent 4d073f8
commit 087b81b
Show file tree

Hide file tree

Showing 8 changed files with 82 additions and 31 deletions.
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/app/ChromeActivity.java b/chrome/android/java/src/org/chromium/chrome/browser/app/ChromeActivity.java
@@ -1034,7 +1034,6 @@ public void onResumeWithNative() {
             if (webContents != null) webContents.notifyRendererPreferenceUpdate();
         }
 
-        ChromeSessionState.setCustomTabVisible(isCustomTab());
         ChromeSessionState.setActivityType(getActivityType());
         ChromeSessionState.setIsInMultiWindowMode(
                 MultiWindowUtils.getInstance().isInMultiWindowMode(this));

diff --git a/chrome/browser/flags/android/chrome_session_state.cc b/chrome/browser/flags/android/chrome_session_state.cc
@@ -4,23 +4,33 @@
 
 #include "chrome/browser/flags/android/chrome_session_state.h"
 
+#include "base/notreached.h"
 #include "chrome/browser/flags/jni_headers/ChromeSessionState_jni.h"
-
 #include "services/metrics/public/cpp/ukm_source.h"
 
 using chrome::android::ActivityType;
 
 namespace {
-bool custom_tab_visible = false;
 ActivityType activity_type = ActivityType::kTabbed;
 bool is_in_multi_window_mode = false;
 }  // namespace
 
 namespace chrome {
 namespace android {
 
-CustomTabsVisibilityHistogram GetCustomTabsVisibleValue() {
-  return custom_tab_visible ? VISIBLE_CUSTOM_TAB : VISIBLE_CHROME_TAB;
+CustomTabsVisibilityHistogram GetCustomTabsVisibleValue(
+    ActivityType activity_type) {
+  switch (activity_type) {
+    case ActivityType::kTabbed:
+    case ActivityType::kWebapp:
+    case ActivityType::kWebApk:
+      return VISIBLE_CHROME_TAB;
+    case ActivityType::kCustomTab:
+    case ActivityType::kTrustedWebActivity:
+      return VISIBLE_CUSTOM_TAB;
+  }
+  NOTREACHED();
+  return VISIBLE_CHROME_TAB;
 }
 
 ActivityType GetActivityType() {
@@ -34,15 +44,12 @@ bool GetIsInMultiWindowModeValue() {
 }  // namespace android
 }  // namespace chrome
 
-static void JNI_ChromeSessionState_SetCustomTabVisible(JNIEnv* env,
-                                                       jboolean visible) {
-  custom_tab_visible = visible;
-  ukm::UkmSource::SetCustomTabVisible(visible);
-}
-
 static void JNI_ChromeSessionState_SetActivityType(JNIEnv* env, jint type) {
   activity_type = static_cast<ActivityType>(type);
   // TODO(peconn): Look into adding this for UKM as well.
+  ukm::UkmSource::SetCustomTabVisible(
+      GetCustomTabsVisibleValue(activity_type) ==
+      chrome::android::VISIBLE_CUSTOM_TAB);
 }
 
 static void JNI_ChromeSessionState_SetIsInMultiWindowMode(

diff --git a/chrome/browser/flags/android/chrome_session_state.h b/chrome/browser/flags/android/chrome_session_state.h
@@ -13,7 +13,7 @@ namespace android {
 enum CustomTabsVisibilityHistogram {
   VISIBLE_CUSTOM_TAB,
   VISIBLE_CHROME_TAB,
-  CUSTOM_TABS_VISIBILITY_MAX
+  kMaxValue = VISIBLE_CHROME_TAB,
 };
 
 // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.flags
@@ -26,7 +26,7 @@ enum class ActivityType {
   kMaxValue = kWebApk,
 };
 
-CustomTabsVisibilityHistogram GetCustomTabsVisibleValue();
+CustomTabsVisibilityHistogram GetCustomTabsVisibleValue(ActivityType type);
 
 ActivityType GetActivityType();
 

diff --git a/.../browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeSessionState.java b/.../browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeSessionState.java
@@ -10,14 +10,6 @@
  * Stores high-level state about a session for metrics logging.
  */
 public class ChromeSessionState {
-    /**
-     * Records the current custom tab visibility state with native-side feature utilities.
-     * @param visible Whether a custom tab is visible.
-     */
-    public static void setCustomTabVisible(boolean visible) {
-        ChromeSessionStateJni.get().setCustomTabVisible(visible);
-    }
-
     /**
      * Records whether the activity is in multi-window mode with native-side feature utilities.
      * @param isInMultiWindowMode Whether the activity is in Android N multi-window mode.
@@ -36,7 +28,6 @@ public static void setActivityType(@ActivityType int activityType) {
 
     @NativeMethods
     interface Natives {
-        void setCustomTabVisible(boolean visible);
         void setActivityType(@ActivityType int type);
         void setIsInMultiWindowMode(boolean isInMultiWindowMode);
     }

diff --git a/chrome/browser/metrics/chrome_android_metrics_provider.cc b/chrome/browser/metrics/chrome_android_metrics_provider.cc
@@ -5,13 +5,36 @@
 #include "chrome/browser/metrics/chrome_android_metrics_provider.h"
 
 #include "base/metrics/histogram_macros.h"
+#include "base/optional.h"
 #include "chrome/android/chrome_jni_headers/NotificationSystemStatusUtil_jni.h"
 #include "chrome/browser/android/locale/locale_manager.h"
 #include "chrome/browser/android/metrics/uma_session_stats.h"
 #include "chrome/browser/flags/android/chrome_session_state.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
 
 namespace {
 
+// Name of local state pref to persist the last |chrome::android::ActivityType|.
+const char kLastActivityTypePref[] =
+    "user_experience_metrics.last_activity_type";
+
+base::Optional<chrome::android::ActivityType> GetActivityTypeFromLocalState(
+    PrefService* local_state_) {
+  auto value = local_state_->GetInteger(kLastActivityTypePref);
+  if (value >= static_cast<int>(chrome::android::ActivityType::kTabbed) &&
+      value <= static_cast<int>(chrome::android::ActivityType::kMaxValue)) {
+    return static_cast<chrome::android::ActivityType>(value);
+  }
+  return base::nullopt;
+}
+
+void EmitActivityTypeHistograms(chrome::android::ActivityType type) {
+  UMA_STABILITY_HISTOGRAM_ENUMERATION(
+      "CustomTabs.Visible", chrome::android::GetCustomTabsVisibleValue(type));
+  UMA_STABILITY_HISTOGRAM_ENUMERATION("Android.ChromeActivity.Type", type);
+}
+
 // Corresponds to APP_NOTIFICATIONS_STATUS_BOUNDARY in
 // NotificationSystemStatusUtil.java
 const int kAppNotificationStatusBoundary = 3;
@@ -25,17 +48,30 @@ void EmitAppNotificationStatusHistogram() {
 
 }  // namespace
 
-ChromeAndroidMetricsProvider::ChromeAndroidMetricsProvider() {}
+ChromeAndroidMetricsProvider::ChromeAndroidMetricsProvider(
+    PrefService* local_state)
+    : local_state_(local_state) {}
 
 ChromeAndroidMetricsProvider::~ChromeAndroidMetricsProvider() {}
 
+// static
+void ChromeAndroidMetricsProvider::RegisterPrefs(PrefRegistrySimple* registry) {
+  // Register with a default value of -1 which is not a valid enum.
+  registry->RegisterIntegerPref(kLastActivityTypePref, -1);
+}
+
 void ChromeAndroidMetricsProvider::OnDidCreateMetricsLog() {
-  UMA_HISTOGRAM_ENUMERATION("CustomTabs.Visible",
-                            chrome::android::GetCustomTabsVisibleValue(),
-                            chrome::android::CUSTOM_TABS_VISIBILITY_MAX);
+  auto type = chrome::android::GetActivityType();
+  EmitActivityTypeHistograms(type);
+  // Save the value off for reporting stability metrics.
+  local_state_->SetInteger(kLastActivityTypePref, static_cast<int>(type));
+}
 
-  UMA_HISTOGRAM_ENUMERATION("Android.ChromeActivity.Type",
-                            chrome::android::GetActivityType());
+void ChromeAndroidMetricsProvider::ProvidePreviousSessionData(
+    metrics::ChromeUserMetricsExtension* uma_proto) {
+  auto activity_type = GetActivityTypeFromLocalState(local_state_);
+  if (activity_type.has_value())
+    EmitActivityTypeHistograms(activity_type.value());
 }
 
 void ChromeAndroidMetricsProvider::ProvideCurrentSessionData(

diff --git a/chrome/browser/metrics/chrome_android_metrics_provider.h b/chrome/browser/metrics/chrome_android_metrics_provider.h
@@ -12,20 +12,30 @@ namespace metrics {
 class ChromeUserMetricsExtension;
 }
 
+class PrefService;
+class PrefRegistrySimple;
+
 // ChromeAndroidMetricsProvider provides Chrome-for-Android-specific stability
 // metrics. Android-specific metrics which apply to lower layers should be
 // implemented in metrics::AndroidMetricsProvider.
 class ChromeAndroidMetricsProvider : public metrics::MetricsProvider {
  public:
-  ChromeAndroidMetricsProvider();
+  explicit ChromeAndroidMetricsProvider(PrefService* local_state);
   ~ChromeAndroidMetricsProvider() override;
 
+  // Registers local state prefs used by this class.
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
   // metrics::MetricsProvider:
   void OnDidCreateMetricsLog() override;
+  void ProvidePreviousSessionData(
+      metrics::ChromeUserMetricsExtension* uma_proto) override;
   void ProvideCurrentSessionData(
       metrics::ChromeUserMetricsExtension* uma_proto) override;
 
  private:
+  PrefService* local_state_;
+
   DISALLOW_COPY_AND_ASSIGN(ChromeAndroidMetricsProvider);
 };
 

diff --git a/chrome/browser/metrics/chrome_android_metrics_provider_unittest.cc b/chrome/browser/metrics/chrome_android_metrics_provider_unittest.cc
@@ -5,15 +5,19 @@
 #include "chrome/browser/metrics/chrome_android_metrics_provider.h"
 
 #include "base/test/metrics/histogram_tester.h"
+#include "components/prefs/testing_pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 class ChromeAndroidMetricsProviderTest : public testing::Test {
  public:
-  ChromeAndroidMetricsProviderTest() = default;
+  ChromeAndroidMetricsProviderTest() : metrics_provider_(&pref_service_) {
+    ChromeAndroidMetricsProvider::RegisterPrefs(pref_service_.registry());
+  }
   ~ChromeAndroidMetricsProviderTest() override = default;
 
  protected:
   base::HistogramTester histogram_tester_;
+  TestingPrefServiceSimple pref_service_;
   ChromeAndroidMetricsProvider metrics_provider_;
 };
 

diff --git a/chrome/browser/metrics/chrome_metrics_service_client.cc b/chrome/browser/metrics/chrome_metrics_service_client.cc
@@ -476,6 +476,10 @@ void ChromeMetricsServiceClient::RegisterPrefs(PrefRegistrySimple* registry) {
 
   metrics::RegisterMetricsReportingStatePrefs(registry);
 
+#if defined(OS_ANDROID)
+  ChromeAndroidMetricsProvider::RegisterPrefs(registry);
+#endif  // defined(OS_ANDROID)
+
 #if BUILDFLAG(ENABLE_PLUGINS)
   PluginMetricsProvider::RegisterPrefs(registry);
 #endif  // BUILDFLAG(ENABLE_PLUGINS)
@@ -688,7 +692,7 @@ void ChromeMetricsServiceClient::RegisterMetricsServiceProviders() {
   metrics_service_->RegisterMetricsProvider(
       std::make_unique<metrics::AndroidMetricsProvider>());
   metrics_service_->RegisterMetricsProvider(
-      std::make_unique<ChromeAndroidMetricsProvider>());
+      std::make_unique<ChromeAndroidMetricsProvider>(local_state));
   metrics_service_->RegisterMetricsProvider(
       std::make_unique<PageLoadMetricsProvider>());
 #endif  // defined(OS_ANDROID)