Report user actions for interactions with the Windows Settings app.

Report user actions for interactions with the Windows Settings app.

BUG=611827
Committed: https://crrev.com/c291eea8c3fe6ff78ee8c653063aff619fab21b2
Cr-Commit-Position: refs/heads/master@{#396184}

[grt (UTC plus 2), 2016-05-20 18:04:45]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 18:04:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003553003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003553003/1

[commit-bot: I haz the power, 2016-05-20 19:05:59]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[grt (UTC plus 2), 2016-05-20 19:07:42]

Patchset #2 (id:20001) has been deleted

[grt (UTC plus 2), 2016-05-20 19:10:11]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[grt (UTC plus 2), 2016-05-20 19:10:20]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-05-20 19:10:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003553003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003553003/40001

[grt (UTC plus 2), 2016-05-20 19:11:12]

grt@chromium.org changed reviewers:
+ asvitkine@chromium.org, pmonette@chromium.org

[grt (UTC plus 2), 2016-05-20 19:11:12]

pmonette: please review the general approach, especially the changes in
shell_integration.

asvitkine: please review use of user actions and change to actions.xml.

thanks.

[commit-bot: I haz the power, 2016-05-20 20:19:18]

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

[commit-bot: I haz the power, 2016-05-20 20:19:19]

Dry run: This issue passed the CQ dry run.

[Patrick Monette, 2016-05-24 18:13:13]

I think the code is pretty readable for all the stuff it has to do.

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
File chrome/browser/settings_app_monitor_win.cc (right):

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:63: // The only and only method that
may be called from outside of the automation
It's funny phrased that way, but did you mean "The one and only"?

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:180: DEFAULT_BROWSER,
Add a comment for each values

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:195: // DetectElementType to detect
the elements of interest.
nit: DetectElementType()

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:207: #endif  // !NDEBUG
nit: Remove exclamation point

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:499: if (event_id !=
UIA_Invoke_InvokedEventId)
This should never happen no?

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:615: observed_app_focused_ = false;
Why is this reset every time the focus change to something else? Doesn't it
defeat the purpose which is to suppress multiple calls to OnAppFocused (based on
the comment for observed_app_focused_)? Why would you get 2 calls to
HandleFocusChangedEvent for the same sender?

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/shell_in...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:266: // Monitors user intraction with
the Windows Settings app for the sake of
nit: s/intraction/interaction

[grt (UTC plus 2), 2016-05-24 18:48:34]

grt@chromium.org changed reviewers:
+ thakis@chromium.org

[grt (UTC plus 2), 2016-05-24 18:48:35]

Thanks! Comments addressed.

+thakis for chrome OWNERS review

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
File chrome/browser/settings_app_monitor_win.cc (right):

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:63: // The only and only method that
may be called from outside of the automation
On 2016/05/24 18:13:13, Patrick Monette wrote:
> It's funny phrased that way, but did you mean "The one and only"?

Done.

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:180: DEFAULT_BROWSER,
On 2016/05/24 18:13:13, Patrick Monette wrote:
> Add a comment for each values

Done.

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:195: // DetectElementType to detect
the elements of interest.
On 2016/05/24 18:13:13, Patrick Monette wrote:
> nit: DetectElementType()

Done.

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:207: #endif  // !NDEBUG
On 2016/05/24 18:13:12, Patrick Monette wrote:
> nit: Remove exclamation point

Done.

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:499: if (event_id !=
UIA_Invoke_InvokedEventId)
On 2016/05/24 18:13:12, Patrick Monette wrote:
> This should never happen no?

Correct. I had hooked this up to other event types during development, so it was
needed then. Removed.

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:615: observed_app_focused_ = false;
On 2016/05/24 18:13:12, Patrick Monette wrote:
> Why is this reset every time the focus change to something else? Doesn't it
> defeat the purpose which is to suppress multiple calls to OnAppFocused (based
on
> the comment for observed_app_focused_)?

It's intended to collapse multiple focus events for the element into one.

> Why would you get 2 calls to
> HandleFocusChangedEvent for the same sender?

I don't know, but I do!

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/shell_in...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:266: // Monitors user intraction with
the Windows Settings app for the sake of
On 2016/05/24 18:13:13, Patrick Monette wrote:
> nit: s/intraction/interaction

Done.

[grt (UTC plus 2), 2016-05-24 18:48:46]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 18:49:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003553003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003553003/60001

[Alexei Svitkine (slow), 2016-05-24 19:33:38]

https://codereview.chromium.org/2003553003/diff/60001/chrome/browser/OWNERS
File chrome/browser/OWNERS (right):

https://codereview.chromium.org/2003553003/diff/60001/chrome/browser/OWNERS#n...
chrome/browser/OWNERS:57: per-file
settings_app_monitor_win*=pmonette@chromium.org
Maybe it would be better to make a chrome/browser/win subdirectory? I noticed
chrome/browser/android and chrome/browser/mac exist as a precedence.

https://codereview.chromium.org/2003553003/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2003553003/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12653: The Windows Settings app monitor failed
to initialize successfully.
Since this doesn't correspond to a user action (something triggered by the
user), I suggest using a histogram to track this case and
SettingsAppMonitor.Initialized. It can be a boolean histogram with those two
options.

