Power button user action metrics are broken and misleading |
||||||
Issue description
In ash/wm/lock_state_controller.cc, LockStateController::OnRealPowerTimeout records the "Accel_ShutDown_PowerButton" user action:
void LockStateController::OnRealPowerTimeout() {
VLOG(1) << "OnRealPowerTimeout";
DCHECK(shutting_down_);
WmShell::Get()->RecordUserMetricsAction(UMA_ACCEL_SHUT_DOWN_POWER_BUTTON);
// Shut down or reboot based on device policy.
shutdown_controller_->ShutDownOrReboot();
}
This was maybe accurate at one point, but I think that that this code path is now used for all shutdown requests (e.g. also the system tray).
Note that OnRealPowerTimeout can also trigger a reboot. An "Accel_Restart_PowerButton" user action is defined as well in ash/metrics/user_metrics_recorder.cc, but there isn't any code that actually records it.
It looks like there's already a "Tray_ShutDown" action, so "Accel_ShutDown_PowerButton" probably needs to get moved to the classes that actually handle the power button (PowerButtonController and TabletPowerButtonController?).
I don't think that the user can actually request that the system reboot instead of shutting down while holding the power button; it's just controlled by a policy. As such, I suspect that there's no value in keeping "Accel_Restart_PowerButton" even if we were to report it correctly.
,
Jun 16 2017
Lann, see what you think of this one. You can work with Dan.
,
Jun 21 2017
The PowerButtonControllers interact with LockStateController by starting a timer and then cancelling it if the button is released. They don't get any feedback on actual shutdown. Another approach here would be to add an e.g. shutdown_reason enum param to appropriate methods on LockStateController and use that to record the appropriate metric on timer expiration.
,
Jun 21 2017
Adding a "reason" enum to LSC and having it map that plus shutdown vs. reboot to an appropriate UMA name sounds good to me.
,
Jun 21 2017
Do we want to distinguish shutdown vs reboot in UMA? As mentioned above that doesn't seem to be under the user's control. If we do want to distinguish, that logic will need to pushed down into ShutdownController.
,
Jun 21 2017
Dropping the shutdown vs. reboot distinction in UMA (and probably standardizing on "shutdown" naming) seems reasonable. The main source of reboots is probably the "Restart to update" item in the system tray, and we should have a separate reason for requests that come from there in any case.
,
Jun 27 2017
,
Jun 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4fd794c35351c388f49dd111d6c6c6432312d1fe commit 4fd794c35351c388f49dd111d6c6c6432312d1fe Author: Lann Martin <lannm@chromium.org> Date: Fri Jun 30 17:51:41 2017 Fix UMA_ACCEL_SHUT_DOWN_POWER_BUTTON reporting This metric was being sent in LockStateController::OnRealPowerTimeout, which is also used by e.g. the tray shut down button. Add ShutdownReason enum with user actions that can trigger shutdown. Pass ShutdownReason from user event handlers through to ShutdownController where it is used to send metrics. Currently only ShutdownReason::POWER_BUTTON is handled here as other metrics are reported in other locations. Remove `shutdown_after_lock` param from LockStateController::StatLockAnimation in favor of new StartLockThenShutdownAnimation method. BUG= chromium:694039 TEST=manually verified correct shutdown reasons Change-Id: I62bddff668a9134d472308d9f5e06283d4b94d1a Reviewed-on: https://chromium-review.googlesource.com/543905 Commit-Queue: Lann Martin <lannm@chromium.org> Reviewed-by: Jacob Dufault <jdufault@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> Cr-Commit-Position: refs/heads/master@{#483750} [modify] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/ash/shelf/shelf_layout_manager_unittest.cc [modify] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/ash/shutdown_controller.cc [modify] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/ash/shutdown_controller.h [add] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/ash/shutdown_reason.h [modify] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/ash/system/power/tablet_power_button_controller.cc [modify] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/ash/system/tiles/tiles_default_view.cc [modify] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/ash/wm/lock_state_controller.cc [modify] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/ash/wm/lock_state_controller.h [modify] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/ash/wm/lock_state_controller_unittest.cc [modify] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/ash/wm/power_button_controller.cc [modify] https://crrev.com/4fd794c35351c388f49dd111d6c6c6432312d1fe/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc
,
Jun 30 2017
,
Jul 5 2017
,
Jan 22 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by derat@chromium.org
, Jun 2 2017Labels: Hotlist-GoodFirstBug
Owner: ----
Status: Available (was: Assigned)