Fixed the Touchscreen.TouchEventsEnabled histogram to record the correct values on X11 and Ozone ba…

Fixed the Touchscreen.TouchEventsEnabled histogram to record the correct values on X11 and Ozone based builds.

The Touchscreen.TouchEventsEnabled histogram was being recorded before
the X11 and Ozone sub-systems had a chance to scan all the devices. This
caused the recorded value to be incorrect.

TEST=ChromeBrowserMainExtraPartsMetricsTest.VerifyTouchEventsEnabledIsNotRecordedAfterConstruction
TEST=ChromeBrowserMainExtraPartsMetricsTest.VerifyTouchEventsEnabledIsNotRecordedAfterPostBrowserStart
TEST=ChromeBrowserMainExtraPartsMetricsTest.VerifyTouchEventsEnabledIsRecordedWhenDeviceListsComplete
TEST=ChromeBrowserMainExtraPartsMetricsTest.VerifyTouchEventsEnabledIsOnlyRecordedOnce
TEST=ChromeBrowserMainExtraPartsMetricsTest.VerifyTouchEventsEnabledIsRecordedAfterPostBrowserStart

BUG=499476
Committed: https://crrev.com/86fbc49b97910112e68ad53dde3d7c9cdf63eef8
Cr-Commit-Position: refs/heads/master@{#338414}

[bruthig, 2015-06-15 20:02:52]

bruthig@chromium.org changed reviewers:
+ asvitkine@chromium.org, rbyers@chromium.org, sadrul@chromium.org

[bruthig, 2015-06-15 20:02:52]

rbyers@chromium.org: Please review to ensure the semantics for the
Touchscreen.TouchEventsEnabled metric hasn't changed.

asvitkine@chromium.org: Please review changes in:
 - chrome/*

sadrul@chromium.org: Please review changes in 
 - ui/*

[Rick Byers, 2015-06-16 14:45:47]

Metrics semantics seems good reasonable modulo possible race:

https://codereview.chromium.org/1182303005/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/1182303005/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:276: #if
defined(USE_OZONE) || defined(USE_X11)
Rather than use an #if, couldn't we encapsulate this into DeviceDataManager? 
Eg. a 'areDeviceListsComplete' method?  Wouldn't that also prevent a race on
OZONE/X11 where OnDeviceListsComplete occurred before we happened to get our
observer installed?

[bruthig, 2015-06-16 15:52:45]

rbyers@, please see my reply to comment.

https://codereview.chromium.org/1182303005/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/1182303005/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:276: #if
defined(USE_OZONE) || defined(USE_X11)
On 2015/06/16 14:45:47, Rick Byers (Slow until 6-22) wrote:
> Rather than use an #if, couldn't we encapsulate this into DeviceDataManager? 
> Eg. a 'areDeviceListsComplete' method?  Wouldn't that also prevent a race on
> OZONE/X11 where OnDeviceListsComplete occurred before we happened to get our
> observer installed?

Agreed I'm not happy about the 'potential' race condition either, but IIUC, and
spang@ can confirm, the race condition does not exist in the current
implementation.  It would only be introduced if the point at which attaching the
observer changed or if the spot where the device scan is started is changed.

Having said that, from what I can tell the DeviceDataManager is not used (or
created) on the Windows or Android platforms (see IsTouchDevicePresent()
implementations here
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...
and here
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...).
 So adding a DeviceDataManager::AreDeviceListsComplete() method is not
sufficient.

IMO the check to determine if the return value for IsTouchDevicePresent() is
valid should live in the same spot as IsTouchDevicePresent() not in
DeviceDataManager.  But this is not trivial to implement since
IsTouchDevicePresent() is a static/global function and for the X11 and Ozone
platforms it requires the asynchronous observer.

If we want to address this concern I think the right thing to do would be to
turn touch_devices.h into a proper class and each platform can have have their
own implementation.

Thoughts?

[spang, 2015-06-16 15:55:39]

spang@chromium.org changed reviewers:
+ spang@chromium.org

[spang, 2015-06-16 15:55:40]

https://codereview.chromium.org/1182303005/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/1182303005/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:276: #if
defined(USE_OZONE) || defined(USE_X11)
On 2015/06/16 15:52:45, bruthig wrote:
> On 2015/06/16 14:45:47, Rick Byers (Slow until 6-22) wrote:
> > Rather than use an #if, couldn't we encapsulate this into DeviceDataManager?

> > Eg. a 'areDeviceListsComplete' method?  Wouldn't that also prevent a race on
> > OZONE/X11 where OnDeviceListsComplete occurred before we happened to get our
> > observer installed?
> 
> Agreed I'm not happy about the 'potential' race condition either, but IIUC,
and
> spang@ can confirm, the race condition does not exist in the current
> implementation.  It would only be introduced if the point at which attaching
the
> observer changed or if the spot where the device scan is started is changed.
> 
> Having said that, from what I can tell the DeviceDataManager is not used (or
> created) on the Windows or Android platforms (see IsTouchDevicePresent()
> implementations here
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...
> and here
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...).
>  So adding a DeviceDataManager::AreDeviceListsComplete() method is not
> sufficient.
> 
> IMO the check to determine if the return value for IsTouchDevicePresent() is
> valid should live in the same spot as IsTouchDevicePresent() not in
> DeviceDataManager.  But this is not trivial to implement since
> IsTouchDevicePresent() is a static/global function and for the X11 and Ozone
> platforms it requires the asynchronous observer.
> 
> If we want to address this concern I think the right thing to do would be to
> turn touch_devices.h into a proper class and each platform can have have their
> own implementation.
> 
> Thoughts?

A race free way to do it is

  if (ui::DeviceDataManager::GetInstance()->device_lists_complete())
    OnDeviceListsComplete();
  else
    ui::DeviceDataManager::GetInstance()->AddObserver(this);

[Rick Byers, 2015-06-16 16:53:30]

On 2015/06/16 15:55:40, spang wrote:
>
https://codereview.chromium.org/1182303005/diff/1/chrome/browser/metrics/chro...
> File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
(right):
> 
>
https://codereview.chromium.org/1182303005/diff/1/chrome/browser/metrics/chro...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:276: #if
> defined(USE_OZONE) || defined(USE_X11)
> On 2015/06/16 15:52:45, bruthig wrote:
> > On 2015/06/16 14:45:47, Rick Byers (Slow until 6-22) wrote:
> > > Rather than use an #if, couldn't we encapsulate this into
DeviceDataManager?
> 
> > > Eg. a 'areDeviceListsComplete' method?  Wouldn't that also prevent a race
on
> > > OZONE/X11 where OnDeviceListsComplete occurred before we happened to get
our
> > > observer installed?
> > 
> > Agreed I'm not happy about the 'potential' race condition either, but IIUC,
> and
> > spang@ can confirm, the race condition does not exist in the current
> > implementation.  It would only be introduced if the point at which attaching
> the
> > observer changed or if the spot where the device scan is started is changed.
> > 
> > Having said that, from what I can tell the DeviceDataManager is not used (or
> > created) on the Windows or Android platforms (see IsTouchDevicePresent()
> > implementations here
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...
> > and here
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...).
> >  So adding a DeviceDataManager::AreDeviceListsComplete() method is not
> > sufficient.
> > 
> > IMO the check to determine if the return value for IsTouchDevicePresent() is
> > valid should live in the same spot as IsTouchDevicePresent() not in
> > DeviceDataManager.  But this is not trivial to implement since
> > IsTouchDevicePresent() is a static/global function and for the X11 and Ozone
> > platforms it requires the asynchronous observer.

I'm ok leaving IsTouchDevicePresent out of the discussion here.  It seems to be
working well enough in practice.

> > If we want to address this concern I think the right thing to do would be to
> > turn touch_devices.h into a proper class and each platform can have have
their
> > own implementation.
> > 
> > Thoughts?
> 
> A race free way to do it is
> 
>   if (ui::DeviceDataManager::GetInstance()->device_lists_complete())
>     OnDeviceListsComplete();
>   else
>     ui::DeviceDataManager::GetInstance()->AddObserver(this);

Yes, this is exactly what I was thinking.  It's also nicer in that
implementation details like what platforms will raise "OnDeviceListsComplete"
notifications can be kept inside of DeviceDataManager instead of each call site
needing to think about that.

[bruthig, 2015-06-24 18:16:54]

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

[bruthig, 2015-06-24 18:16:54]

Please note this CL is now based off of
https://codereview.chromium.org/1202383002/.

Replacing asvitkine@ with isherman@ since he is OOO.

rbyers@chromium.org: Please review to ensure the semantics for the
Touchscreen.TouchEventsEnabled metric hasn't changed.

isherman@chromium.org: Please review changes in:
 - chrome/*

sadrul@chromium.org: Please review changes in 
 - ui/*

[Ilya Sherman, 2015-06-24 23:46:51]

What was previously recorded to this histogram on these platforms, in the cases
where the data was not yet available?  Is it worth adding a caveat to the
histogram description in histograms.xml to document which data is trustworthy
vs. not trustworthy?  Is it perhaps worthwhile to even rename the histogram, so
that trustworthy data is not mixed with untrustworthy data under a single name?

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

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:239: }
nit: Please revert the change to this line (i.e. restore the newline below it).

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:254:
ui::DeviceDataManager::GetInstance()->RemoveObserver(this);
Is it safe to call RemoveObserver() even if |this| is not currently registered
as an observer?

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
(right):

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:18:
namespace {
nit: Please leave a blank line below this one.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:20:
"Touchscreen.TouchEventsEnabled";
nit: Please leave a blank line below this one.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:23:
// Test fixture for the ChromeBrowserMainExtraPartsMetricsTest class.
Optional nit: This comment seems unnecessary to me, in that it is redundant with
the code.  OK to leave if you really prefer it, though.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:31:
void TearDown() override;
nit: Please inline all of the method definitions for this class.  In my
experience, that's the common style in unit test files.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:69:
new ui::test::DeviceDataManagerTestAPI(device_data_manager_.get()));
Can you move all of this work to the constructor, rather than to the SetUp()
method?

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:75:
device_data_manager_test_api_.reset();
Are you writing this explicitly because the order matters?  If so, please
document that.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:77:
ui::test::DeviceDataManagerTestAPI::SetDeviceDataManagerInstance(nullptr);
Likewise, can you move all of this to the destructor?

[Rick Byers, 2015-06-25 17:17:32]

On 2015/06/24 23:46:51, Ilya Sherman wrote:
> What was previously recorded to this histogram on these platforms, in the
cases
> where the data was not yet available?  Is it worth adding a caveat to the
> histogram description in histograms.xml to document which data is trustworthy
> vs. not trustworthy?  Is it perhaps worthwhile to even rename the histogram,
so
> that trustworthy data is not mixed with untrustworthy data under a single
name?

Note that it was trustworthy at one point but regressed with Ozone.  So I'd
rather not change the name now (to retain easy comparison between future data
and the correct past dasta), there's just a period in time where the data has
issues (and documenting that in the XML SGTM).  Concretely I have contour
queries that have been collecting this data for years (eg.
https://contour.corp.google.com/#view/14067) - I'd rather just have an anonymous
period (which is a small slice of the total data) than loose continuity
completely.

[Rick Byers, 2015-06-25 17:22:44]

chrome/browser/metrics LGTM

[Rick Byers, 2015-06-25 17:23:36]

On 2015/06/25 17:22:44, Rick Byers wrote:
> chrome/browser/metrics LGTM

modulo isherman@'s feedback of course...

[bruthig, 2015-06-25 17:26:52]

I've addressed isherman@'s concerns from patch set 5.

On 2015/06/24 23:46:51, Ilya Sherman wrote:
> What was previously recorded to this histogram on these platforms, in the
cases
> where the data was not yet available?  Is it worth adding a caveat to the
> histogram description in histograms.xml to document which data is trustworthy
> vs. not trustworthy?  Is it perhaps worthwhile to even rename the histogram,
so
> that trustworthy data is not mixed with untrustworthy data under a single
name?

The metric would have been incorrectly recorded in the "Automatic - disabled"
bucket instead of the "Automatic - enabled".  I went to add a note to the
histogram description and noticed it was in the internal histograms.xml. 
_rbyers@_, is there any reason not to move it in to the public histograms.xml?
Alternatively, what are your thoughts on deprecating the existing histogram name
and creating a new one? Would "Touchscreen.TouchEventsEnabled_v2" work?


rbyers@chromium.org: Please review to ensure the semantics for the
Touchscreen.TouchEventsEnabled metric hasn't changed.

isherman@chromium.org: Please review changes in:
 - chrome/*

sadrul@chromium.org: Please review changes in 
 - ui/*

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

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:239: }
On 2015/06/24 23:46:50, Ilya Sherman wrote:
> nit: Please revert the change to this line (i.e. restore the newline below
it).

Done.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:254:
ui::DeviceDataManager::GetInstance()->RemoveObserver(this);
On 2015/06/24 23:46:50, Ilya Sherman wrote:
> Is it safe to call RemoveObserver() even if |this| is not currently registered
> as an observer?

That path is executed by the tests so I assume it is.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
(right):

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:18:
namespace {
On 2015/06/24 23:46:51, Ilya Sherman wrote:
> nit: Please leave a blank line below this one.

Done.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:20:
"Touchscreen.TouchEventsEnabled";
On 2015/06/24 23:46:51, Ilya Sherman wrote:
> nit: Please leave a blank line below this one.

Done.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:23:
// Test fixture for the ChromeBrowserMainExtraPartsMetricsTest class.
On 2015/06/24 23:46:51, Ilya Sherman wrote:
> Optional nit: This comment seems unnecessary to me, in that it is redundant
with
> the code.  OK to leave if you really prefer it, though.

Done.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:31:
void TearDown() override;
On 2015/06/24 23:46:51, Ilya Sherman wrote:
> nit: Please inline all of the method definitions for this class.  In my
> experience, that's the common style in unit test files.

The reason I don't do that is because it implies an 'inline' request to the
compiler which will bloats the code. Do you know the reason why the common style
is to inline them?

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:69:
new ui::test::DeviceDataManagerTestAPI(device_data_manager_.get()));
On 2015/06/24 23:46:51, Ilya Sherman wrote:
> Can you move all of this work to the constructor, rather than to the SetUp()
> method?

Done.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:75:
device_data_manager_test_api_.reset();
On 2015/06/24 23:46:50, Ilya Sherman wrote:
> Are you writing this explicitly because the order matters?  If so, please
> document that.

Removed.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:77:
ui::test::DeviceDataManagerTestAPI::SetDeviceDataManagerInstance(nullptr);
On 2015/06/24 23:46:51, Ilya Sherman wrote:
> Likewise, can you move all of this to the destructor?

Done.

[Rick Byers, 2015-06-25 17:29:31]

On 2015/06/25 17:26:52, bruthig wrote:
> I've addressed isherman@'s concerns from patch set 5.
> 
> On 2015/06/24 23:46:51, Ilya Sherman wrote:
> > What was previously recorded to this histogram on these platforms, in the
> cases
> > where the data was not yet available?  Is it worth adding a caveat to the
> > histogram description in histograms.xml to document which data is
trustworthy
> > vs. not trustworthy?  Is it perhaps worthwhile to even rename the histogram,
> so
> > that trustworthy data is not mixed with untrustworthy data under a single
> name?
> 
> The metric would have been incorrectly recorded in the "Automatic - disabled"
> bucket instead of the "Automatic - enabled".  I went to add a note to the
> histogram description and noticed it was in the internal histograms.xml. 
> _rbyers@_, is there any reason not to move it in to the public histograms.xml?

Moving is almost certainly fine.  IIRC at the time this was added years ago, the
only option was the internal file.
 
> Alternatively, what are your thoughts on deprecating the existing histogram
name
> and creating a new one? Would "Touchscreen.TouchEventsEnabled_v2" work?

I'd rather not do that for the loss of continuity reasons mentioned earlier. 
Eg. on Windows the data has always been fine, sucks to screw up all the
long-term logging we've got for that (and tools like contour aren't great at
letting us fix up the logging).

> 
> 
> mailto:rbyers@chromium.org: Please review to ensure the semantics for the
> Touchscreen.TouchEventsEnabled metric hasn't changed.
> 
> mailto:isherman@chromium.org: Please review changes in:
>  - chrome/*
> 
> mailto:sadrul@chromium.org: Please review changes in 
>  - ui/*
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
(right):
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:239: }
> On 2015/06/24 23:46:50, Ilya Sherman wrote:
> > nit: Please revert the change to this line (i.e. restore the newline below
> it).
> 
> Done.
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:254:
> ui::DeviceDataManager::GetInstance()->RemoveObserver(this);
> On 2015/06/24 23:46:50, Ilya Sherman wrote:
> > Is it safe to call RemoveObserver() even if |this| is not currently
registered
> > as an observer?
> 
> That path is executed by the tests so I assume it is.
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> File
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:18:
> namespace {
> On 2015/06/24 23:46:51, Ilya Sherman wrote:
> > nit: Please leave a blank line below this one.
> 
> Done.
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:20:
> "Touchscreen.TouchEventsEnabled";
> On 2015/06/24 23:46:51, Ilya Sherman wrote:
> > nit: Please leave a blank line below this one.
> 
> Done.
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:23:
> // Test fixture for the ChromeBrowserMainExtraPartsMetricsTest class.
> On 2015/06/24 23:46:51, Ilya Sherman wrote:
> > Optional nit: This comment seems unnecessary to me, in that it is redundant
> with
> > the code.  OK to leave if you really prefer it, though.
> 
> Done.
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:31:
> void TearDown() override;
> On 2015/06/24 23:46:51, Ilya Sherman wrote:
> > nit: Please inline all of the method definitions for this class.  In my
> > experience, that's the common style in unit test files.
> 
> The reason I don't do that is because it implies an 'inline' request to the
> compiler which will bloats the code. Do you know the reason why the common
style
> is to inline them?
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:69:
> new ui::test::DeviceDataManagerTestAPI(device_data_manager_.get()));
> On 2015/06/24 23:46:51, Ilya Sherman wrote:
> > Can you move all of this work to the constructor, rather than to the SetUp()
> > method?
> 
> Done.
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:75:
> device_data_manager_test_api_.reset();
> On 2015/06/24 23:46:50, Ilya Sherman wrote:
> > Are you writing this explicitly because the order matters?  If so, please
> > document that.
> 
> Removed.
> 
>
https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:77:
> ui::test::DeviceDataManagerTestAPI::SetDeviceDataManagerInstance(nullptr);
> On 2015/06/24 23:46:51, Ilya Sherman wrote:
> > Likewise, can you move all of this to the destructor?
> 
> Done.

[bruthig, 2015-06-25 18:27:35]

I've added the TouchScreen.TouchEventsEnabled histogram to the public
histograms.xml.

rbyers@, isherman@, can you have a look at the description?

isherman@, Should I submit the change to the private histograms.xml first or the
public one?

[Ilya Sherman, 2015-06-25 20:31:48]

Metrics LGTM % the remaining nits.  For histograms.xml, it's fine to land the
CLs in either order.

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
(right):

https://codereview.chromium.org/1182303005/diff/80001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:31:
void TearDown() override;
On 2015/06/25 17:26:52, bruthig wrote:
> On 2015/06/24 23:46:51, Ilya Sherman wrote:
> > nit: Please inline all of the method definitions for this class.  In my
> > experience, that's the common style in unit test files.
> 
> The reason I don't do that is because it implies an 'inline' request to the
> compiler which will bloats the code. Do you know the reason why the common
style
> is to inline them?

I think that for test suites, it just doesn't matter that much, and so people
prefer the increased readability.

https://codereview.chromium.org/1182303005/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
(right):

https://codereview.chromium.org/1182303005/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:33: }
nit: It's fine for test fixture classes to have public or protected data
members.

https://codereview.chromium.org/1182303005/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:54:
ui::test::DeviceDataManagerTestAPI::CreateDeviceDataManager().Pass()),
nit: I think you can omit the ".Pass()" here and still have correct behavior.

https://codereview.chromium.org/1182303005/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1182303005/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44870: +    through 44, see
http://crbug.com/499476 for more details. Tracks whether
nit: Please move the note to a <details> section.

[Rick Byers, 2015-06-25 20:37:04]

Description LGTM modulo isherman@'s nit (we want the short summary on the
dashboard to describe what it is first).

[bruthig, 2015-06-25 22:36:25]

https://codereview.chromium.org/1182303005/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
(right):

https://codereview.chromium.org/1182303005/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:33: }
On 2015/06/25 20:31:47, Ilya Sherman wrote:
> nit: It's fine for test fixture classes to have public or protected data
> members.

Done.

https://codereview.chromium.org/1182303005/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:54:
ui::test::DeviceDataManagerTestAPI::CreateDeviceDataManager().Pass()),
On 2015/06/25 20:31:47, Ilya Sherman wrote:
> nit: I think you can omit the ".Pass()" here and still have correct behavior.

Done.

https://codereview.chromium.org/1182303005/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1182303005/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44870: +    through 44, see
http://crbug.com/499476 for more details. Tracks whether
On 2015/06/25 20:31:47, Ilya Sherman wrote:
> nit: Please move the note to a <details> section.

Done.

[sadrul, 2015-06-27 07:24:44]

ui lgtm

[bruthig, 2015-06-29 18:39:24]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-06-29 18:39:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1182303005/#ps140001
(title: "Moved histogram note to <details> and fixed nits in tests.")

[commit-bot: I haz the power, 2015-06-29 18:39:58]

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

[commit-bot: I haz the power, 2015-06-29 18:55:20]

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

[commit-bot: I haz the power, 2015-06-29 18:55:21]

Exceeded global retry quota

[bruthig, 2015-06-29 19:24:15]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-06-29 19:24:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, sadrul@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1182303005/#ps160001
(title: "Added #if guard around DeviceDataManager access.")

[commit-bot: I haz the power, 2015-06-29 19:24:34]

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

[commit-bot: I haz the power, 2015-06-29 19:40:53]

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

[commit-bot: I haz the power, 2015-06-29 19:40:55]

Exceeded global retry quota

[Alexei Svitkine (slow), 2015-07-06 17:37:23]

asvitkine@chromium.org changed reviewers:
- asvitkine@chromium.org

[bruthig, 2015-07-07 20:49:29]

I've had to fix some compile time issues.

asvitkine@, can you please have a look at the changes in patch set 10 for:
 - chrome/*

sadrul@, can you please have a look at the changes in patch set 10 for:
 - ui/*

[Alexei Svitkine (slow), 2015-07-07 22:00:58]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-07-07 22:00:59]

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

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:21: #if
defined(USE_OZONE) || defined(USE_X11)
Nit: I don't think it's necessary to guard forward declarations. It will still
compile without the guards - they just make this code messier.

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
(right):

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:24:
class TestScreen : public gfx::Screen {
Seems like we have a number of these TestScreen subclasses already in the
codebase.

e.g.:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Can we move it to its own file and share the same class from different tests?

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:32:
<< "Function not implemented!!! 'TestScreen::GetCursorScreenPoint()'";
Nit: No need for these messages. I think when NOTREACHED() is hit, you get the
stack trace. So no need to mention the function name. And then, "Function not
implemented" is redundant since it's a NOTREACHED() already.

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:107:
#endif  // defined(USE_OZONE) || defined(USE_X11)
This is a lot of extra complexity being put into the chrome/ code. Can this
complexity be tucked away in the ui test helper?

i.e. can ui::test::DeviceDataManagerTestAPI own the device_data_manager and also
have the ifdefs about ozone? So that this code can just have its instance around
without ifdefs?

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:131:
ui::test::DeviceDataManagerTestAPI::SetDeviceDataManagerInstance(nullptr);
Can this be handled in the
DeviceDataManagerTestAPI::SetDeviceDataManagerInstance() implementation, since
it's already a helper for testing?

https://codereview.chromium.org/1182303005/diff/180001/ui/events/devices/devi...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/1182303005/diff/180001/ui/events/devices/devi...
ui/events/devices/device_data_manager.cc:38: CHECK(!instance || !instance_)
Nit: I think this should be a DCHECK().

https://codereview.chromium.org/1182303005/diff/180001/ui/events/test/device_...
File ui/events/test/device_data_manager_test_api.cc (right):

https://codereview.chromium.org/1182303005/diff/180001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.cc:31: }
Nit: Add blank lines between functions.

[sadrul, 2015-07-07 22:04:39]

lgtm with the nits (and feedback from asvitkine@) addressed.

https://codereview.chromium.org/1182303005/diff/180001/ui/events/devices/devi...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/1182303005/diff/180001/ui/events/devices/devi...
ui/events/devices/device_data_manager.cc:39: << "Can not set multiple instances
of DeviceDataManager";
Have separate DCHECK()s with the right message instead? "Cannot set multiple ...
" and "Must be non-null"

[Ilya Sherman, 2015-07-07 22:06:22]

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
(right):

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:39:
return NULL;
nit: nullptr (pretty much everywhere where you're using NULL now)

[bruthig, 2015-07-08 20:29:52]

I've made some non-trivial changes.

asvitkine@/isherman@: Can you please have another look at:
 - chrome/*

sadrul@chromium.org: Can you please have another look at:
 - ui/*

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

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:21: #if
defined(USE_OZONE) || defined(USE_X11)
On 2015/07/07 22:00:58, Alexei Svitkine wrote:
> Nit: I don't think it's necessary to guard forward declarations. It will still
> compile without the guards - they just make this code messier.

Done.

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
(right):

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:24:
class TestScreen : public gfx::Screen {
On 2015/07/07 22:00:58, Alexei Svitkine wrote:
> Seems like we have a number of these TestScreen subclasses already in the
> codebase.
> 
> e.g.:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> Can we move it to its own file and share the same class from different tests?

I've created a simple gfx::test::TestScreen and updated two existing tests to
use it.  There are still two custom test Screens, including the one you linked
to, that are more complex and not functionally equivalent.

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:32:
<< "Function not implemented!!! 'TestScreen::GetCursorScreenPoint()'";
On 2015/07/07 22:00:58, Alexei Svitkine wrote:
> Nit: No need for these messages. I think when NOTREACHED() is hit, you get the
> stack trace. So no need to mention the function name. And then, "Function not
> implemented" is redundant since it's a NOTREACHED() already.

Done.

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:39:
return NULL;
On 2015/07/07 22:06:22, Ilya Sherman (Away July 8-13) wrote:
> nit: nullptr (pretty much everywhere where you're using NULL now)

Done.

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:107:
#endif  // defined(USE_OZONE) || defined(USE_X11)
On 2015/07/07 22:00:58, Alexei Svitkine wrote:
> This is a lot of extra complexity being put into the chrome/ code. Can this
> complexity be tucked away in the ui test helper?
> 
> i.e. can ui::test::DeviceDataManagerTestAPI own the device_data_manager and
also
> have the ifdefs about ozone? So that this code can just have its instance
around
> without ifdefs?

Done.

https://codereview.chromium.org/1182303005/diff/180001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:131:
ui::test::DeviceDataManagerTestAPI::SetDeviceDataManagerInstance(nullptr);
On 2015/07/07 22:00:58, Alexei Svitkine wrote:
> Can this be handled in the
> DeviceDataManagerTestAPI::SetDeviceDataManagerInstance() implementation, since
> it's already a helper for testing?

Done.

https://codereview.chromium.org/1182303005/diff/180001/ui/events/devices/devi...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/1182303005/diff/180001/ui/events/devices/devi...
ui/events/devices/device_data_manager.cc:38: CHECK(!instance || !instance_)
On 2015/07/07 22:00:59, Alexei Svitkine wrote:
> Nit: I think this should be a DCHECK().

Done.

https://codereview.chromium.org/1182303005/diff/180001/ui/events/devices/devi...
ui/events/devices/device_data_manager.cc:39: << "Can not set multiple instances
of DeviceDataManager";
On 2015/07/07 22:04:39, sadrul wrote:
> Have separate DCHECK()s with the right message instead? "Cannot set multiple
...
> " and "Must be non-null"

Actually the logic is, 'instance' can be null, or 'instance_' cannot be null. 
The tests need to be able to explicitly reset the instance to something else. I
have updated the message though.  WDYT?

https://codereview.chromium.org/1182303005/diff/180001/ui/events/test/device_...
File ui/events/test/device_data_manager_test_api.cc (right):

https://codereview.chromium.org/1182303005/diff/180001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.cc:31: }
On 2015/07/07 22:00:59, Alexei Svitkine wrote:
> Nit: Add blank lines between functions.

Done.  Weird, I don't know how those got removed....

[Alexei Svitkine (slow), 2015-07-09 15:17:06]

Thanks, this looks a lot cleaner! Just a few more comments.

https://codereview.chromium.org/1182303005/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
(right):

https://codereview.chromium.org/1182303005/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:31:
scoped_ptr<ui::test::DeviceDataManagerTestAPI> device_data_manager_test_api_;
Does this need to be a scoped_ptr? Seems like a direct member would work.

https://codereview.chromium.org/1182303005/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:38:
scoped_ptr<gfx::test::TestScreen> test_screen_;
Ditto.

https://codereview.chromium.org/1182303005/diff/200001/ui/events/test/device_...
File ui/events/test/device_data_manager_test_api.cc (right):

https://codereview.chromium.org/1182303005/diff/200001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.cc:7: #define
DEVICE_DATA_MANAGER_IS_AVAILABLE() \
Nit: I would define this without ()'s. I think ()'s in macro names are for
macros that are more like functions - i.e. allow runtime checks. But in this
case, you're using for compile time checks - so no need for ()'s.

https://codereview.chromium.org/1182303005/diff/200001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.cc:40: 
Nit: Remove empty line.

https://codereview.chromium.org/1182303005/diff/200001/ui/gfx/test/test_screen.h
File ui/gfx/test/test_screen.h (right):

https://codereview.chromium.org/1182303005/diff/200001/ui/gfx/test/test_scree...
ui/gfx/test/test_screen.h:25: gfx::Display& display() { return display_; }
I believe non-const refs are discouraged. Return a pointer instead.

[bruthig, 2015-07-10 15:38:42]

https://codereview.chromium.org/1182303005/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc
(right):

https://codereview.chromium.org/1182303005/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:31:
scoped_ptr<ui::test::DeviceDataManagerTestAPI> device_data_manager_test_api_;
On 2015/07/09 15:17:05, Alexei Svitkine wrote:
> Does this need to be a scoped_ptr? Seems like a direct member would work.

Done.

https://codereview.chromium.org/1182303005/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc:38:
scoped_ptr<gfx::test::TestScreen> test_screen_;
On 2015/07/09 15:17:05, Alexei Svitkine wrote:
> Ditto.

Done.

https://codereview.chromium.org/1182303005/diff/200001/ui/events/test/device_...
File ui/events/test/device_data_manager_test_api.cc (right):

https://codereview.chromium.org/1182303005/diff/200001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.cc:7: #define
DEVICE_DATA_MANAGER_IS_AVAILABLE() \
On 2015/07/09 15:17:05, Alexei Svitkine wrote:
> Nit: I would define this without ()'s. I think ()'s in macro names are for
> macros that are more like functions - i.e. allow runtime checks. But in this
> case, you're using for compile time checks - so no need for ()'s.

Done.

https://codereview.chromium.org/1182303005/diff/200001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.cc:40: 
On 2015/07/09 15:17:05, Alexei Svitkine wrote:
> Nit: Remove empty line.

Done.

https://codereview.chromium.org/1182303005/diff/200001/ui/gfx/test/test_screen.h
File ui/gfx/test/test_screen.h (right):

https://codereview.chromium.org/1182303005/diff/200001/ui/gfx/test/test_scree...
ui/gfx/test/test_screen.h:25: gfx::Display& display() { return display_; }
On 2015/07/09 15:17:05, Alexei Svitkine wrote:
> I believe non-const refs are discouraged. Return a pointer instead.

Done.

[Alexei Svitkine (slow), 2015-07-10 15:50:59]

LGTM

[bruthig, 2015-07-10 16:43:50]

bruthig@chromium.org changed reviewers:
+ ddorwin@chromium.org, dfalcantara@chromium.org

[bruthig, 2015-07-10 16:43:51]

ddorwin@chromium.org: Please review changes in 
- content/browser/media/capture/web_contents_video_capture_device_unittest.cc

dfalcantara@chromium.org: Please review changes in 
- chrome/browser/manifest/manifest_icon_selector_unittest.cc

[gone, 2015-07-10 17:16:33]

manifest_icon_selector_unittest.cc lgtm

[ddorwin, 2015-07-10 17:33:49]

ddorwin@chromium.org changed reviewers:
+ miu@chromium.org
- ddorwin@chromium.org

[ddorwin, 2015-07-10 17:34:13]

ddorwin@chromium.org changed reviewers:
+ ddorwin@chromium.org

[ddorwin, 2015-07-10 17:34:13]

+miu to review capture/.

On 2015/07/10 16:43:51, bruthig wrote:
> mailto:ddorwin@chromium.org: Please review changes in 
> - content/browser/media/capture/web_contents_video_capture_device_unittest.cc

[sadrul, 2015-07-10 17:35:48]

Not in this CL, but maybe aura::TestScreen could inherit from gfx::TestScreen?

https://codereview.chromium.org/1182303005/diff/220001/ui/events/devices/devi...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/1182303005/diff/220001/ui/events/devices/devi...
ui/events/devices/device_data_manager.cc:40: "set to nullptr first.";
This should be more like:

  DCHECK(instance) << "Must reset the DDM instance using DeleteInstance()
instead.";
  DCHECK(instance_) << "Cannot set multiple instances of DDM.";

https://codereview.chromium.org/1182303005/diff/220001/ui/events/devices/devi...
ui/events/devices/device_data_manager.cc:211: 
This can possibly be templatized. Not sure if that would cleaner.

https://codereview.chromium.org/1182303005/diff/220001/ui/events/test/device_...
File ui/events/test/device_data_manager_test_api.cc (right):

https://codereview.chromium.org/1182303005/diff/220001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.cc:94: 
Split this into two files: device_data_manager_test_api_stub.cc, and
device_data_manager_test_api_impl.cc, and conditionally include them from gyp/gn

[ddorwin, 2015-07-10 17:38:36]

ddorwin@chromium.org changed reviewers:
- ddorwin@chromium.org

[bruthig, 2015-07-10 20:04:58]

sadrul@, I've addressed your comments from patch set 12.  Can you please take a
look at the delta?

https://codereview.chromium.org/1182303005/diff/220001/ui/events/devices/devi...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/1182303005/diff/220001/ui/events/devices/devi...
ui/events/devices/device_data_manager.cc:40: "set to nullptr first.";
On 2015/07/10 17:35:48, sadrul wrote:
> This should be more like:
> 
>   DCHECK(instance) << "Must reset the DDM instance using DeleteInstance()
> instead.";
>   DCHECK(instance_) << "Cannot set multiple instances of DDM.";

Done.

https://codereview.chromium.org/1182303005/diff/220001/ui/events/devices/devi...
ui/events/devices/device_data_manager.cc:211: 
On 2015/07/10 17:35:48, sadrul wrote:
> This can possibly be templatized. Not sure if that would cleaner.

Done.

https://codereview.chromium.org/1182303005/diff/220001/ui/events/test/device_...
File ui/events/test/device_data_manager_test_api.cc (right):

https://codereview.chromium.org/1182303005/diff/220001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.cc:94: 
On 2015/07/10 17:35:48, sadrul wrote:
> Split this into two files: device_data_manager_test_api_stub.cc, and
> device_data_manager_test_api_impl.cc, and conditionally include them from
gyp/gn

Done.

[miu, 2015-07-10 20:50:08]

content/browser/media/capture/web_contents_video_capture_device_unittest.cc lgtm

Thanks for the clean-up!  :)

[sadrul, 2015-07-10 21:57:34]

https://codereview.chromium.org/1182303005/diff/240001/ui/events/test/device_...
File ui/events/test/device_data_manager_test_api.h (right):

https://codereview.chromium.org/1182303005/diff/240001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.h:39: bool
CreateDeviceDataManagerInstance();
Why does delete_instance need to be passed-in to the ctor? Can this instead keep
track of whether it created an instance, and if it did, delete it during
destruction?

[bruthig, 2015-07-10 22:27:28]

sadrul@, can you PTAL?

https://codereview.chromium.org/1182303005/diff/240001/ui/events/test/device_...
File ui/events/test/device_data_manager_test_api.h (right):

https://codereview.chromium.org/1182303005/diff/240001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.h:39: bool
CreateDeviceDataManagerInstance();
On 2015/07/10 21:57:34, sadrul wrote:
> Why does delete_instance need to be passed-in to the ctor? Can this instead
keep
> track of whether it created an instance, and if it did, delete it during
> destruction?

Done.

[sadrul, 2015-07-10 22:29:54]

lgtm

https://codereview.chromium.org/1182303005/diff/260001/ui/events/test/device_...
File ui/events/test/device_data_manager_test_api.h (right):

https://codereview.chromium.org/1182303005/diff/260001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.h:23: explicit
DeviceDataManagerTestAPI();
No explicit

[bruthig, 2015-07-10 22:42:09]

https://codereview.chromium.org/1182303005/diff/260001/ui/events/test/device_...
File ui/events/test/device_data_manager_test_api.h (right):

https://codereview.chromium.org/1182303005/diff/260001/ui/events/test/device_...
ui/events/test/device_data_manager_test_api.h:23: explicit
DeviceDataManagerTestAPI();
On 2015/07/10 22:29:54, sadrul wrote:
> No explicit

Done.

[bruthig, 2015-07-10 22:42:18]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-07-10 22:42:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, rbyers@chromium.org, asvitkine@chromium.org,
dfalcantara@chromium.org, miu@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1182303005/#ps280001
(title: "Removed explicit.")

[commit-bot: I haz the power, 2015-07-10 22:43:13]

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

[commit-bot: I haz the power, 2015-07-10 22:46:22]

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

[commit-bot: I haz the power, 2015-07-10 22:46:24]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

(exceeded global retry quota)

[bruthig, 2015-07-10 23:00:16]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-07-10 23:00:17]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
rbyers@chromium.org, asvitkine@chromium.org, miu@chromium.org,
isherman@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1182303005/#ps300001
(title: "Rebased on master.")

[commit-bot: I haz the power, 2015-07-10 23:00:44]

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

[commit-bot: I haz the power, 2015-07-10 23:09:01]

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

[commit-bot: I haz the power, 2015-07-10 23:09:03]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

(exceeded global retry quota)

[bruthig, 2015-07-10 23:31:39]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-07-10 23:31:39]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
rbyers@chromium.org, asvitkine@chromium.org, miu@chromium.org,
isherman@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1182303005/#ps320001
(title: "Added logging.h include to device_data_manager_test_api_stub.cc.")

[commit-bot: I haz the power, 2015-07-10 23:32:18]

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

[commit-bot: I haz the power, 2015-07-11 00:52:50]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-07-11 00:53:58]

Patchset 17 (id:??) landed as
https://crrev.com/86fbc49b97910112e68ad53dde3d7c9cdf63eef8
Cr-Commit-Position: refs/heads/master@{#338414}

[Adrian Kuegel, 2015-07-13 08:18:25]

On 2015/07/11 00:53:58, commit-bot: I haz the power wrote:
> Patchset 17 (id:??) landed as
> https://crrev.com/86fbc49b97910112e68ad53dde3d7c9cdf63eef8
> Cr-Commit-Position: refs/heads/master@{#338414}

This CL has broken the ChromiumOS Codesearch bot:
https://build.chromium.org/p/chromium.infra.cron/builders/ChromiumOS%20Codese...

[bruthig, 2015-07-13 22:57:07]

On 2015/07/13 08:18:25, Adrian Kuegel wrote:
> On 2015/07/11 00:53:58, commit-bot: I haz the power wrote:
> > Patchset 17 (id:??) landed as
> > https://crrev.com/86fbc49b97910112e68ad53dde3d7c9cdf63eef8
> > Cr-Commit-Position: refs/heads/master@{#338414}
> 
> This CL has broken the ChromiumOS Codesearch bot:
>
https://build.chromium.org/p/chromium.infra.cron/builders/ChromiumOS%20Codese...

CL to fix the errors can be found here:
https://codereview.chromium.org/1236073002/