mac: Add metrics for AppKit Fullscreen style.

mac: Update metrics for Fullscreen styles.

I removed the metric "OSX.Fullscreen.Enter" because the confusion it was
causing outweighted the additional information it provided.

It has been replaced by 3 metrics:
  - OSX.Fullscreen.Enter.Style
  - OSX.Fullscreen.Enter.WindowLocation
  - OSX.Settings.ScreensHaveSeparateSpaces

BUG=414527
Committed: https://crrev.com/b4159bc3afe9f269ee0d1db3fe3112a351247c80
Cr-Commit-Position: refs/heads/master@{#301484}

[erikchen, 2014-10-14 01:28:18]

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

[erikchen, 2014-10-14 01:28:19]

rsesek: Please review.

[Robert Sesek, 2014-10-14 14:22:31]

Why is this just not another bit in the existing metric?

[erikchen, 2014-10-14 16:55:53]

In an ideal world, this would be another property on that same metric.
Unfortunately, the usability of the metric falls with the number of "bits"
mapped into it. In addition, I expect this particular property to be independent
from the existing mapped "bits". Using a separate histogram does not reduce the
usability of the existing metric, and likely has minimal information loss.

[Robert Sesek, 2014-10-16 19:07:30]

On 2014/10/14 16:55:53, erikchen wrote:
> In an ideal world, this would be another property on that same metric.
> Unfortunately, the usability of the metric falls with the number of "bits"
> mapped into it.

This was my criticism on the CL that added the metric in this way, and it's
already getting unusable now? I understand that some of these metrics are more
meaningful/useful when combined with other bits of data, but that's not true for
all the bits. I still think this is not the best way to handle the metric
collection, and this CL I think demonstrates that.

[erikchen, 2014-10-16 22:44:12]

On 2014/10/16 19:07:30, Robert Sesek wrote:
> On 2014/10/14 16:55:53, erikchen wrote:
> > In an ideal world, this would be another property on that same metric.
> > Unfortunately, the usability of the metric falls with the number of "bits"
> > mapped into it.
> 
> This was my criticism on the CL that added the metric in this way, and it's
> already getting unusable now? I understand that some of these metrics are more
> meaningful/useful when combined with other bits of data, but that's not true
for
> all the bits. I still think this is not the best way to handle the metric
> collection, and this CL I think demonstrates that.

I talked with some members of the Finch team, and there's really no better way
to do this. I sent them some feedback via email, and cc'ed you, but I don't
expect anything to happen on that front.

I dislike intentionally losing information, but I think you're right. Having
better maintainability and usability likely outweighs the loss of information. I
will update the CL appropriately.

For reference, we have some initial data from the metric I'm about to remove.
https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&endDate=10-15-2014...

Interesting tidbits include:
+ Users with multiple monitors are just as likely to fullscreen on a primary vs
secondary monitor. 
+ AppKit Fullscreen is 10x less common on a single monitor, but only 3x less
common when the user has multiple monitors. 
+ Screens have separate spaces is far more common than I would expect, given
that 10.9 defaults it to off.

[erikchen, 2014-10-17 00:17:44]

Patchset #2 (id:20001) has been deleted

[erikchen, 2014-10-17 00:18:49]

rsesek: PTAL

[Robert Sesek, 2014-10-20 17:34:46]

LGTM

https://codereview.chromium.org/640053007/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm
(right):

https://codereview.chromium.org/640053007/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:23:
[NSScreen respondsToSelector:@selector(screensHaveSeparateSpaces)] &&
You may want to collect !respondsToSelector:#selector(screensHaveSeparateFaces)
under a separate enum value.

[erikchen, 2014-10-20 18:30:59]

https://codereview.chromium.org/640053007/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm
(right):

https://codereview.chromium.org/640053007/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:23:
[NSScreen respondsToSelector:@selector(screensHaveSeparateSpaces)] &&
On 2014/10/20 17:34:46, Robert Sesek wrote:
> You may want to collect
!respondsToSelector:#selector(screensHaveSeparateFaces)
> under a separate enum value.

I expect your suggested metric to be almost identical to users on 10.9+ vs users
on 10.8 and older. Do you have reason to believe otherwise?

[Robert Sesek, 2014-10-20 18:54:36]

https://codereview.chromium.org/640053007/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm
(right):

https://codereview.chromium.org/640053007/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:23:
[NSScreen respondsToSelector:@selector(screensHaveSeparateSpaces)] &&
On 2014/10/20 18:30:59, erikchen wrote:
> On 2014/10/20 17:34:46, Robert Sesek wrote:
> > You may want to collect
> !respondsToSelector:#selector(screensHaveSeparateFaces)
> > under a separate enum value.
> 
> I expect your suggested metric to be almost identical to users on 10.9+ vs
users
> on 10.8 and older. Do you have reason to believe otherwise?

Right, my suggestion would separate out users that do not have this option.

[erikchen, 2014-10-21 18:44:38]

https://codereview.chromium.org/640053007/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm
(right):

https://codereview.chromium.org/640053007/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:23:
[NSScreen respondsToSelector:@selector(screensHaveSeparateSpaces)] &&
On 2014/10/20 18:54:36, Robert Sesek wrote:
> On 2014/10/20 18:30:59, erikchen wrote:
> > On 2014/10/20 17:34:46, Robert Sesek wrote:
> > > You may want to collect
> > !respondsToSelector:#selector(screensHaveSeparateFaces)
> > > under a separate enum value.
> > 
> > I expect your suggested metric to be almost identical to users on 10.9+ vs
> users
> > on 10.8 and older. Do you have reason to believe otherwise?
> 
> Right, my suggestion would separate out users that do not have this option.

Done.

[erikchen, 2014-10-21 18:45:28]

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

[erikchen, 2014-10-21 18:45:29]

isherman: Looking for an OWNER review of chrome/browser/metrics/* and
tools/metrics/histograms/histograms.xml

[Ilya Sherman, 2014-10-21 21:23:56]

https://codereview.chromium.org/640053007/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm
(right):

https://codereview.chromium.org/640053007/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:13: enum
SeparateSpaces {
nit: "SeparateSpaces" is a pretty odd type name.  One suggestion:
"ScreenSpacesConfiguration".

https://codereview.chromium.org/640053007/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:23: int
separate_spaces = SCREENS_CANNOT_HAVE_SEPARATE_SPACES;
nit: Why is this an int, rather than a "SeparateSpaces"?

https://codereview.chromium.org/640053007/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/640053007/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:21622: -<histogram
name="OSX.Fullscreen.Enter" enum="OSXFullscreenParameters">
Please mark the old histogram as <obsolete> rather than deleting the code
entirely.

https://codereview.chromium.org/640053007/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/640053007/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:48522: +  <int value="0"
label="CAANOT_HAVE_SEPARATE_SPACES">
nit: "caan" -> "cann"

[erikchen, 2014-10-24 17:55:03]

isherman: PTAL

https://codereview.chromium.org/640053007/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm
(right):

https://codereview.chromium.org/640053007/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:13: enum
SeparateSpaces {
On 2014/10/21 21:23:55, Ilya Sherman wrote:
> nit: "SeparateSpaces" is a pretty odd type name.  One suggestion:
> "ScreenSpacesConfiguration".

Used your suggestion. Thanks!

https://codereview.chromium.org/640053007/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:23: int
separate_spaces = SCREENS_CANNOT_HAVE_SEPARATE_SPACES;
On 2014/10/21 21:23:55, Ilya Sherman wrote:
> nit: Why is this an int, rather than a "SeparateSpaces"?

No good reason. Fixed.

https://codereview.chromium.org/640053007/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/640053007/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:21622: -<histogram
name="OSX.Fullscreen.Enter" enum="OSXFullscreenParameters">
On 2014/10/21 21:23:56, Ilya Sherman wrote:
> Please mark the old histogram as <obsolete> rather than deleting the code
> entirely.

Done.

https://codereview.chromium.org/640053007/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/640053007/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:48522: +  <int value="0"
label="CAANOT_HAVE_SEPARATE_SPACES">
On 2014/10/21 21:23:56, Ilya Sherman wrote:
> nit: "caan" -> "cann"

Done.

[Ilya Sherman, 2014-10-24 19:21:39]

histograms lgtm, thanks

[erikchen, 2014-10-24 20:56:45]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-24 20:59:41]

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

[commit-bot: I haz the power, 2014-10-24 21:38:31]

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

[commit-bot: I haz the power, 2014-10-24 21:38:32]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/73390)

[erikchen, 2014-10-27 21:47:06]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-27 21:52:44]

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

[commit-bot: I haz the power, 2014-10-27 23:39:04]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2014-10-27 23:40:22]

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