[sensors] Add a browser test to sanity check ambient light sensor.

[sensors] Add a browser test to sanity check ambient light sensor.

The idea of the test is to sanity check the entire generic sensor
integration especially to verify that the data put in the shared
buffer is correctly handled by Blink.

The patch add a way to provide a fake provider which will create
fake sensors so we have a reliable and hardware independent solution.

The rest of the testing is happening either as layout tests or
as unittests in device/generic_sensor.

BUG=606766
Committed: https://crrev.com/17dc30da8eac6754b74cadf5a530cad8c14c22f8
Cr-Commit-Position: refs/heads/master@{#428025}

[darktears, 2016-10-19 20:35:57]

alexis.menard@intel.com changed reviewers:
+ mikhail.pozdnyakov@intel.com, phajdan.jr@chromium.org, reillyg@chromium.org,
timvolodine@chomium.org

[darktears, 2016-10-19 20:35:58]

I'm not sure the location is the appropriate place. I can't put the browser test
in device/ because it depends on content/ so if it's not the appropriate place
where it could go?

[Reilly Grant (use Gerrit), 2016-10-19 21:28:23]

The placement is awkward because this should be a layout test using mocked Mojo
services. It looks like these already exist so I'm not sure of the value these
tests provide. What was not previously being tested?

[darktears, 2016-10-19 22:29:22]

On 2016/10/19 at 21:28:23, reillyg wrote:
> The placement is awkward because this should be a layout test using mocked
Mojo services. It looks like these already exist so I'm not sure of the value
these tests provide. What was not previously being tested?

This test end to end from Javascript. The current tests in device/generic_sensor
doesn't exercise the entire content/ layer + Blink bindings. A LayoutTest is
also not appropriate because we need to have the provider returning fake values,
otherwise it's hardware dependent.

[Reilly Grant (use Gerrit), 2016-10-20 00:01:27]

On 2016/10/19 at 22:29:22, alexis.menard wrote:
> On 2016/10/19 at 21:28:23, reillyg wrote:
> > The placement is awkward because this should be a layout test using mocked
Mojo services. It looks like these already exist so I'm not sure of the value
these tests provide. What was not previously being tested?
> 
> This test end to end from Javascript. The current tests in
device/generic_sensor doesn't exercise the entire content/ layer + Blink
bindings. A LayoutTest is also not appropriate because we need to have the
provider returning fake values, otherwise it's hardware dependent.

But what you've discovered in trying to find a place to put these tests is that
there is no content/ layer.

Your LayoutTest doesn't have to be hardware dependent if they're running against
a mocked implementation of the Mojo sensor service. Look at the existing tests
in LayoutTests/sensor.

[Mikhail, 2016-10-20 13:32:16]

On 2016/10/20 00:01:27, Reilly Grant wrote:
> On 2016/10/19 at 22:29:22, alexis.menard wrote:
> > On 2016/10/19 at 21:28:23, reillyg wrote:
> > > The placement is awkward because this should be a layout test using mocked
> Mojo services. It looks like these already exist so I'm not sure of the value
> these tests provide. What was not previously being tested?
> > 
> > This test end to end from Javascript. The current tests in
> device/generic_sensor doesn't exercise the entire content/ layer + Blink
> bindings. A LayoutTest is also not appropriate because we need to have the
> provider returning fake values, otherwise it's hardware dependent.
> 
> But what you've discovered in trying to find a place to put these tests is
that
> there is no content/ layer.
> 
> Your LayoutTest doesn't have to be hardware dependent if they're running
against
> a mocked implementation of the Mojo sensor service. Look at the existing tests
> in LayoutTests/sensor.

So far we can only test blink and platform parts of generic sensor framework
separately from each other. This can lead to unnoticed regressions in case the
functional code behaviour differs from the behaviour implemented in the mock
testing backends.

The intention of this test is to catch such situations, for example check that
if test data is put to shared buffer from browser process via the actual (not
mocked) codepath, then it is read and handled properly in Blink.

Would it be possible to find a place for such tests somewhere inside device/
directory? Thanks.

[darktears, 2016-10-21 16:27:47]

On 2016/10/20 at 13:32:16, mikhail.pozdnyakov wrote:
> On 2016/10/20 00:01:27, Reilly Grant wrote:
> > On 2016/10/19 at 22:29:22, alexis.menard wrote:
> > > On 2016/10/19 at 21:28:23, reillyg wrote:
> > > > The placement is awkward because this should be a layout test using
mocked
> > Mojo services. It looks like these already exist so I'm not sure of the
value
> > these tests provide. What was not previously being tested?
> > > 
> > > This test end to end from Javascript. The current tests in
> > device/generic_sensor doesn't exercise the entire content/ layer + Blink
> > bindings. A LayoutTest is also not appropriate because we need to have the
> > provider returning fake values, otherwise it's hardware dependent.
> > 
> > But what you've discovered in trying to find a place to put these tests is
that
> > there is no content/ layer.
> > 
> > Your LayoutTest doesn't have to be hardware dependent if they're running
against
> > a mocked implementation of the Mojo sensor service. Look at the existing
tests
> > in LayoutTests/sensor.
> 
> So far we can only test blink and platform parts of generic sensor framework
separately from each other. This can lead to unnoticed regressions in case the
functional code behaviour differs from the behaviour implemented in the mock
testing backends.
> 
> The intention of this test is to catch such situations, for example check that
if test data is put to shared buffer from browser process via the actual (not
mocked) codepath, then it is read and handled properly in Blink.
> 
> Would it be possible to find a place for such tests somewhere inside device/
directory? Thanks.

@timvolodine : do you have any opinion?

[Reilly Grant (use Gerrit), 2016-10-21 17:32:56]

I talked to rockot@ about this and the conclusion we came to was that there is a
small value in testing that the Mojo services are registered properly but that
the proper place for extensive testing is in LayoutTests and service unittests.
So it's okay to add some small tests that sanity check the integration but it
should not be a full test suite for the feature.

[darktears, 2016-10-21 20:49:35]

Description was changed from

==========
[sensors] Introduce infrastructure to do browser tests for generic sensors with
a simple test.

Add a way to provide a fake provider which will create fake sensors so
we can reliably and cross platform test the entire feature. A simple
test for ambient light was added, more are going to be added in follow up CLs
(e.g. testing error reporting).

BUG=606766
==========

to

==========
[sensors] Add a browser test to sanity check ambient light sensor.
    
The idea of the test is to sanity check the entire generic sensor
integration especially to verify that the data put in the shared
buffer is correctly handled by Blink.
    
The patch add a way to provide a fake provider which will create
fake sensors so we have a reliable and hardware independent solution.
    
The rest of the testing is happening either as layout tests or
as unittests in device/generic_sensor.
    
BUG=606766
==========

[darktears, 2016-10-21 20:51:30]

On 2016/10/21 at 17:32:56, reillyg wrote:
> I talked to rockot@ about this and the conclusion we came to was that there is
a small value in testing that the Mojo services are registered properly but that
the proper place for extensive testing is in LayoutTests and service unittests.
So it's okay to add some small tests that sanity check the integration but it
should not be a full test suite for the feature.

Sounds good to me. Let's do a simple sanity check here that things behave as
expected especially regarding the passing the values of the sensor with the
shared buffer. We'll keep the advanced testing (behavior, APIs) in the other
places you mentioned. I have updated the CL Description/commit message
explaining the reasoning.

So based on that, what would be the appropriate place for that test?

[darktears, 2016-10-21 20:51:49]

The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 20:52:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 21:27:39]

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

[commit-bot: I haz the power, 2016-10-21 21:27:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Reilly Grant (use Gerrit), 2016-10-21 21:49:18]

On 2016/10/21 at 20:51:30, alexis.menard wrote:
> On 2016/10/21 at 17:32:56, reillyg wrote:
> > I talked to rockot@ about this and the conclusion we came to was that there
is a small value in testing that the Mojo services are registered properly but
that the proper place for extensive testing is in LayoutTests and service
unittests. So it's okay to add some small tests that sanity check the
integration but it should not be a full test suite for the feature.
> 
> Sounds good to me. Let's do a simple sanity check here that things behave as
expected especially regarding the passing the values of the sensor with the
shared buffer. We'll keep the advanced testing (behavior, APIs) in the other
places you mentioned. I have updated the CL Description/commit message
explaining the reasoning.
> 
> So based on that, what would be the appropriate place for that test?

I would put the browser tests in content/browser/generic_sensor_browsertest.cc.
I'm almost tempted to say that these should go in
render_frame_host_impl_browsertest.cc because the only line these tests add
coverage for is render_frame_host_impl.cc:2205 but we probably shouldn't clutter
that file with integration tests for every single feature.

[darktears, 2016-10-21 21:59:07]

The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 22:00:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[darktears, 2016-10-21 22:47:49]

