Add metrics for the number of attached displays.

Add metrics for the number of attached displays.

This CL adds 2 metrics. The first records the number of attached displays at
the time that Chrome is launched. The second records the number of attached
displays immediately after the user has added or removed a display.

BUG=418703
Committed: https://crrev.com/90b1ea7b00271b0fe7594d2a2efb00eff3c9bd92
Cr-Commit-Position: refs/heads/master@{#301889}

[erikchen, 2014-10-08 01:28:35]

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

[erikchen, 2014-10-08 01:28:35]

rsesek: Please review.

[Robert Sesek, 2014-10-08 19:25:20]

https://codereview.chromium.org/641453003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h (right):

https://codereview.chromium.org/641453003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:33:
base::scoped_ptr<ChromeBrowserMainExtraPartsMetricsMac>
I think this could be an OS_MACOSX-specific method instead,
AddMacMonitorMetrics() that is called in the right override instead. The
implementation can then live in the .mm file. The extra C++ class seems
heavyweight.

https://codereview.chromium.org/641453003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm
(right):

https://codereview.chromium.org/641453003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:23:
@implementation MetricsBridge
Could you instead use gfx::Screen::AddObserver(gfx::DisplayObserver*) ? Though
that uses CGDisplayRegisterReconfigurationCallback instead of this notification.

https://codereview.chromium.org/641453003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:50:
UMA_HISTOGRAM_ENUMERATION("OSX.DisplayCount.Changed",
This seems like a _COUNTS instead of an _ENUMERATION.

[erikchen, 2014-10-08 22:49:19]

https://codereview.chromium.org/641453003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h (right):

https://codereview.chromium.org/641453003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:33:
base::scoped_ptr<ChromeBrowserMainExtraPartsMetricsMac>
On 2014/10/08 19:25:20, Robert Sesek wrote:
> I think this could be an OS_MACOSX-specific method instead,
> AddMacMonitorMetrics() that is called in the right override instead. The
> implementation can then live in the .mm file. The extra C++ class seems
> heavyweight.

I removed all of the ObjC and OSX specific code from the CL. Theoretically, this
CL could be refactored so that no new files are required. If we wanted to
collect these metrics on all platforms, moving all the code into the existing
files seems like the best way forward.

I am operating under the assumption that we should only add metrics that we
directly intend on using, and that we shouldn't add these metrics for the other
platforms. In that case, separating the code (~80 lines total) into OS specific
files seems like the right way forward. 

Your suggestion works well for the metric OSX.DisplayCount.StartUp, but doesn't
work well for the metric OSX.DisplayCount.Changed, which requires
adding/removing observers, changing the inheritance hierarchy, etc.

I don't care too strongly about this, so if you still prefer a reduction in the
number of new files, just let me know.

https://codereview.chromium.org/641453003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm
(right):

https://codereview.chromium.org/641453003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:23:
@implementation MetricsBridge
On 2014/10/08 19:25:20, Robert Sesek wrote:
> Could you instead use gfx::Screen::AddObserver(gfx::DisplayObserver*) ? Though
> that uses CGDisplayRegisterReconfigurationCallback instead of this
notification.

I went with your suggestion and also switched to GetNumDisplays(), which has a
slightly different behavior. I updated the summaries in histograms.xml to
indicate this change.

https://codereview.chromium.org/641453003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:50:
UMA_HISTOGRAM_ENUMERATION("OSX.DisplayCount.Changed",
On 2014/10/08 19:25:20, Robert Sesek wrote:
> This seems like a _COUNTS instead of an _ENUMERATION.

_COUNTS uses an exponential histogram (even _COUNTS_100). I want a linear
histogram to make sure that I can distinguish the low numbers (1, 2, 3), so I
have to use _ENUMERATION.

[chromium-reviews, 2014-10-09 05:44:14]

Exponential histograms give you low resolution at the low counts range
already. iirc, you get exact values up to 8, but you can double check by
looking at an existing counts histogram on the dashboard.

On Wed, Oct 8, 2014 at 6:49 PM, <erikchen@chromium.org> wrote:

>
> https://codereview.chromium.org/641453003/diff/1/chrome/
> browser/metrics/chrome_browser_main_extra_parts_metrics.h
> File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h
> (right):
>
> https://codereview.chromium.org/641453003/diff/1/chrome/
> browser/metrics/chrome_browser_main_extra_parts_metrics.h#newcode33
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:33:
> base::scoped_ptr<ChromeBrowserMainExtraPartsMetricsMac>
> On 2014/10/08 19:25:20, Robert Sesek wrote:
>
>> I think this could be an OS_MACOSX-specific method instead,
>> AddMacMonitorMetrics() that is called in the right override instead.
>>
> The
>
>> implementation can then live in the .mm file. The extra C++ class
>>
> seems
>
>> heavyweight.
>>
>
> I removed all of the ObjC and OSX specific code from the CL.
> Theoretically, this CL could be refactored so that no new files are
> required. If we wanted to collect these metrics on all platforms, moving
> all the code into the existing files seems like the best way forward.
>
> I am operating under the assumption that we should only add metrics that
> we directly intend on using, and that we shouldn't add these metrics for
> the other platforms. In that case, separating the code (~80 lines total)
> into OS specific files seems like the right way forward.
>
> Your suggestion works well for the metric OSX.DisplayCount.StartUp, but
> doesn't work well for the metric OSX.DisplayCount.Changed, which
> requires adding/removing observers, changing the inheritance hierarchy,
> etc.
>
> I don't care too strongly about this, so if you still prefer a reduction
> in the number of new files, just let me know.
>
> https://codereview.chromium.org/641453003/diff/1/chrome/
> browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm
> File
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm
> (right):
>
> https://codereview.chromium.org/641453003/diff/1/chrome/
> browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm#newcode23
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:23:
> @implementation MetricsBridge
> On 2014/10/08 19:25:20, Robert Sesek wrote:
>
>> Could you instead use gfx::Screen::AddObserver(gfx::DisplayObserver*)
>>
> ? Though
>
>> that uses CGDisplayRegisterReconfigurationCallback instead of this
>>
> notification.
>
> I went with your suggestion and also switched to GetNumDisplays(), which
> has a slightly different behavior. I updated the summaries in
> histograms.xml to indicate this change.
>
> https://codereview.chromium.org/641453003/diff/1/chrome/
> browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm#newcode50
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:50:
> UMA_HISTOGRAM_ENUMERATION("OSX.DisplayCount.Changed",
> On 2014/10/08 19:25:20, Robert Sesek wrote:
>
>> This seems like a _COUNTS instead of an _ENUMERATION.
>>
>
> _COUNTS uses an exponential histogram (even _COUNTS_100). I want a
> linear histogram to make sure that I can distinguish the low numbers (1,
> 2, 3), so I have to use _ENUMERATION.
>
> https://codereview.chromium.org/641453003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Robert Sesek, 2014-10-09 14:42:28]

On 2014/10/09 05:44:14, chromium-reviews wrote:
> Exponential histograms give you low resolution at the low counts range
> already. iirc, you get exact values up to 8, but you can double check by
> looking at an existing counts histogram on the dashboard.

A CUSTOM_COUNTS could also be used to control the bucketing explicitly.

https://codereview.chromium.org/641453003/diff/20001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h
(right):

https://codereview.chromium.org/641453003/diff/20001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h:16: virtual
~ChromeBrowserMainExtraPartsMetricsMac() override;
Is |override| right here?

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

Patchset #4 (id:80001) has been deleted

[erikchen, 2014-10-09 22:28:47]

Patchset #4 (id:140001) has been deleted

[erikchen, 2014-10-09 22:29:28]

rsesek: PTAL

I switched to UMA_HISTOGRAM_COUNTS_100 as it has buckets for all integers
between 1 and 26.

https://codereview.chromium.org/641453003/diff/20001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h
(right):

https://codereview.chromium.org/641453003/diff/20001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h:16: virtual
~ChromeBrowserMainExtraPartsMetricsMac() override;
On 2014/10/09 14:42:28, Robert Sesek wrote:
> Is |override| right here?

Yes. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance
"""Explicitly annotate overrides of virtual functions or virtual destructors
with an override or (less frequently) final specifier."""

The style guide indicates that the keywords |virtual| and |override| should not
both be used, but the Chrome style checker isn't ready for that yet.

[Robert Sesek, 2014-10-10 15:17:07]

LGTM w/ nits

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

https://codereview.chromium.org/641453003/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h:29: int
displayCount_;
naming: display_count_

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

https://codereview.chromium.org/641453003/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:37: int
displayCount = gfx::Screen::GetNativeScreen()->GetNumDisplays();
naming: display_count

[erikchen, 2014-10-10 17:01:54]

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

https://codereview.chromium.org/641453003/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h:29: int
displayCount_;
On 2014/10/10 15:17:07, Robert Sesek wrote:
> naming: display_count_

Done.

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

https://codereview.chromium.org/641453003/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.mm:37: int
displayCount = gfx::Screen::GetNativeScreen()->GetNumDisplays();
On 2014/10/10 15:17:07, Robert Sesek wrote:
> naming: display_count

Done.

[erikchen, 2014-10-10 17:05:16]

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

[erikchen, 2014-10-10 17:05:21]

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

avi: Looking for OWNER review of ui/gfx/screen_mac.mm

[Avi (use Gerrit), 2014-10-10 17:20:44]

On 2014/10/10 17:05:21, erikchen wrote:
> isherman: Looking for OWNER review of chrome/browser/metrics/* and
> tools/metrics/histograms/*
> 
> avi: Looking for OWNER review of ui/gfx/screen_mac.mm

ui/gfx/screen_mac.mm LGTM

[Ilya Sherman, 2014-10-10 22:29:57]

https://codereview.chromium.org/641453003/diff/210001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h
(right):

https://codereview.chromium.org/641453003/diff/210001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h:32: };
None of this code looks Mac-specific.  Why are you only tracking this metric on
that one platform?

[erikchen, 2014-10-10 22:51:19]

On 2014/10/10 22:29:57, Ilya Sherman wrote:
>
https://codereview.chromium.org/641453003/diff/210001/chrome/browser/metrics/...
> File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h
> (right):
> 
>
https://codereview.chromium.org/641453003/diff/210001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h:32: };
> None of this code looks Mac-specific.  Why are you only tracking this metric
on
> that one platform?

I am operating under the assumption that we should only add metrics that we
directly intend on using. I have no intention (and I know of no one else with
the intention) of using these metrics on other platforms. Am I mistaken?

[Ilya Sherman, 2014-10-10 22:53:48]

On 2014/10/10 22:51:19, erikchen wrote:
> On 2014/10/10 22:29:57, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/641453003/diff/210001/chrome/browser/metrics/...
> > File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/641453003/diff/210001/chrome/browser/metrics/...
> > chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h:32: };
> > None of this code looks Mac-specific.  Why are you only tracking this metric
> on
> > that one platform?
> 
> I am operating under the assumption that we should only add metrics that we
> directly intend on using. I have no intention (and I know of no one else with
> the intention) of using these metrics on other platforms. Am I mistaken?

If you think this metric is useful on one platform, it's likely that it would be
useful on other platforms as well.  Typically, we add platform-specific metrics
for one of two reasons:
  (1) The metric only makes sense on a specific platform, e.g. which flavor of
Mac OS X the user is running.
  (2) The metric is a PITA to implement on other platforms, and we currently
only need it on one platform.

If neither of these applies, then it's typically best to just implement the
metric for all OSes where it makes sense.

[erikchen, 2014-10-14 00:09:09]

Patchset #5 (id:330001) has been deleted

[erikchen, 2014-10-14 01:09:59]

Patchset #5 (id:350001) has been deleted

[erikchen, 2014-10-14 01:11:25]

On 2014/10/10 22:53:48, Ilya Sherman wrote:
> On 2014/10/10 22:51:19, erikchen wrote:
> > On 2014/10/10 22:29:57, Ilya Sherman wrote:
> > >
> >
>
https://codereview.chromium.org/641453003/diff/210001/chrome/browser/metrics/...
> > > File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/641453003/diff/210001/chrome/browser/metrics/...
> > > chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h:32:
};
> > > None of this code looks Mac-specific.  Why are you only tracking this
metric
> > on
> > > that one platform?
> > 
> > I am operating under the assumption that we should only add metrics that we
> > directly intend on using. I have no intention (and I know of no one else
with
> > the intention) of using these metrics on other platforms. Am I mistaken?
> 
> If you think this metric is useful on one platform, it's likely that it would
be
> useful on other platforms as well.  Typically, we add platform-specific
metrics
> for one of two reasons:
>   (1) The metric only makes sense on a specific platform, e.g. which flavor of
> Mac OS X the user is running.
>   (2) The metric is a PITA to implement on other platforms, and we currently
> only need it on one platform.
> 
> If neither of these applies, then it's typically best to just implement the
> metric for all OSes where it makes sense.

Got it. I did as you suggested. PTAL

[erikchen, 2014-10-14 01:11:32]

https://codereview.chromium.org/641453003/diff/210001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h
(right):

https://codereview.chromium.org/641453003/diff/210001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_mac.h:32: };
On 2014/10/10 22:29:57, Ilya Sherman wrote:
> None of this code looks Mac-specific.  Why are you only tracking this metric
on
> that one platform?

Deleted this class. Moved all code into chrome_browser_main_extra_parts_metrics.

[Ilya Sherman, 2014-10-14 06:34:22]

I started pondering why we didn't already have a metric like this, and realized
that it's because it's already included in the system profile: [
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...
].  Given that, you might want to think about whether there's value in adding
this as a histogram as well -- perhaps you could simply use the existing metric
instead?

If you do want to add this metric, I'll strongly recommend choosing a broader
group.  I suggest "Display" below, but maybe something even broader like
"Hardware" would be better, i.e. something like
"Hardware.Display.Count.OnStartup".  The reason for this suggestion is that the
histograms dashboard lists all of the group names first, and so it's nice to try
to keep that list reasonably short, when possible, so that people can actually
scan through it and find stuff.

If you decide that you do want to add this extra metric, then LGTM with a
broader group name for the histograms.

https://codereview.chromium.org/641453003/diff/370001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/641453003/diff/370001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:224:
UMA_HISTOGRAM_COUNTS_100("DisplayCount.StartUp", display_count_);
nit: Please name this "Display.Count.StartUp" (or, even better IMO,
"Display.Count.AtStartUp".  The important part here is to have the group name be
"Display" rather than "DisplayCount", so that there's room to add additional
display metrics if they become useful over time.

https://codereview.chromium.org/641453003/diff/370001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:277:
UMA_HISTOGRAM_COUNTS_100("DisplayCount.Changed", display_count_);
nit: And correspondingly, please name this "Display.Count.Changed" (or
"Display.Count.OnChange").

[erikchen, 2014-10-14 23:09:38]

One of the metrics measured in this CL (Hardware.Display.Count.OnStartup) is
available as raw dremel data, but not in histogram form. Once that becomes
available, we can remove the histogram.

https://codereview.chromium.org/641453003/diff/370001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/641453003/diff/370001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:224:
UMA_HISTOGRAM_COUNTS_100("DisplayCount.StartUp", display_count_);
On 2014/10/14 06:34:22, Ilya Sherman wrote:
> nit: Please name this "Display.Count.StartUp" (or, even better IMO,
> "Display.Count.AtStartUp".  The important part here is to have the group name
be
> "Display" rather than "DisplayCount", so that there's room to add additional
> display metrics if they become useful over time.

Went with "Hardware.Display.Count.OnStartup".

https://codereview.chromium.org/641453003/diff/370001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:277:
UMA_HISTOGRAM_COUNTS_100("DisplayCount.Changed", display_count_);
On 2014/10/14 06:34:22, Ilya Sherman wrote:
> nit: And correspondingly, please name this "Display.Count.Changed" (or
> "Display.Count.OnChange").

Went with "Hardware.Display.Count.OnChange".

[erikchen, 2014-10-14 23:09:49]

The CQ bit was checked by erikchen@chromium.org

[Ilya Sherman, 2014-10-14 23:10:46]

LGTM, thanks.

[commit-bot: I haz the power, 2014-10-14 23:14:41]

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

[commit-bot: I haz the power, 2014-10-14 23:48:16]

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

[commit-bot: I haz the power, 2014-10-14 23:48:17]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[erikchen, 2014-10-15 00:06:21]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-15 00:08:26]

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

[commit-bot: I haz the power, 2014-10-15 00:42:41]

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

[commit-bot: I haz the power, 2014-10-15 00:42:42]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2014-10-21 19:01:45]

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

[erikchen, 2014-10-21 19:01:47]

sky: Looking for an OWNER review of ui/gfx/gfx.gyp.

[sky, 2014-10-21 19:47:47]

LGTM

[erikchen, 2014-10-21 19:50:45]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-21 19:51:53]

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

[commit-bot: I haz the power, 2014-10-21 19:59:46]

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

[commit-bot: I haz the power, 2014-10-21 19:59:47]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/76460)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[erikchen, 2014-10-22 00:45:22]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-22 00:48:14]

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

[commit-bot: I haz the power, 2014-10-22 01:22:22]

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

[commit-bot: I haz the power, 2014-10-22 01:22:24]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[erikchen, 2014-10-22 01:29:21]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-22 01:29:53]

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

[commit-bot: I haz the power, 2014-10-22 01:55:35]

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

[commit-bot: I haz the power, 2014-10-22 01:55:37]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[erikchen, 2014-10-22 17:11:10]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-22 17:13:12]

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

[commit-bot: I haz the power, 2014-10-22 17:23:35]

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

[commit-bot: I haz the power, 2014-10-22 17:23:37]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2014-10-22 23:16:08]

[3162:3162:1021/191413:FATAL:screen_aura.cc(12)] Check failed: false.
Implementation should be installed at higher level.

[erikchen, 2014-10-28 23:52:03]

Patchset #10 (id:470001) has been deleted

[erikchen, 2014-10-29 01:23:06]

isherman: PTAL

Diff patch set 11 against patch set 9. I had to delay the recording of metrics
to wait for the native screen to get initialized on aura.

[Ilya Sherman, 2014-10-29 01:30:11]

LGTM % nits:

https://codereview.chromium.org/641453003/diff/510001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h (right):

https://codereview.chromium.org/641453003/diff/510001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:47: int
display_count_;
nit: Please leave a blank line after this one.

https://codereview.chromium.org/641453003/diff/510001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:48: // This
parameter is set to true after an instance of this class makes itself
nit: "parameter" -> "variable" (though, I'd actually suggest shortening this
comment to something like "True iff |this| instance is registered as an observer
of the native screen.")

[erikchen, 2014-10-29 17:15:41]

https://codereview.chromium.org/641453003/diff/510001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h (right):

https://codereview.chromium.org/641453003/diff/510001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:47: int
display_count_;
On 2014/10/29 01:30:10, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/641453003/diff/510001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:48: // This
parameter is set to true after an instance of this class makes itself
On 2014/10/29 01:30:10, Ilya Sherman wrote:
> nit: "parameter" -> "variable" (though, I'd actually suggest shortening this
> comment to something like "True iff |this| instance is registered as an
observer
> of the native screen.")

Went with your suggestion.

[erikchen, 2014-10-29 17:15:47]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-10-29 17:17:34]

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

[commit-bot: I haz the power, 2014-10-29 18:42:45]

Committed patchset #12 (id:530001)

[commit-bot: I haz the power, 2014-10-29 18:43:32]

Patchset 12 (id:??) landed as
https://crrev.com/90b1ea7b00271b0fe7594d2a2efb00eff3c9bd92
Cr-Commit-Position: refs/heads/master@{#301889}