Skip to content

Commit

Permalink
cros: Add histogram for tracking desk lifetime.
Browse files Browse the repository at this point in the history
This CL adds a histogram for tracking desk lifetime. Desks now store
their creation time and emits their lifetime as a record when they are
removed. Additionally, there is a new user pref for persisting
creation times across sessions.

Test: manual
Bug: 1133391
Change-Id: I78a5e310a4bcfad57727de04adffc91b3af945c2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2712983
Commit-Queue: Jeremy Chinsen <chinsenj@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Caitlin Fischer <caitlinfischer@google.com>
Cr-Commit-Position: refs/heads/master@{#859839}
  • Loading branch information
chinsenj authored and Chromium LUCI CQ committed Mar 4, 2021
1 parent 57e04a0 commit 7936a65
Show file tree
Hide file tree
Showing 9 changed files with 146 additions and 18 deletions.
4 changes: 4 additions & 0 deletions ash/public/cpp/ash_pref_names.cc
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,10 @@ const char kContextualTooltips[] = "settings.contextual_tooltip.shown_info";
// name will appear in this list as an empty string. The desk names are stored
// as UTF8 strings.
const char kDesksNamesList[] = "ash.desks.desks_names_list";
// This list stores the metrics of virtual desks. Like |kDesksNamesList|, this
// list stores entries in the same order of the desks in the overview desks bar.
// Values are stored as dictionaries.
const char kDesksMetricsList[] = "ash.desks.desks_metrics_list";
// An integer index of a user's active desk.
const char kDesksActiveDesk[] = "ash.desks.active_desk";

Expand Down
1 change: 1 addition & 0 deletions ash/public/cpp/ash_pref_names.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ ASH_PUBLIC_EXPORT extern const char kAltTabPerDesk[];
ASH_PUBLIC_EXPORT extern const char kContextualTooltips[];

ASH_PUBLIC_EXPORT extern const char kDesksNamesList[];
ASH_PUBLIC_EXPORT extern const char kDesksMetricsList[];
ASH_PUBLIC_EXPORT extern const char kDesksActiveDesk[];

ASH_PUBLIC_EXPORT extern const char kDockedMagnifierEnabled[];
Expand Down
16 changes: 15 additions & 1 deletion ash/wm/desks/desk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "ash/wm/workspace_controller.h"
#include "base/containers/adapters.h"
#include "base/containers/contains.h"
#include "base/metrics/histogram_functions.h"
#include "base/stl_util.h"
#include "chromeos/ui/base/window_properties.h"
#include "ui/aura/client/aura_constants.h"
Expand All @@ -36,6 +37,9 @@ namespace ash {

namespace {

// Prefix for the desks lifetime histogram.
constexpr char kDeskLifetimeHistogramNamePrefix[] = "Ash.Desks.DeskLifetime_";

void UpdateBackdropController(aura::Window* desk_container) {
auto* workspace_controller = GetWorkspaceController(desk_container);
// Work might have already been cleared when the display is removed. See
Expand Down Expand Up @@ -180,7 +184,8 @@ class DeskContainerObserver : public aura::WindowObserver {
// Desk:

Desk::Desk(int associated_container_id)
: container_id_(associated_container_id) {
: container_id_(associated_container_id),
creation_time_(base::Time::Now()) {
// For the very first default desk added during initialization, there won't be
// any root windows yet. That's OK, OnRootWindowAdded() will be called
// explicitly by the RootWindowController when they're initialized.
Expand Down Expand Up @@ -489,6 +494,15 @@ void Desk::SetDeskBeingRemoved() {
is_desk_being_removed_ = true;
}

void Desk::RecordLifetimeHistogram() {
// Desk index is 1-indexed in histograms.
const int desk_index =
Shell::Get()->desks_controller()->GetDeskIndex(this) + 1;
base::UmaHistogramCounts1000(
base::StringPrintf("%s%i", kDeskLifetimeHistogramNamePrefix, desk_index),
(base::Time::Now() - creation_time_).InHours());
}

void Desk::MoveWindowToDeskInternal(aura::Window* window,
Desk* target_desk,
aura::Window* target_root) {
Expand Down
16 changes: 16 additions & 0 deletions ash/wm/desks/desk.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "base/macros.h"
#include "base/observer_list.h"
#include "base/strings/string16.h"
#include "base/time/time.h"
#include "ui/aura/window_observer.h"

namespace ash {
Expand Down Expand Up @@ -54,6 +55,8 @@ class ASH_EXPORT Desk {

const std::vector<aura::Window*>& windows() const { return windows_; }

const base::Time& creation_time() const { return creation_time_; }

const base::string16& name() const { return name_; }

bool is_active() const { return is_active_; }
Expand All @@ -66,6 +69,12 @@ class ASH_EXPORT Desk {

bool is_name_set_by_user() const { return is_name_set_by_user_; }

// Sets the desk's |creation_time_| to |creation_time|. This will overwrite
// |creation_time_| so this should only be called when restoring desks.
void set_creation_time(base::Time creation_time) {
creation_time_ = creation_time;
}

void AddObserver(Observer* observer);
void RemoveObserver(Observer* observer);

Expand Down Expand Up @@ -130,6 +139,10 @@ class ASH_EXPORT Desk {
// when desk is already removed from |desks_| in DesksController.
void SetDeskBeingRemoved();

// Records the lifetime of the desk based on its desk index. Should be called
// when this desk is removed by the user.
void RecordLifetimeHistogram();

private:
void MoveWindowToDeskInternal(aura::Window* window,
Desk* target_desk,
Expand Down Expand Up @@ -181,6 +194,9 @@ class ASH_EXPORT Desk {
// True if the desk is being removed.
bool is_desk_being_removed_ = false;

// The time this desk was created at. Used to record desk lifetimes.
base::Time creation_time_;

DISALLOW_COPY_AND_ASSIGN(Desk);
};

Expand Down
17 changes: 16 additions & 1 deletion ash/wm/desks/desks_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,7 @@ void DesksController::NewDesk(DesksCreationRemovalSource source) {

if (!is_first_ever_desk) {
desks_restore_util::UpdatePrimaryUserDeskNamesPrefs();
desks_restore_util::UpdatePrimaryUserDeskMetricsPrefs();
UMA_HISTOGRAM_ENUMERATION(kNewDeskHistogramName, source);
ReportDesksCountHistogram();
}
Expand Down Expand Up @@ -422,8 +423,10 @@ void DesksController::ReorderDesk(int old_index, int new_index) {
// Meanwhile, only the primary user needs to update the active desk, which is
// independent across profiles but only recoverable for the primary user.

// 1. Update desk name list in the user prefs to maintain the right order.
// 1. Update desk name and metrics lists in the user prefs to maintain the
// right order.
desks_restore_util::UpdatePrimaryUserDeskNamesPrefs();
desks_restore_util::UpdatePrimaryUserDeskMetricsPrefs();

// 2. For multi-profile switching, update all affected active desk index in
// |user_to_active_desk_index_|.
Expand Down Expand Up @@ -673,6 +676,14 @@ void DesksController::RestoreNameOfDeskAtIndex(base::string16 name,
desks_[index]->SetName(std::move(name), /*set_by_user=*/true);
}

void DesksController::RestoreMetricsOfDeskAtIndex(base::Time creation_time,
size_t index) {
DCHECK_LT(index, desks_.size());

const auto& target_desk = desks_[index];
target_desk->set_creation_time(std::move(creation_time));
}

void DesksController::OnRootWindowAdded(aura::Window* root_window) {
for (auto& desk : desks_)
desk->OnRootWindowAdded(root_window);
Expand Down Expand Up @@ -886,6 +897,9 @@ void DesksController::RemoveDeskInternal(const Desk* desk,
// Used by accessibility to indicate the desk that has been removed.
const int removed_desk_number = std::distance(desks_.begin(), iter) + 1;

// Record |desk|'s lifetime before it's removed from |desks_|.
const_cast<Desk*>(desk)->RecordLifetimeHistogram();

// Keep the removed desk alive until the end of this function.
std::unique_ptr<Desk> removed_desk = std::move(*iter);
removed_desk->SetDeskBeingRemoved();
Expand Down Expand Up @@ -1017,6 +1031,7 @@ void DesksController::RemoveDeskInternal(const Desk* desk,
active_desk_->name()));

desks_restore_util::UpdatePrimaryUserDeskNamesPrefs();
desks_restore_util::UpdatePrimaryUserDeskMetricsPrefs();

DCHECK_LE(available_container_ids_.size(), desks_util::GetMaxNumberOfDesks());
}
Expand Down
9 changes: 6 additions & 3 deletions ash/wm/desks/desks_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,14 @@ class ASH_EXPORT DesksController : public DesksHelper,
// it was never modified by users.
void RevertDeskNameToDefault(Desk* desk);

// Restores the desk at |index| to the given |name|. This is only for user-
// modified desk names, and hence |name| should never be empty since users are
// not allowed to set empty names.
// Restores the desk at |index| to the given |name|. This is only for
// user-modified desk names, and hence |name| should never be empty since
// users are not allowed to set empty names.
void RestoreNameOfDeskAtIndex(base::string16 name, size_t index);

// Restores the metrics of the desk at |index|.
void RestoreMetricsOfDeskAtIndex(base::Time creation_time, size_t index);

// Called explicitly by the RootWindowController when a root window has been
// added or about to be removed in order to update all the available desks.
void OnRootWindowAdded(aura::Window* root_window);
Expand Down
77 changes: 64 additions & 13 deletions ash/wm/desks/desks_restore_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ namespace desks_restore_util {

namespace {

// Keys for the dictionaries stored in |kDesksMetricsList|. The entry is an int
// which represents the number of minutes for
// base::Time::FromDeltaSinceWindowsEpoch().
constexpr char kCreationTimeKey[] = "creation_time";

// While restore is in progress, changes are being made to the desks and their
// names. Those changes should not trigger an update to the prefs.
bool g_pause_desks_prefs_updates = false;
Expand All @@ -46,6 +51,7 @@ bool IsValidDeskIndex(int desk_index) {
void RegisterProfilePrefs(PrefRegistrySimple* registry) {
constexpr int kDefaultActiveDeskIndex = 0;
registry->RegisterListPref(prefs::kDesksNamesList);
registry->RegisterListPref(prefs::kDesksMetricsList);
if (features::IsBentoEnabled()) {
registry->RegisterIntegerPref(prefs::kDesksActiveDesk,
kDefaultActiveDeskIndex);
Expand All @@ -61,11 +67,13 @@ void RestorePrimaryUserDesks() {
return;
}

const base::ListValue* desks_names_list =
const base::ListValue* desks_names =
primary_user_prefs->GetList(prefs::kDesksNamesList);
const base::ListValue* desks_metrics =
primary_user_prefs->GetList(prefs::kDesksMetricsList);

// First create the same number of desks.
const size_t restore_size = desks_names_list->GetSize();
const size_t restore_size = desks_names->GetSize();

// If we don't have any restore data, or the list is corrupt for some reason,
// abort.
Expand All @@ -76,9 +84,12 @@ void RestorePrimaryUserDesks() {
while (desks_controller->desks().size() < restore_size)
desks_controller->NewDesk(DesksCreationRemovalSource::kDesksRestore);

size_t index = 0;
for (const auto& value : desks_names_list->GetList()) {
const std::string& desk_name = value.GetString();
const auto& desks_names_list = desks_names->GetList();
const auto& desks_metrics_list = desks_metrics->GetList();
const size_t desks_metrics_list_size = desks_metrics->GetSize();
const auto now = base::Time::Now();
for (size_t index = 0; index < restore_size; ++index) {
const std::string& desk_name = desks_names_list[index].GetString();
// Empty desks names are just place holders for desks whose names haven't
// been modified by the user. Those don't need to be restored; they already
// have the correct default names based on their positions in the desks
Expand All @@ -87,7 +98,20 @@ void RestorePrimaryUserDesks() {
desks_controller->RestoreNameOfDeskAtIndex(base::UTF8ToUTF16(desk_name),
index);
}
++index;

if (index >= desks_metrics_list_size)
continue;

// Only restore metrics if there is existing data.
const auto& desks_metrics_dict = desks_metrics_list[index];
const auto& creation_time_entry =
desks_metrics_dict.FindIntPath(kCreationTimeKey);
if (creation_time_entry.has_value()) {
const auto creation_time = base::Time::FromDeltaSinceWindowsEpoch(
base::TimeDelta::FromMinutes(*creation_time_entry));
if (!creation_time.is_null() && creation_time < now)
desks_controller->RestoreMetricsOfDeskAtIndex(creation_time, index);
}
}

// Restore an active desk for the primary user.
Expand All @@ -114,20 +138,47 @@ void UpdatePrimaryUserDeskNamesPrefs() {
return;
}

ListPrefUpdate update(primary_user_prefs, prefs::kDesksNamesList);
base::ListValue* pref_data = update.Get();
pref_data->Clear();
ListPrefUpdate name_update(primary_user_prefs, prefs::kDesksNamesList);
base::ListValue* name_pref_data = name_update.Get();
name_pref_data->Clear();

const auto& desks = DesksController::Get()->desks();
for (const auto& desk : desks) {
// Desks whose names were not changed by the user, are stored as empty
// strings. They're just place holders to restore the correct desks count.
// RestorePrimaryUserDesks() restores only non-empty desks names.
pref_data->Append(desk->is_name_set_by_user()
? base::UTF16ToUTF8(desk->name())
: std::string());
name_pref_data->Append(desk->is_name_set_by_user()
? base::UTF16ToUTF8(desk->name())
: std::string());
}

DCHECK_EQ(name_pref_data->GetSize(), desks.size());
}

void UpdatePrimaryUserDeskMetricsPrefs() {
if (g_pause_desks_prefs_updates)
return;

PrefService* primary_user_prefs = GetPrimaryUserPrefService();
if (!primary_user_prefs) {
// Can be null in tests.
return;
}

ListPrefUpdate metrics_update(primary_user_prefs, prefs::kDesksMetricsList);
base::ListValue* metrics_pref_data = metrics_update.Get();
metrics_pref_data->Clear();

const auto& desks = DesksController::Get()->desks();
for (const auto& desk : desks) {
base::DictionaryValue metrics_dict;
metrics_dict.SetInteger(
kCreationTimeKey,
desk->creation_time().ToDeltaSinceWindowsEpoch().InMinutes());
metrics_pref_data->Append(std::move(metrics_dict));
}

DCHECK_EQ(pref_data->GetSize(), desks.size());
DCHECK_EQ(metrics_pref_data->GetSize(), desks.size());
}

void UpdatePrimaryUserActiveDeskPrefs(int active_desk_index) {
Expand Down
4 changes: 4 additions & 0 deletions ash/wm/desks/desks_restore_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ void RestorePrimaryUserDesks();
// desks count or a desk name changes.
ASH_EXPORT void UpdatePrimaryUserDeskNamesPrefs();

// Called to update the desk metrics restore prefs for the primary user whenever
// desks count changes or desks reordered.
ASH_EXPORT void UpdatePrimaryUserDeskMetricsPrefs();

// Called to update the active desk restore prefs for the primary user whenever
// the primary user switches an active desk.
void UpdatePrimaryUserActiveDeskPrefs(int active_desk_index);
Expand Down
20 changes: 20 additions & 0 deletions tools/metrics/histograms/histograms_xml/ash/histograms.xml
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,26 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
</summary>
</histogram>

<histogram name="Ash.Desks.DeskLifetime_{DeskIndex}" units="hr"
expires_after="2022-02-19">
<owner>chinsenj@chromium.org</owner>
<owner>janetmac@chromium.org</owner>
<summary>
The lifetime of the desk at index {DeskIndex} (1-indexed). Emitted when a
desk is removed. This metric is persisted across sessions.
</summary>
<token key="DeskIndex">
<variant name="1" summary="1"/>
<variant name="2" summary="2"/>
<variant name="3" summary="3"/>
<variant name="4" summary="4"/>
<variant name="5" summary="5"/>
<variant name="6" summary="6"/>
<variant name="7" summary="7"/>
<variant name="8" summary="8"/>
</token>
</histogram>

<histogram name="Ash.Desks.DesksCount2" units="units"
expires_after="2021-11-29">
<owner>afakhry@chromium.org</owner>
Expand Down

0 comments on commit 7936a65

Please sign in to comment.