The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 22:48:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[darktears, 2016-10-21 23:36:18]

On 2016/10/21 21:49:18, Reilly Grant wrote:
> On 2016/10/21 at 20:51:30, alexis.menard wrote:
> > On 2016/10/21 at 17:32:56, reillyg wrote:
> > > I talked to rockot@ about this and the conclusion we came to was that
there
> is a small value in testing that the Mojo services are registered properly but
> that the proper place for extensive testing is in LayoutTests and service
> unittests. So it's okay to add some small tests that sanity check the
> integration but it should not be a full test suite for the feature.
> > 
> > Sounds good to me. Let's do a simple sanity check here that things behave as
> expected especially regarding the passing the values of the sensor with the
> shared buffer. We'll keep the advanced testing (behavior, APIs) in the other
> places you mentioned. I have updated the CL Description/commit message
> explaining the reasoning.
> > 
> > So based on that, what would be the appropriate place for that test?
> 
> I would put the browser tests in
content/browser/generic_sensor_browsertest.cc.
> I'm almost tempted to say that these should go in
> render_frame_host_impl_browsertest.cc because the only line these tests add
> coverage for is render_frame_host_impl.cc:2205 but we probably shouldn't
clutter
> that file with integration tests for every single feature.

@mikhail @reillyg @rockot : I moved the test to the suggested place. Please have
a look.

[commit-bot: I haz the power, 2016-10-22 01:34:39]

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

[commit-bot: I haz the power, 2016-10-22 01:34:40]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-10-22 02:31:37]

https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
File content/browser/generic_sensor_browsertest.cc (right):

https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
content/browser/generic_sensor_browsertest.cc:42: (base::TimeTicks::Now() -
base::TimeTicks()).InSecondsF();
base::TimeTicks() is 0 so why not base::TimeTicks::Now().InSecondsF()?

https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
content/browser/generic_sensor_browsertest.cc:90: :
fake_sensor_provider_(nullptr),
nit: This initialization is unnecessary.

https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
content/browser/generic_sensor_browsertest.cc:110: fake_sensor_provider_);
Set the testing provider back to null when cleaning up the test to prevent
testing state from leaking into the next test to run in this process.

[Mikhail, 2016-10-24 06:41:56]

https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
File content/browser/generic_sensor_browsertest.cc (right):

https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
content/browser/generic_sensor_browsertest.cc:42: (base::TimeTicks::Now() -
base::TimeTicks()).InSecondsF();
On 2016/10/22 02:31:36, Reilly Grant wrote:
> base::TimeTicks() is 0 so why not base::TimeTicks::Now().InSecondsF()?

TimeTicks does not contain 'InSecondsF' or similar methods, so need to obtain a
'TimeDelta' instance. 'base::TimeTicks::Now() - base::TimeTicks()' returns
'TimeDelta'.

https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
content/browser/generic_sensor_browsertest.cc:115: FakeSensorProvider*
fake_sensor_provider_;
unique_ptr ?

https://codereview.chromium.org/2434563005/diff/60001/device/generic_sensor/p...
File device/generic_sensor/platform_sensor_provider.h (right):

https://codereview.chromium.org/2434563005/diff/60001/device/generic_sensor/p...
device/generic_sensor/platform_sensor_provider.h:23: static void
SetProviderForTesting(PlatformSensorProvider* provider);
nit: a short description is welcome