The other cases seem fine to use actions for.

[Patrick Monette, 2016-05-24 20:33:33]

lgtm

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
File chrome/browser/settings_app_monitor_win.cc (right):

https://codereview.chromium.org/2003553003/diff/40001/chrome/browser/settings...
chrome/browser/settings_app_monitor_win.cc:615: observed_app_focused_ = false;
On 2016/05/24 18:48:35, grt (slow) wrote:
> On 2016/05/24 18:13:12, Patrick Monette wrote:
> > Why is this reset every time the focus change to something else? Doesn't it
> > defeat the purpose which is to suppress multiple calls to OnAppFocused
(based
> on
> > the comment for observed_app_focused_)?
> 
> It's intended to collapse multiple focus events for the element into one.
> 
> > Why would you get 2 calls to
> > HandleFocusChangedEvent for the same sender?
> 
> I don't know, but I do!

¯\_(ツ)_/¯

[commit-bot: I haz the power, 2016-05-24 20:33:54]

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

[commit-bot: I haz the power, 2016-05-24 20:33:55]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2016-05-25 12:52:47]

Thanks for the suggestions. Please see proposal below.

https://codereview.chromium.org/2003553003/diff/60001/chrome/browser/OWNERS
File chrome/browser/OWNERS (right):

https://codereview.chromium.org/2003553003/diff/60001/chrome/browser/OWNERS#n...
chrome/browser/OWNERS:57: per-file
settings_app_monitor_win*=pmonette@chromium.org
On 2016/05/24 19:33:38, Alexei Svitkine (OOO til Tues) wrote:
> Maybe it would be better to make a chrome/browser/win subdirectory? I noticed
> chrome/browser/android and chrome/browser/mac exist as a precedence.

Good idea for an independent CL.

https://codereview.chromium.org/2003553003/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2003553003/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12653: The Windows Settings app monitor failed
to initialize successfully.
On 2016/05/24 19:33:38, Alexei Svitkine (OOO til Tues) wrote:
> Since this doesn't correspond to a user action (something triggered by the
> user), I suggest using a histogram to track this case and
> SettingsAppMonitor.Initialized.

I had thought Initialized would be good because it gives a concrete start point
(w/ a timestamp) to the interaction regardless of how it was initiated
(chrome://settings, the default browser infobar, or some other thing).

> It can be a boolean histogram with those two
> options.

WDYT of making Failed a BooleanHit histogram but leaving Initialized as-is?

[Alexei Svitkine (slow), 2016-05-25 15:51:34]

https://codereview.chromium.org/2003553003/diff/60001/chrome/browser/OWNERS
File chrome/browser/OWNERS (right):

https://codereview.chromium.org/2003553003/diff/60001/chrome/browser/OWNERS#n...
chrome/browser/OWNERS:57: per-file
settings_app_monitor_win*=pmonette@chromium.org
On 2016/05/25 12:52:47, grt (slow) wrote:
> On 2016/05/24 19:33:38, Alexei Svitkine (OOO til Tues) wrote:
> > Maybe it would be better to make a chrome/browser/win subdirectory? I
noticed
> > chrome/browser/android and chrome/browser/mac exist as a precedence.
> 
> Good idea for an independent CL.

If you're adding new files, I think it would be better to start them off in the
new directory rather than moving them. You also won't need to change this file
then.

For other files that should belong there eventually, you don't need to touch in
this CL.

https://codereview.chromium.org/2003553003/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2003553003/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12653: The Windows Settings app monitor failed
to initialize successfully.
On 2016/05/25 12:52:47, grt (slow) wrote:
> On 2016/05/24 19:33:38, Alexei Svitkine (OOO til Tues) wrote:
> > Since this doesn't correspond to a user action (something triggered by the
> > user), I suggest using a histogram to track this case and
> > SettingsAppMonitor.Initialized.
> 
> I had thought Initialized would be good because it gives a concrete start
point
> (w/ a timestamp) to the interaction regardless of how it was initiated
> (chrome://settings, the default browser infobar, or some other thing).

OK. Maybe mention that in its description.

> 
> > It can be a boolean histogram with those two
> > options.
> 
> WDYT of making Failed a BooleanHit histogram but leaving Initialized as-is?

A BooleanHit histogram is not great - because you get an absolute number that
won't tell you much w/ out other information (e.g. BooleanHit 38929 - is that a
lot or a little?). I would make it a BooleanSuccess with buckets for success and
failed.

