mac: Add metrics for fullscreen entry.

mac: Add metrics for fullscreen entry.

A histogram entry is emitted each time a browser window enters fullscreen. Each
entry has 3 associated parameters:
  - Whether the mechanism used was AppKit or Immersive.
  - Whether the window was on a primary or secondary screen.
  - Whether the "Displays have separate spaces" option in mission control was
  enabled.

BUG=

Committed: https://crrev.com/da30ed354c83de42e6c07b4369446b47b5cfb2cb
Cr-Commit-Position: refs/heads/master@{#299061}

[erikchen, 2014-09-30 00:01:42]

erikchen@chromium.org changed reviewers:
+ rsesek@chromium.org

[erikchen, 2014-09-30 00:01:43]

rsesek: Please review.

[Mark Mentovai, 2014-09-30 12:53:03]

> I think we do want what Mike suggests too.
> 
> For what Erik suggests, we need to be careful about how we structure
> it. If we only know whether the user entered fullscreen on a primary
> or secondary monitor, we’ll have some hard-to-digest data. We’d need a
> tri-state to make the most sense of it: entered fullscreen and only
> one display is present, entered fullscreen on primary with multiple
> displays, entered fullscreen on non-primary with multiple displays.

[Mark Mentovai, 2014-09-30 12:53:46]

That was from https://code.google.com/p/chromium/issues/detail?id=418703#c4
(noting it here because the BUG= is currently empty).

[erikchen, 2014-10-01 00:18:20]

rsesek: PTAL

[Robert Sesek, 2014-10-01 15:41:52]

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:63: // + Fullscreen
Mechanism: The mechanism by which the window's size is changed
Is this really an interesting metric? This will just test user vs. web initiated
fullscreen. Don't we really want to differentiate between entering "Presentation
Mode" and entering "Fullscreen"? (Or maybe we want both of these metricS).

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:79: // + Multiple
screens: Whether the user has multiple screens. If the window is
I thought this was going to be tracked once per startup? Also, I think doing a
histogram counts would be better than just a boolean, so that we can know if
users have 1, 2, 3, or 42 monitors.

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:119:
UMA_HISTOGRAM_ENUMERATION("OSX.Fullscreen.Enter", output, max_output);
I think this histogram is going to be harder to understand than just tracking
each one of these items uniquely.

[erikchen, 2014-10-01 16:47:45]

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:63: // + Fullscreen
Mechanism: The mechanism by which the window's size is changed
On 2014/10/01 15:41:51, Robert Sesek wrote:
> Is this really an interesting metric? This will just test user vs. web
initiated
> fullscreen. Don't we really want to differentiate between entering
"Presentation
> Mode" and entering "Fullscreen"? (Or maybe we want both of these metricS).

I intend to create a separate metric for presentation mode vs canonical
fullscreen.

We want both of the metrics. In a world where our analytics supported events
with properties, both of the metrics would be properties for this event. As it
stands, there's a tradeoff between adding more "properties" to this enumeration,
and readability of the results. Given the mirrored mechanisms of entry into
presentation mode and canonical fullscreen, I judged that the results from that
measurement would likely be independent from the properties measured in this
event, and thus could be measured separately.

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:79: // + Multiple
screens: Whether the user has multiple screens. If the window is
On 2014/10/01 15:41:51, Robert Sesek wrote:
> I thought this was going to be tracked once per startup? Also, I think doing a
> histogram counts would be better than just a boolean, so that we can know if
> users have 1, 2, 3, or 42 monitors.

I intend on creating a separate metric, which is why this CL is not tied to that
bug.

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:119:
UMA_HISTOGRAM_ENUMERATION("OSX.Fullscreen.Enter", output, max_output);
On 2014/10/01 15:41:51, Robert Sesek wrote:
> I think this histogram is going to be harder to understand than just tracking
> each one of these items uniquely.

I agree, but tracking the items separately loses information, which we may
actually care about. For example, let's say that 10% of people use fullscreen on
a second monitor, and 30% of people use AppKit Fullscreen. We might naively
think that 3% of people use AppKit Fullscreen on a second monitior, but it's
possible that these properties are not independent, and that all 10% of people
who use a second monitor also use AppKit Fullscreen.

[erikchen, 2014-10-06 19:37:17]

rsesek: PTAL

[Robert Sesek, 2014-10-06 22:08:03]

What's the point in collecting this data if it's not attached to any bug
tracking these improvements? I still think that the way the histogram is
organized will make it hard to actually utilize this data on the dashboards. If
you actually think you'll use this data, then I guess this is fine; but I think
we're really just interested in the number of screens a user has, which is a far
more general metric.

[erikchen, 2014-10-08 01:26:26]

I intend to use this data. One example is related to:
https://code.google.com/p/chromium/issues/detail?id=396980#c37

The bug only affects AppKit Fullscreen on a secondary screen, when "Screens Have
Separate Spaces" is off. I want to know how many users the problem affects so I
can make two decisions:
1. The priority of the problem.
2. Whether I should introduce hacks to work around the problem while I look for
a better solution.

By the time this particular metric is available, the example I gave will likely
no longer be relevant. That being said, I imagine that other use cases will take
its place. If you want, I can provide more examples of how I would use this data
if it were available today.


As I mentioned in a previous comment, I still intend to record the # of screens
as an isolated metric.

[Robert Sesek, 2014-10-08 19:28:48]

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:102: bool
displays_have_separate_spaces = [NSScreen screensHaveSeparateSpaces];
This will crash anything < 10.9.

[erikchen, 2014-10-08 23:49:46]

Patchset #4 (id:60001) has been deleted

[erikchen, 2014-10-08 23:50:45]

rsesek: PTAL

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/600303003/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:102: bool
displays_have_separate_spaces = [NSScreen screensHaveSeparateSpaces];
On 2014/10/08 19:28:48, Robert Sesek wrote:
> This will crash anything < 10.9.

Right you are. Thanks!

[Robert Sesek, 2014-10-09 14:11:34]

LGTM

https://codereview.chromium.org/600303003/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/600303003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:98: bool
primary_screen = ([window screen] == [screens objectAtIndex:0]);
Use -isEqual: instead?

[erikchen, 2014-10-09 22:07:56]

https://codereview.chromium.org/600303003/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/600303003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:98: bool
primary_screen = ([window screen] == [screens objectAtIndex:0]);
On 2014/10/09 14:11:33, Robert Sesek wrote:
> Use -isEqual: instead?

Done.

[erikchen, 2014-10-09 22:08:56]

erikchen@chromium.org changed reviewers:
+ isherman@chromium.org

[erikchen, 2014-10-09 22:08:56]

isherman: Looking for an OWNER review of tools/metrics/histograms/histograms.xml

[Ilya Sherman, 2014-10-09 22:16:58]

https://codereview.chromium.org/600303003/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/600303003/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:48067: +  </int>
Why are some of the possible values skipped?  If it's because they shouldn't be
reachable, I'd still include labels for them, with something like "invalid" in
the name.

[Ilya Sherman, 2014-10-09 22:18:03]

Also, is there a bug filed corresponding to this CL?

[erikchen, 2014-10-09 22:42:32]

There is no corresponding bug for this CL. See comment #10 on this CL for an
example of how I intend to use this metric. 

By the time we actually collect data from this metric, none of my current use
cases will still be valid, but I imagine that I will have new, unknown use
cases.

https://codereview.chromium.org/600303003/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/600303003/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:48067: +  </int>
On 2014/10/09 22:16:58, Ilya Sherman wrote:
> Why are some of the possible values skipped?  If it's because they shouldn't
be
> reachable, I'd still include labels for them, with something like "invalid" in
> the name.

Your assumption is correct. I've added labels for the non-reachable values.

[Ilya Sherman, 2014-10-09 22:44:52]

Histograms LGTM, thanks.

[erikchen, 2014-10-09 22:46:05]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-09 22:47:20]

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

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

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2014-10-10 05:20:52]

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