[darktears, 2016-10-24 20:47:03]

On 2016/10/22 at 02:31:37, reillyg wrote:
>
https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
> File content/browser/generic_sensor_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
> content/browser/generic_sensor_browsertest.cc:42: (base::TimeTicks::Now() -
base::TimeTicks()).InSecondsF();
> base::TimeTicks() is 0 so why not base::TimeTicks::Now().InSecondsF()?
> 
>
https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
> content/browser/generic_sensor_browsertest.cc:90: :
fake_sensor_provider_(nullptr),
> nit: This initialization is unnecessary.

Fixed.

> 
>
https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
> content/browser/generic_sensor_browsertest.cc:110: fake_sensor_provider_);
> Set the testing provider back to null when cleaning up the test to prevent
testing state from leaking into the next test to run in this process.

Fixed.

[darktears, 2016-10-24 20:56:12]

On 2016/10/24 at 06:41:56, mikhail.pozdnyakov wrote:
>
https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
> File content/browser/generic_sensor_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
> content/browser/generic_sensor_browsertest.cc:42: (base::TimeTicks::Now() -
base::TimeTicks()).InSecondsF();
> On 2016/10/22 02:31:36, Reilly Grant wrote:
> > base::TimeTicks() is 0 so why not base::TimeTicks::Now().InSecondsF()?
> 
> TimeTicks does not contain 'InSecondsF' or similar methods, so need to obtain
a 'TimeDelta' instance. 'base::TimeTicks::Now() - base::TimeTicks()' returns
'TimeDelta'.
> 
>
https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
> content/browser/generic_sensor_browsertest.cc:115: FakeSensorProvider*
fake_sensor_provider_;
> unique_ptr ?

PlatformSensorProvider are expected to be a singleton and static therefore to be
always around. Using unique_ptr will destroy the object when
GenericSensorBrowserTest is deleted however it will be deleted in the wrong
thread (not IO) which asserts. If I delete it in the IO thread before the
teardown it doesn't work as well the cleanup around generic_sensor is not yet
finished and expect the provider to be around. I think it's fine to let it like
this (technically leaking) or we should make it static so it's deallocated
properly.

> 
>
https://codereview.chromium.org/2434563005/diff/60001/device/generic_sensor/p...
> File device/generic_sensor/platform_sensor_provider.h (right):
> 
>
https://codereview.chromium.org/2434563005/diff/60001/device/generic_sensor/p...
> device/generic_sensor/platform_sensor_provider.h:23: static void
SetProviderForTesting(PlatformSensorProvider* provider);
> nit: a short description is welcome

Fixed

[darktears, 2016-10-24 20:58:07]

The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 20:59:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 21:06:52]

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

[commit-bot: I haz the power, 2016-10-24 21:06:53]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Reilly Grant (use Gerrit), 2016-10-24 21:24:07]

On 2016/10/24 at 20:56:12, alexis.menard wrote:
> On 2016/10/24 at 06:41:56, mikhail.pozdnyakov wrote:
> >
https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
> > content/browser/generic_sensor_browsertest.cc:115: FakeSensorProvider*
fake_sensor_provider_;
> > unique_ptr ?
> 
> PlatformSensorProvider are expected to be a singleton and static therefore to
be always around. Using unique_ptr will destroy the object when
GenericSensorBrowserTest is deleted however it will be deleted in the wrong
thread (not IO) which asserts. If I delete it in the IO thread before the
teardown it doesn't work as well the cleanup around generic_sensor is not yet
finished and expect the provider to be around. I think it's fine to let it like
this (technically leaking) or we should make it static so it's deallocated
properly.

The LSan bots will likely start to complain if we introduce a memory leak. I
looked through some code I've written that solves this problem to try to give
you some ideas and all the examples I can find just do cleanup on the main
thread.

[darktears, 2016-10-24 21:24:18]

On 2016/10/24 at 21:06:53, commit-bot wrote:
> Dry run: Try jobs failed on following builders:
>   ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

@reillyg : feel free to have a look.

[darktears, 2016-10-24 21:24:31]

The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 21:25:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 21:33:08]

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

