Skip to content

GitLab

T
tangled
Commit 92ee9e82 authored by Danan S's avatar Danan S Committed by Commit Bot
Browse files
Options

Revert "Added UMA metric to count unique apps launched during Demo Mode." 

This reverts commit e775e110.

Reason for revert: This is inconsistently crashing demo mode
when Android apps are launched on certain devices.

Original change's description:
> Added UMA metric to count unique apps launched during Demo Mode.
>
> Bug: 904564
> Change-Id: I653438bf5244bc98bcb742d9d986b4ea2bd4b40a
> Reviewed-on: https://chromium-review.googlesource.com/c/1334563
> Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
> Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
> Reviewed-by: Brian White <bcwhite@chromium.org>
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#609961}

TBR=oshima@chromium.org,michaelpg@chromium.org,bcwhite@chromium.org,danan@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 917156
Change-Id: I7e8d88541ad5f74fda720425c05816f48e4ba4a8
Reviewed-on: https://chromium-review.googlesource.com/c/1387953Reviewed-by: default avatarMichael Giuffrida <michaelpg@chromium.org>
Reviewed-by: default avatarJames Cook <jamescook@chromium.org>
Commit-Queue: Danan S <danan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619542}
parent d7cf3101
Hide whitespace changes
Inline Side-by-side
Showing with 9 additions and 172 deletions
ash/metrics/demo_session_metrics_recorder.cc
View file @ 92ee9e82
......@@ -37,26 +37,6 @@ constexpr auto kSamplePeriod = base::TimeDelta::FromSeconds(1);
constexpr int kMaxPeriodsWithoutActivity =
base::TimeDelta::FromSeconds(15) / kSamplePeriod;
// Returns a package name for an ARC window. The returned value is unowned and
// may be null.
const std::string* GetPackageNameForArcWindow(const aura::Window* window) {
// Make sure this is an ARC app window.
DCHECK(static_cast<ash::AppType>(window->GetProperty(
aura::client::kAppType)) == ash::AppType::ARC_APP);
// We use a dedicated key for instead of kShelfIDKey for identifiying ARC++
// apps. The ShelfID app id isn't used to identify ARC++ apps since it's a
// hash of both the package name and the activity.
return static_cast<std::string*>(window->GetProperty(kArcPackageNameKey));
}
// Returns the app ID For a non-ARC window. This function crashes if the
// window is an ARC window.
std::string GetAppIdForWindow(const aura::Window* window) {
DCHECK(static_cast<ash::AppType>(window->GetProperty(
aura::client::kAppType)) != ash::AppType::ARC_APP);
return ShelfID::Deserialize(window->GetProperty(kShelfIDKey)).app_id;
}
// Maps a Chrome app ID to a DemoModeApp value for metrics.
DemoModeApp GetAppFromAppId(const std::string& app_id) {
// Each version of the Highlights app is bucketed into the same value.
......@@ -111,12 +91,17 @@ DemoModeApp GetAppFromWindow(const aura::Window* window) {
ash::AppType app_type =
static_cast<ash::AppType>(window->GetProperty(aura::client::kAppType));
if (app_type == ash::AppType::ARC_APP) {
const std::string* package_name = GetPackageNameForArcWindow(window);
// The ShelfID app id isn't used to identify ARC++ apps since it's a hash of
// both the package name and the activity.
const std::string* package_name =
static_cast<std::string*>(window->GetProperty(kArcPackageNameKey));
return package_name ? GetAppFromPackageName(*package_name)
: DemoModeApp::kOtherArcApp;
}
std::string app_id = GetAppIdForWindow(window);
std::string app_id =
ShelfID::Deserialize(window->GetProperty(kShelfIDKey)).app_id;
// The Chrome "app" in the shelf is just the browser.
if (app_id == extension_misc::kChromeAppId)
return DemoModeApp::kBrowser;
......@@ -151,29 +136,13 @@ DemoSessionMetricsRecorder::DemoSessionMetricsRecorder(
timer_ = std::make_unique<base::RepeatingTimer>();
StartRecording();
// Listen for user activity events.
observer_.Add(ui::UserActivityDetector::Get());
// Listen for Window activation events.
Shell::Get()->activation_client()->AddObserver(this);
}
DemoSessionMetricsRecorder::~DemoSessionMetricsRecorder() {
// Report any remaining stored samples on exit. (If the user went idle, there
// won't be any.)
ReportSamples();
// Stop listening for window activation events.
Shell::Get()->activation_client()->RemoveObserver(this);
// Report the number of unique apps launched since the last
// time we reported, and when the demo session ends. Only
// do this if there are any entries in the set, because an idle
// session that was shut down can result in erroneous
// sample stating that 0 unique apps were launched.
if (unique_apps_launched_.size() > 0)
ReportUniqueAppsLaunched();
}
void DemoSessionMetricsRecorder::OnUserActivity(const ui::Event* event) {
......@@ -186,33 +155,6 @@ void DemoSessionMetricsRecorder::OnUserActivity(const ui::Event* event) {
periods_since_activity_ = 0;
}
void DemoSessionMetricsRecorder::OnWindowActivated(ActivationReason reason,
aura::Window* gained_active,
aura::Window* lost_active) {
if (gained_active == nullptr)
return;
// Don't count popup windows.
if (gained_active->type() != aura::client::WINDOW_TYPE_NORMAL)
return;
// Track unique apps opened. There is no namespace collision between
// ARC apps and ChromeOS Apps because ARC app package names use a different
// naming scheme than ChromeOS Apps.
std::string unique_app_id;
ash::AppType app_type = static_cast<ash::AppType>(
gained_active->GetProperty(aura::client::kAppType));
if (app_type == ash::AppType::ARC_APP) {
const std::string* package_name = GetPackageNameForArcWindow(gained_active);
unique_app_id = *package_name;
} else {
unique_app_id = GetAppIdForWindow(gained_active);
}
unique_apps_launched_.insert(unique_app_id);
}
void DemoSessionMetricsRecorder::StartRecording() {
timer_->Start(FROM_HERE, kSamplePeriod, this,
&DemoSessionMetricsRecorder::TakeSampleOrPause);
......@@ -224,10 +166,6 @@ void DemoSessionMetricsRecorder::TakeSampleOrPause() {
// These samples were collected since the last user activity.
unreported_samples_.clear();
timer_->Stop();
// Since we are assuming that the user left, report how many
// unique apps have been launched since we last reported.
ReportUniqueAppsLaunched();
return;
}
......@@ -248,10 +186,4 @@ void DemoSessionMetricsRecorder::ReportSamples() {
unreported_samples_.clear();
}
void DemoSessionMetricsRecorder::ReportUniqueAppsLaunched() {
UMA_HISTOGRAM_COUNTS_100("DemoMode.UniqueAppsLaunched",
unique_apps_launched_.size());
unique_apps_launched_.clear();
}
} // namespace ash
ash/metrics/demo_session_metrics_recorder.h
View file @ 92ee9e82
......@@ -6,15 +6,12 @@
#define ASH_METRICS_DEMO_SESSION_METRICS_RECORDER_H_
#include <memory>
#include <string>
#include <vector>
#include "ash/ash_export.h"
#include "base/containers/flat_set.h"
#include "base/macros.h"
#include "base/scoped_observer.h"
#include "ui/base/user_activity/user_activity_observer.h"
#include "ui/wm/public/activation_change_observer.h"
namespace base {
class RepeatingTimer;
......@@ -28,9 +25,7 @@ namespace ash {
// A metrics recorder for demo sessions that samples the active window's app or
// window type. Only used when the device is in Demo Mode.
class ASH_EXPORT DemoSessionMetricsRecorder
: public ui::UserActivityObserver,
public wm::ActivationChangeObserver {
class ASH_EXPORT DemoSessionMetricsRecorder : public ui::UserActivityObserver {
public:
// These apps are preinstalled in Demo Mode. This list is not exhaustive, and
// includes first- and third-party Chrome and ARC apps.
......@@ -71,11 +66,6 @@ class ASH_EXPORT DemoSessionMetricsRecorder
// ui::UserActivityObserver:
void OnUserActivity(const ui::Event* event) override;
// wm::ActivationChangeObserver:
void OnWindowActivated(ActivationReason reason,
aura::Window* gained_active,
aura::Window* lost_active) override;
private:
// Starts the timer for periodic sampling.
void StartRecording();
......@@ -87,16 +77,10 @@ class ASH_EXPORT DemoSessionMetricsRecorder
// Emits histograms for recorded samples.
void ReportSamples();
// Emits histograms for the number of unique apps launched.
void ReportUniqueAppsLaunched();
// Stores samples as they are collected. Report to UMA if we see user
// activity soon after. Guaranteed not to grow too large.
std::vector<DemoModeApp> unreported_samples_;
// Tracks the ids of apps that have been launched in Demo Mode.
base::flat_set<std::string> unique_apps_launched_;
// How many periods have elapsed since the last user activity.
int periods_since_activity_ = 0;
......@@ -110,4 +94,4 @@ class ASH_EXPORT DemoSessionMetricsRecorder
} // namespace ash
#endif // ASH_METRICS_DEMO_SESSION_METRICS_RECORDER_H_
#endif // ASH_METRICS_POINTER_METRICS_RECORDER_H_
ash/metrics/demo_session_metrics_recorder_unittest.cc
View file @ 92ee9e82
......@@ -445,84 +445,5 @@ TEST_F(DemoSessionMetricsRecorderTest, IgnoreOnIdleSession) {
histogram_tester_->ExpectTotalCount("DemoMode.ActiveApp", 0);
}
TEST_F(DemoSessionMetricsRecorderTest, UniqueAppsLaunchedOnIdle) {
// Activate each window twice. Despite activating each twice,
// the count should only be incremented once per unique app.
auto chrome_app_window = CreateChromeAppWindow(extension_misc::kCameraAppId);
wm::ActivateWindow(chrome_app_window.get());
wm::DeactivateWindow(chrome_app_window.get());
wm::ActivateWindow(chrome_app_window.get());
auto chrome_browser_window =
CreateChromeAppWindow(extension_misc::kChromeAppId);
wm::ActivateWindow(chrome_browser_window.get());
wm::DeactivateWindow(chrome_browser_window.get());
wm::ActivateWindow(chrome_browser_window.get());
auto arc_window_1 = CreateArcWindow("com.google.Photos");
wm::ActivateWindow(arc_window_1.get());
wm::DeactivateWindow(arc_window_1.get());
wm::ActivateWindow(arc_window_1.get());
auto arc_window_2 = CreateArcWindow("com.google.Maps");
wm::ActivateWindow(arc_window_2.get());
wm::DeactivateWindow(arc_window_2.get());
wm::ActivateWindow(arc_window_2.get());
// Popup windows shouldn't be counted at all.
auto popup_window = CreatePopupWindow();
wm::ActivateWindow(popup_window.get());
wm::DeactivateWindow(popup_window.get());
wm::ActivateWindow(popup_window.get());
for (int i = 0; i < 20; i++)
FireTimer();
histogram_tester_->ExpectUniqueSample("DemoMode.UniqueAppsLaunched", 4, 1);
}
TEST_F(DemoSessionMetricsRecorderTest, UniqueAppsLaunchedOnDeletion) {
// Activate each window twice. Despite activating each twice,
// the count should only be incremented once per unique app.
auto chrome_app_window = CreateChromeAppWindow(extension_misc::kCameraAppId);
wm::ActivateWindow(chrome_app_window.get());
wm::DeactivateWindow(chrome_app_window.get());
wm::ActivateWindow(chrome_app_window.get());
auto chrome_browser_window =
CreateChromeAppWindow(extension_misc::kChromeAppId);
wm::ActivateWindow(chrome_browser_window.get());
wm::DeactivateWindow(chrome_browser_window.get());
wm::ActivateWindow(chrome_browser_window.get());
auto arc_window_1 = CreateArcWindow("com.google.Photos");
wm::ActivateWindow(arc_window_1.get());
wm::DeactivateWindow(arc_window_1.get());
wm::ActivateWindow(arc_window_1.get());
auto arc_window_2 = CreateArcWindow("com.google.Maps");
wm::ActivateWindow(arc_window_2.get());
wm::DeactivateWindow(arc_window_2.get());
wm::ActivateWindow(arc_window_2.get());
// Popup windows shouldn't be counted at all.
auto popup_window = CreatePopupWindow();
wm::ActivateWindow(popup_window.get());
wm::DeactivateWindow(popup_window.get());
wm::ActivateWindow(popup_window.get());
DeleteMetricsRecorder();
histogram_tester_->ExpectUniqueSample("DemoMode.UniqueAppsLaunched", 4, 1);
}
TEST_F(DemoSessionMetricsRecorderTest, NoUniqueAppsLaunchedOnDeletion) {
DeleteMetricsRecorder();
// There should be no samples if the recorder is deleted with 0 unique apps
// launched.
histogram_tester_->ExpectTotalCount("DemoMode.ReportUniqueAppsLaunched", 0);
}
} // namespace
} // namespace ash
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment