Instrument "Remove this person" in user manager.

Instrument "Remove this person" in user manager.

BUG=465884
Committed: https://crrev.com/a5c808a0be557fe269013edf04a8d3ab39b00055
Cr-Commit-Position: refs/heads/master@{#322065}

[anthonyvd, 2015-03-18 18:57:05]

anthonyvd@chromium.org changed reviewers:
+ asvitkine@chromium.org, noms@chromium.org

[anthonyvd, 2015-03-18 18:57:05]

Hi! Could you guys please take a look at this instrumentation of the user's
manager profile removal:

asvitkine@: histograms.xml
noms@: profile_metrics, user_manager_screen_handler, and user_pod_row

Thanks a lot!

[anthonyvd, 2015-03-18 19:01:04]

anthonyvd@chromium.org changed reviewers:
+ jwd@chromium.org
- asvitkine@chromium.org

[anthonyvd, 2015-03-18 19:01:05]

Oops, didn't realize Alexei was away. Jesse, could you take a look at histograms
please?

[anthonyvd, 2015-03-18 19:04:52]

Oops, didn't realize Alexei was away. Jesse, could you take a look at histograms
please?

[jwd, 2015-03-18 21:14:21]

https://codereview.chromium.org/1016113002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1016113002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:56989: +    profile deletion warning.
So, if the user actually deletes, this will be followed by the a "Settings Page"
value being logged too, right?

I might tweak the Profile.DeleteProfileAction histogram description a bit, as it
implies that it tracks actions that would trigger a deletion attempt (assuming
my interpretation of this enum value). Also, maybe add yourself as an owner, if
you're comfortable.

[noms (inactive), 2015-03-18 21:20:48]

lgtm % nits

https://codereview.chromium.org/1016113002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/1016113002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_metrics.h:55: // Show the delete profile warning
in the user manager.
nit: User Manager (title caps)

[anthonyvd, 2015-03-18 21:56:56]

anthonyvd@chromium.org changed reviewers:
+ dbeam@chromium.org

[anthonyvd, 2015-03-18 21:56:56]

Turns out logging the same information when deletion happens from the settings
page is required as well.

dbeam@ could you please take a look at:
chrome/browser/resources/options/manage_profile_overlay.js	chrome/browser/ui/webui/options/manage_profile_handler.h	chrome/browser/ui/webui/options/manage_profile_handler.cc

Thanks a lot!

https://codereview.chromium.org/1016113002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/1016113002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_metrics.h:55: // Show the delete profile warning
in the user manager.
On 2015/03/18 21:20:48, Monica Dinculescu wrote:
> nit: User Manager (title caps)

Done.

https://codereview.chromium.org/1016113002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1016113002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:56989: +    profile deletion warning.
On 2015/03/18 21:14:21, Jesse Doherty wrote:
> So, if the user actually deletes, this will be followed by the a "Settings
Page"
> value being logged too, right?
> 
> I might tweak the Profile.DeleteProfileAction histogram description a bit, as
it
> implies that it tracks actions that would trigger a deletion attempt (assuming
> my interpretation of this enum value). Also, maybe add yourself as an owner,
if
> you're comfortable.

Actually it would be the "User Manager" value being logged next but I was just
asked to add something similar for the settings page as well. Basically the idea
is that those warnings are probably going to be tweaked and this histogram will
help us track the changes in the amount of users who keep going after seeing the
warning.

I'll definitely follow your advice and tweak the description/add myself as an
owner. Thanks!

(Done.)

[Dan Beam, 2015-03-18 22:36:33]

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:576:
chrome.send('showDeleteUserDialog');
this looks very much like you're saying imperatively "Show the delete user
dialog!" with this chrome.send() call.

but you're actually just logging that it was shown.

can you think of a more appropriate name for this, like
"logDeleteUserDialogShown" or something?

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:225: void
ManageProfileHandler::HandleDeleteUserDialogShown(
instead of making this a private instance method, maybe just:


namespace {

void HandleDeleteUserDialogShown() {
  ProfileMetrics::LogProfileDeleteUser(
      ProfileMetrics::DELETE_PROFILE_SETTINGS_SHOW_WARNING);
}

}  // namespace

// ...later on...

  web_ui()->RegisterMessageCallback("showDeleteUserDialog",
      base::Bind(&HandleDeleteUserDialogShown));

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.h (right):

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.h:50: void
HandleDeleteUserDialogShown(const base::ListValue* args);
make private

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:78: const char
kJsApiUserManagerShowRemoveUserWarning[] = "showRemoveUserWarning";
same nit about naming -> if this doesn't actually show a remove user warning,
rename it to something like "log*Shown"

https://codereview.chromium.org/1016113002/diff/20001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/1016113002/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:2405: chrome.send('kioskAppsLoaded',
[]);
why extra []?

[anthonyvd, 2015-03-19 15:22:35]

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.js (right):

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.js:576:
chrome.send('showDeleteUserDialog');
On 2015/03/18 22:36:33, Dan Beam wrote:
> this looks very much like you're saying imperatively "Show the delete user
> dialog!" with this chrome.send() call.
> 
> but you're actually just logging that it was shown.
> 
> can you think of a more appropriate name for this, like
> "logDeleteUserDialogShown" or something?

Done.

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.h (right):

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.h:50: void
HandleDeleteUserDialogShown(const base::ListValue* args);
On 2015/03/18 22:36:33, Dan Beam wrote:
> make private

Acknowledged.

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1016113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:78: const char
kJsApiUserManagerShowRemoveUserWarning[] = "showRemoveUserWarning";
On 2015/03/18 22:36:33, Dan Beam wrote:
> same nit about naming -> if this doesn't actually show a remove user warning,
> rename it to something like "log*Shown"

Done.

https://codereview.chromium.org/1016113002/diff/20001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/1016113002/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:2405: chrome.send('kioskAppsLoaded',
[]);
On 2015/03/18 22:36:33, Dan Beam wrote:
> why extra []?

No idea how that slipped in here, reverted.

[Dan Beam, 2015-03-19 17:28:39]

lgtm

https://codereview.chromium.org/1016113002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/1016113002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:67: void
HandleDeleteUserDialogShown(const base::ListValue* args) {
nit: HandleLogDeleteUserDialogShown

https://codereview.chromium.org/1016113002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1016113002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:539: void
UserManagerScreenHandler::HandleRemoveUserWarningShown(
nit: HandleLogRemoveUserWarningShown

https://codereview.chromium.org/1016113002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:539: void
UserManagerScreenHandler::HandleRemoveUserWarningShown(
nit: could also be in an anonymous namespace

[jwd, 2015-03-20 20:43:44]

lgtm

[anthonyvd, 2015-03-23 15:48:48]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2015-03-23 15:48:49]

The patchset sent to the CQ was uploaded after l-g-t-m from noms@chromium.org,
jwd@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1016113002/#ps60001
(title: "Nits before submit")

[commit-bot: I haz the power, 2015-03-23 15:48:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1016113002/60001

[commit-bot: I haz the power, 2015-03-23 17:19:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-23 17:19:57]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[anthonyvd, 2015-03-23 19:53:29]

anthonyvd@chromium.org changed reviewers:
+ antrim@chromium.org

[anthonyvd, 2015-03-23 19:53:30]

antrim@, could you please take a look at the signin_screen_handler files? This
change broke a couple ChromeOS browser tests so I made the fixes to those files.

Thanks!

[Denis Kuznetsov (DE-MUC), 2015-03-24 17:20:01]

lgtm

[Denis Kuznetsov (DE-MUC), 2015-03-24 17:20:02]

lgtm

lgtm

[anthonyvd, 2015-03-24 19:17:13]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2015-03-24 19:17:14]

The patchset sent to the CQ was uploaded after l-g-t-m from noms@chromium.org,
jwd@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1016113002/#ps80001
(title: "Fix ChromeOS Supervised User creation/deletion browser tests.")

[commit-bot: I haz the power, 2015-03-24 19:17:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1016113002/80001

[commit-bot: I haz the power, 2015-03-24 20:28:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-24 20:28:45]

Patchset 5 (id:??) landed as
https://crrev.com/a5c808a0be557fe269013edf04a8d3ab39b00055
Cr-Commit-Position: refs/heads/master@{#322065}