[commit-bot: I haz the power, 2016-10-24 21:33:09]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[darktears, 2016-10-25 18:17:59]

On 2016/10/24 at 21:24:07, reillyg wrote:
> On 2016/10/24 at 20:56:12, alexis.menard wrote:
> > On 2016/10/24 at 06:41:56, mikhail.pozdnyakov wrote:
> > >
https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
> > > content/browser/generic_sensor_browsertest.cc:115: FakeSensorProvider*
fake_sensor_provider_;
> > > unique_ptr ?
> > 
> > PlatformSensorProvider are expected to be a singleton and static therefore
to be always around. Using unique_ptr will destroy the object when
GenericSensorBrowserTest is deleted however it will be deleted in the wrong
thread (not IO) which asserts. If I delete it in the IO thread before the
teardown it doesn't work as well the cleanup around generic_sensor is not yet
finished and expect the provider to be around. I think it's fine to let it like
this (technically leaking) or we should make it static so it's deallocated
properly.
> 
> The LSan bots will likely start to complain if we introduce a memory leak. I
looked through some code I've written that solves this problem to try to give
you some ideas and all the examples I can find just do cleanup on the main
thread.

Not saying it's the right thing to do but
https://chromium.googlesource.com/experimental/chromium/src/+/master/content/...
leaks as well. Same issue as me.

[darktears, 2016-10-26 15:23:12]

The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 15:23:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[darktears, 2016-10-26 15:24:40]

On 2016/10/24 at 21:24:07, reillyg wrote:
> On 2016/10/24 at 20:56:12, alexis.menard wrote:
> > On 2016/10/24 at 06:41:56, mikhail.pozdnyakov wrote:
> > >
https://codereview.chromium.org/2434563005/diff/60001/content/browser/generic...
> > > content/browser/generic_sensor_browsertest.cc:115: FakeSensorProvider*
fake_sensor_provider_;
> > > unique_ptr ?
> > 
> > PlatformSensorProvider are expected to be a singleton and static therefore
to be always around. Using unique_ptr will destroy the object when
GenericSensorBrowserTest is deleted however it will be deleted in the wrong
thread (not IO) which asserts. If I delete it in the IO thread before the
teardown it doesn't work as well the cleanup around generic_sensor is not yet
finished and expect the provider to be around. I think it's fine to let it like
this (technically leaking) or we should make it static so it's deallocated
properly.
> 
> The LSan bots will likely start to complain if we introduce a memory leak. I
looked through some code I've written that solves this problem to try to give
you some ideas and all the examples I can find just do cleanup on the main
thread.

@reillyg : please have a look, the fake provider is now properly deleted.

[darktears, 2016-10-26 16:25:16]

The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 16:25:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mikhail, 2016-10-26 17:02:12]

https://codereview.chromium.org/2434563005/diff/120001/content/browser/generi...
File content/browser/generic_sensor_browsertest.cc (right):

https://codereview.chromium.org/2434563005/diff/120001/content/browser/generi...
content/browser/generic_sensor_browsertest.cc:114:
device::PlatformSensorProvider::SetProviderForTesting(nullptr);
^ this also should be done on IO thread.

Think it's better to have 

TearDownOnIOThread() {
device::PlatformSensorProvider::SetProviderForTesting(nullptr);
delete fake_sensor_provider_;
}

and schedule it from TearDown().

[commit-bot: I haz the power, 2016-10-26 17:34:54]

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

[commit-bot: I haz the power, 2016-10-26 17:34:56]

Dry run: This issue passed the CQ dry run.

[darktears, 2016-10-26 18:25:36]

On 2016/10/26 at 17:34:56, commit-bot wrote:
> Dry run: This issue passed the CQ dry run.

@mikhail : what about something like that?

[Reilly Grant (use Gerrit), 2016-10-26 19:55:26]

lgtm

https://codereview.chromium.org/2434563005/diff/140001/device/generic_sensor/...
File device/generic_sensor/platform_sensor_provider.cc (right):

