The enum for shelf app shortcuts overlaps the enum for applist context menu commands, breaking histograms for context menu usage |
||||||
Issue description
This should be fixed before M-69, we should consider merging a fix for M-68 because this could distort the Apps.ContextMenuExecuteCommand histogram by artificially inflating the overlapping AppList(launcher) histogram values.
The overlapping ones are:
LAUNCH_NEW = 100,
TOGGLE_PIN = 101,
SHOW_APP_INFO = 102,
OPTIONS = 103,
UNINSTALL = 104,
REMOVE_FROM_FOLDER = 105,
MENU_NEW_WINDOW = 106,
MENU_NEW_INCOGNITO_WINDOW = 107,
INSTALL = 108,
The fix is to move the range of LauncherContextMenu::MenuItem::LAUNCH_APP_SHORTCUT_FIRST to 1000, instead of 100.
,
Jun 1 2018
,
Jun 1 2018
I just chatted with Omri. Omri said "distinguish metrics between these two (1) launching app shortcuts from shelf context menu (2) launching app shortcuts from launcher context menu" would be better. I think we also need to add enums in "ChromeOSUICommands" in enums.xml to better translate the metrics. Note: app shortcut items are at most five items, so we could define labels as: Launch App shortcut item 1 from shelf Launch App shortcut item 2 from shelf Launch App shortcut item 3 from shelf Launch App shortcut item 4 from shelf Launch App shortcut item 5 from shelf Launch App shortcut item 1 from launcher Launch App shortcut item 2 from launcher Launch App shortcut item 3 from launcher Launch App shortcut item 4 from launcher Launch App shortcut item 5 from launcher These are just some thoughts from mine.
,
Jun 1 2018
No idea on how to better translate the metrics, that seems like a different bug. These histograms are already separated by ShelfButton SearchResult, SuggestedApp, and AppGridApp. The last three are in the app list, so these metrics are already separated by launcher/shelf.
,
Jun 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f02698168b50d46fd951eeb5cfaff94b3888959c commit f02698168b50d46fd951eeb5cfaff94b3888959c Author: Alex Newcomer <newcomer@chromium.org> Date: Wed Jun 06 17:24:45 2018 cros: Fix enums so they don't over-write app list UMA Move the LAUNCH_APP_SHORTCUT_FIRST and LAUNCH_APP_SHORTCUT_LAST enums so they overlap properly with it's app list counterparts. The problem was that AppContextMenu::CommandId enums start at 100, and they are recorded using the same enums.xml entry as launcher would be mis-recorded in UMA as AppContextMenu::CommandId's. LauncherContextMenu: :MenuItem. This means that the app shortcuts in the Bug: 848491 Change-Id: I7b1e1d09eda3114842a349eab93a1c91d960ab4f Reviewed-on: https://chromium-review.googlesource.com/1082977 Commit-Queue: Alex Newcomer <newcomer@chromium.org> Reviewed-by: Yury Khmel <khmel@chromium.org> Cr-Commit-Position: refs/heads/master@{#564943} [modify] https://crrev.com/f02698168b50d46fd951eeb5cfaff94b3888959c/chrome/browser/ui/ash/launcher/launcher_context_menu.h
,
Jun 6 2018
Requesting merge for M-68. This change just modifies an enum which is used in histogram recording. Without this change we will get bad context menu metrics for all of M-68, which would be a bummer.
,
Jun 7 2018
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 11 2018
,
Jun 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9dfc9d6e20fd53f746c1757dba1068f70e7adec commit b9dfc9d6e20fd53f746c1757dba1068f70e7adec Author: Alex Newcomer <newcomer@chromium.org> Date: Wed Jun 13 01:00:39 2018 cros: Fix enums so they don't over-write app list UMA Move the LAUNCH_APP_SHORTCUT_FIRST and LAUNCH_APP_SHORTCUT_LAST enums so they overlap properly with it's app list counterparts. The problem was that AppContextMenu::CommandId enums start at 100, and they are recorded using the same enums.xml entry as launcher would be mis-recorded in UMA as AppContextMenu::CommandId's. TBR=newcomer@chromium.org (cherry picked from commit f02698168b50d46fd951eeb5cfaff94b3888959c) LauncherContextMenu: :MenuItem. This means that the app shortcuts in the Bug: 848491 Change-Id: I7b1e1d09eda3114842a349eab93a1c91d960ab4f Reviewed-on: https://chromium-review.googlesource.com/1082977 Commit-Queue: Alex Newcomer <newcomer@chromium.org> Reviewed-by: Yury Khmel <khmel@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#564943} Reviewed-on: https://chromium-review.googlesource.com/1098391 Reviewed-by: Alex Newcomer <newcomer@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#330} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/b9dfc9d6e20fd53f746c1757dba1068f70e7adec/chrome/browser/ui/ash/launcher/launcher_context_menu.h
,
Jun 13 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by newcomer@chromium.org
, Jun 1 2018Owner: newcomer@chromium.org
Status: Started (was: Assigned)