[grt (UTC plus 2), 2016-05-26 13:13:48]

PTAL

https://codereview.chromium.org/2003553003/diff/60001/chrome/browser/OWNERS
File chrome/browser/OWNERS (right):

https://codereview.chromium.org/2003553003/diff/60001/chrome/browser/OWNERS#n...
chrome/browser/OWNERS:57: per-file
settings_app_monitor_win*=pmonette@chromium.org
On 2016/05/25 15:51:34, Alexei Svitkine (slow) wrote:
> On 2016/05/25 12:52:47, grt (slow) wrote:
> > On 2016/05/24 19:33:38, Alexei Svitkine (OOO til Tues) wrote:
> > > Maybe it would be better to make a chrome/browser/win subdirectory? I
> noticed
> > > chrome/browser/android and chrome/browser/mac exist as a precedence.
> > 
> > Good idea for an independent CL.
> 
> If you're adding new files, I think it would be better to start them off in
the
> new directory rather than moving them. You also won't need to change this file
> then.
> 
> For other files that should belong there eventually, you don't need to touch
in
> this CL.

Done.

https://codereview.chromium.org/2003553003/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2003553003/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12653: The Windows Settings app monitor failed
to initialize successfully.
On 2016/05/25 15:51:34, Alexei Svitkine (slow) wrote:
> On 2016/05/25 12:52:47, grt (slow) wrote:
> > On 2016/05/24 19:33:38, Alexei Svitkine (OOO til Tues) wrote:
> > > Since this doesn't correspond to a user action (something triggered by the
> > > user), I suggest using a histogram to track this case and
> > > SettingsAppMonitor.Initialized.
> > 
> > I had thought Initialized would be good because it gives a concrete start
> point
> > (w/ a timestamp) to the interaction regardless of how it was initiated
> > (chrome://settings, the default browser infobar, or some other thing).
> 
> OK. Maybe mention that in its description.
> 
> > 
> > > It can be a boolean histogram with those two
> > > options.
> > 
> > WDYT of making Failed a BooleanHit histogram but leaving Initialized as-is?
> 
> A BooleanHit histogram is not great - because you get an absolute number that
> won't tell you much w/ out other information (e.g. BooleanHit 38929 - is that
a
> lot or a little?). I would make it a BooleanSuccess with buckets for success
and
> failed.

Done.

[grt (UTC plus 2), 2016-05-26 13:16:48]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-26 13:16:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003553003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003553003/80001

[commit-bot: I haz the power, 2016-05-26 13:24:41]

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

[commit-bot: I haz the power, 2016-05-26 13:24:42]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[grt (UTC plus 2), 2016-05-26 13:27:46]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-26 13:28:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003553003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003553003/100001

[Alexei Svitkine (slow), 2016-05-26 14:58:39]

lgtm

[Nico, 2016-05-26 15:02:29]

lgtm

[commit-bot: I haz the power, 2016-05-26 15:22:37]

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

[commit-bot: I haz the power, 2016-05-26 15:22:38]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-26 15:22:51]

Dry run: None

[grt (UTC plus 2), 2016-05-26 15:33:24]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2016-05-26 15:33:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
pmonette@chromium.org
Link to the patchset: https://codereview.chromium.org/2003553003/#ps100001
(title: "fix histograms.xml typo")

[commit-bot: I haz the power, 2016-05-26 15:33:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003553003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003553003/100001

[commit-bot: I haz the power, 2016-05-26 15:39:02]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-05-26 15:40:35]

Description was changed from

==========
Report user actions for interactions with the Windows Settings app.

BUG=611827
==========

to

==========
Report user actions for interactions with the Windows Settings app.

BUG=611827

Committed: https://crrev.com/c291eea8c3fe6ff78ee8c653063aff619fab21b2
Cr-Commit-Position: refs/heads/master@{#396184}
==========

[commit-bot: I haz the power, 2016-05-26 15:40:36]

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