https://codereview.chromium.org/2434563005/diff/140001/device/generic_sensor/...
device/generic_sensor/platform_sensor_provider.cc:16: }
nit: clang-format seems to be messing up inserting and removing newlines for
anonymous namespaces like this. See if you can get it to be consistent by adding
"// namespace" after the closing brace like we do for named namespaces.

[darktears, 2016-10-26 20:14:11]

Description was changed from

==========
[sensors] Add a browser test to sanity check ambient light sensor.
    
The idea of the test is to sanity check the entire generic sensor
integration especially to verify that the data put in the shared
buffer is correctly handled by Blink.
    
The patch add a way to provide a fake provider which will create
fake sensors so we have a reliable and hardware independent solution.
    
The rest of the testing is happening either as layout tests or
as unittests in device/generic_sensor.
    
BUG=606766
==========

to

==========
[sensors] Add a browser test to sanity check ambient light sensor.
    
The idea of the test is to sanity check the entire generic sensor
integration especially to verify that the data put in the shared
buffer is correctly handled by Blink.
    
The patch add a way to provide a fake provider which will create
fake sensors so we have a reliable and hardware independent solution.
    
The rest of the testing is happening either as layout tests or
as unittests in device/generic_sensor.
    
BUG=606766
==========

[darktears, 2016-10-26 20:14:11]

alexis.menard@intel.com changed reviewers:
+ jam@chromium.org, sky@chromium.org

[darktears, 2016-10-26 20:14:58]

On 2016/10/26 19:55:26, Reilly Grant wrote:
> lgtm
> 
>
https://codereview.chromium.org/2434563005/diff/140001/device/generic_sensor/...
> File device/generic_sensor/platform_sensor_provider.cc (right):
> 
>
https://codereview.chromium.org/2434563005/diff/140001/device/generic_sensor/...
> device/generic_sensor/platform_sensor_provider.cc:16: }
> nit: clang-format seems to be messing up inserting and removing newlines for
> anonymous namespaces like this. See if you can get it to be consistent by
adding
> "// namespace" after the closing brace like we do for named namespaces.

@jam : I need a LGTM for content/browser/generic_sensor_browsertest.cc

@sky or @pawel : I need a LGTM for content/test.

Thanks.

[darktears, 2016-10-26 20:15:24]

The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 20:15:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-10-26 21:09:49]

On 2016/10/26 20:14:58, darktears wrote:
> On 2016/10/26 19:55:26, Reilly Grant wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2434563005/diff/140001/device/generic_sensor/...
> > File device/generic_sensor/platform_sensor_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/2434563005/diff/140001/device/generic_sensor/...
> > device/generic_sensor/platform_sensor_provider.cc:16: }
> > nit: clang-format seems to be messing up inserting and removing newlines for
> > anonymous namespaces like this. See if you can get it to be consistent by
> adding
> > "// namespace" after the closing brace like we do for named namespaces.
> 
> @jam : I need a LGTM for content/browser/generic_sensor_browsertest.cc

should this be in content/browser/device_sensors?
> 
> @sky or @pawel : I need a LGTM for content/test.
> 
> Thanks.

[commit-bot: I haz the power, 2016-10-26 21:11:16]

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

[commit-bot: I haz the power, 2016-10-26 21:11:17]

Dry run: This issue passed the CQ dry run.

[darktears, 2016-10-26 21:20:59]

On 2016/10/26 at 21:09:49, jam wrote:
> On 2016/10/26 20:14:58, darktears wrote:
> > On 2016/10/26 19:55:26, Reilly Grant wrote:
> > > lgtm
> > > 
> > >
> >
https://codereview.chromium.org/2434563005/diff/140001/device/generic_sensor/...
> > > File device/generic_sensor/platform_sensor_provider.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/2434563005/diff/140001/device/generic_sensor/...
> > > device/generic_sensor/platform_sensor_provider.cc:16: }
> > > nit: clang-format seems to be messing up inserting and removing newlines
for
> > > anonymous namespaces like this. See if you can get it to be consistent by
> > adding
> > > "// namespace" after the closing brace like we do for named namespaces.
> > 
> > @jam : I need a LGTM for content/browser/generic_sensor_browsertest.cc
> 
> should this be in content/browser/device_sensors?

well device_sensors/ contains the "old" implementation, event based. Not sure
it's the appropriate place.

> > 
> > @sky or @pawel : I need a LGTM for content/test.
> > 
> > Thanks.

[sky, 2016-10-26 23:30:07]

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

[sky, 2016-10-26 23:30:07]

jam@ is an owner of content, so you don't need me for content/test. I'm removing
myself.

[jam, 2016-10-27 01:15:12]

On 2016/10/26 21:20:59, darktears wrote:
> On 2016/10/26 at 21:09:49, jam wrote:
> > On 2016/10/26 20:14:58, darktears wrote:
> > > On 2016/10/26 19:55:26, Reilly Grant wrote:
> > > > lgtm
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2434563005/diff/140001/device/generic_sensor/...
> > > > File device/generic_sensor/platform_sensor_provider.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2434563005/diff/140001/device/generic_sensor/...
> > > > device/generic_sensor/platform_sensor_provider.cc:16: }
> > > > nit: clang-format seems to be messing up inserting and removing newlines
> for
> > > > anonymous namespaces like this. See if you can get it to be consistent
by
> > > adding
> > > > "// namespace" after the closing brace like we do for named namespaces.
> > > 
> > > @jam : I need a LGTM for content/browser/generic_sensor_browsertest.cc
> > 
> > should this be in content/browser/device_sensors?
> 
> well device_sensors/ contains the "old" implementation, event based. Not sure
> it's the appropriate place.

so that directory will be removed soon? if so, lgtm

[Mikhail, 2016-10-27 06:01:45]

lgtm

[Paweł Hajdan Jr., 2016-10-27 08:09:33]

LGTM

[darktears, 2016-10-27 14:43:27]

The CQ bit was checked by alexis.menard@intel.com

[darktears, 2016-10-27 14:43:28]

The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2434563005/#ps160001
(title: "Fix nits found by Reilly")

[commit-bot: I haz the power, 2016-10-27 14:43:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 14:47:49]

Description was changed from

==========
[sensors] Add a browser test to sanity check ambient light sensor.
    
The idea of the test is to sanity check the entire generic sensor
integration especially to verify that the data put in the shared
buffer is correctly handled by Blink.
    
The patch add a way to provide a fake provider which will create
fake sensors so we have a reliable and hardware independent solution.
    
The rest of the testing is happening either as layout tests or
as unittests in device/generic_sensor.
    
BUG=606766
==========

to

==========
[sensors] Add a browser test to sanity check ambient light sensor.
    
The idea of the test is to sanity check the entire generic sensor
integration especially to verify that the data put in the shared
buffer is correctly handled by Blink.
    
The patch add a way to provide a fake provider which will create
fake sensors so we have a reliable and hardware independent solution.
    
The rest of the testing is happening either as layout tests or
as unittests in device/generic_sensor.
    
BUG=606766
==========

[commit-bot: I haz the power, 2016-10-27 14:47:51]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-27 14:51:11]

Description was changed from

==========
[sensors] Add a browser test to sanity check ambient light sensor.
    
The idea of the test is to sanity check the entire generic sensor
integration especially to verify that the data put in the shared
buffer is correctly handled by Blink.
    
The patch add a way to provide a fake provider which will create
fake sensors so we have a reliable and hardware independent solution.
    
The rest of the testing is happening either as layout tests or
as unittests in device/generic_sensor.
    
BUG=606766
==========

to

==========
[sensors] Add a browser test to sanity check ambient light sensor.

The idea of the test is to sanity check the entire generic sensor
integration especially to verify that the data put in the shared
buffer is correctly handled by Blink.

The patch add a way to provide a fake provider which will create
fake sensors so we have a reliable and hardware independent solution.

The rest of the testing is happening either as layout tests or
as unittests in device/generic_sensor.

BUG=606766

Committed: https://crrev.com/17dc30da8eac6754b74cadf5a530cad8c14c22f8
Cr-Commit-Position: refs/heads/master@{#428025}
==========

[commit-bot: I haz the power, 2016-10-27 14:51:13]

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