Replace device_sensor browsertest by service unittest.

Replace device_sensor browsertest by service unittest.

To decouple device_sensor browsertest from //content, we have to port it into
//service/device as a service unittest.

Browsertest is an end to end test but browsertest cannot be decoupled out of
//content. So we use 1)Service unittest plus 2)Layouttest to replace the
browsertest.

Service unittest simulates how render side call the mojo interface which is
provided by browser side. Layouttest verifys the logics from JS API of blink to
render layer. So Service unittest plus Layouttest have the same test effect as
browsertest.

The layouttests for device/sensors already exist, so here we only add the
service unittests.

All the test cases of service unittest run in the same process, while the IO
thread will be re-created when setup each test case. But in the product code,
there are many thread_checkers to make sure the DeviceSensorService instance runs in
the same thread. So we have to use UI thread to simulate the IO thread in order
to not breaking those DCHECKs.

The shared_memory_seqlock_reader.{cc|h} is used by both render side and service
unittest, so move it into //device/sensors/public/cpp/ folder.

BUG=694888

[ke.he, 2017-04-13 06:03:01]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 06:03:36]

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, 2017-04-13 06:07:35]

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

[commit-bot: I haz the power, 2017-04-13 06:07:36]

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

[ke.he, 2017-04-13 06:59:52]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 07:00:17]

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, 2017-04-13 07:12:49]

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

[commit-bot: I haz the power, 2017-04-13 07:12:50]

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

[ke.he, 2017-04-13 07:26:27]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 07:27:01]

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, 2017-04-13 08:02:26]

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

[commit-bot: I haz the power, 2017-04-13 08:02:27]

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_...)

[ke.he, 2017-04-13 08:29:39]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 08:30:08]

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, 2017-04-13 09:17:11]

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

[commit-bot: I haz the power, 2017-04-13 09:17:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ke.he, 2017-04-13 09:48:19]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 09:48:38]

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, 2017-04-13 10:41:36]

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

[commit-bot: I haz the power, 2017-04-13 10:41:37]

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

[ke.he, 2017-04-14 03:38:58]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 03:39:09]

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, 2017-04-14 03:43:29]

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

[commit-bot: I haz the power, 2017-04-14 03:43:30]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[ke.he, 2017-04-14 04:09:44]

Patchset #5 (id:80001) has been deleted

[ke.he, 2017-04-14 04:09:51]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 04:10:08]

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, 2017-04-14 05:06:59]

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

[commit-bot: I haz the power, 2017-04-14 05:07:00]

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

[ke.he, 2017-04-14 05:53:14]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 05:53:36]

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, 2017-04-14 06:54:02]

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

[commit-bot: I haz the power, 2017-04-14 06:54:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ke.he, 2017-04-17 03:23:01]

Patchset #7 (id:140001) has been deleted

[ke.he, 2017-04-17 03:23:13]

Patchset #7 (id:160001) has been deleted

[ke.he, 2017-04-17 03:23:26]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 03:23:33]

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, 2017-04-17 04:25:46]

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

[commit-bot: I haz the power, 2017-04-17 04:25:47]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-04-17 05:01:09]

ke.he@intel.com changed reviewers:
+ blundell@chromium.org

[ke.he, 2017-04-17 05:01:11]

Hi, Colin, PTAL, thanks!

[ke.he, 2017-04-17 05:29:44]

ke.he@intel.com changed reviewers:
+ reillyg@chromium.org

[ke.he, 2017-04-17 05:29:45]

Reillyg@ is working on the design doc to replace //device/sensor by
//device/generic_sensor.

Hi, Reillyg@, I think this won't effect your design much, right? Our purpose is
to decouple device sensor from //content. After this CL, it is quite
straightforward to move all code of //device/sensor into //service/device,

[Reilly Grant (use Gerrit), 2017-04-17 15:52:31]

On 2017/04/17 05:29:45, ke.he wrote:
> Reillyg@ is working on the design doc to replace //device/sensor by
> //device/generic_sensor.
> 
> Hi, Reillyg@, I think this won't effect your design much, right? Our purpose
is
> to decouple device sensor from //content. After this CL, it is quite
> straightforward to move all code of //device/sensor into //service/device,

This is fine work to do. I only recommend that:

a) We keep one browser test around to make sure that the Mojo service is
properly registered. Without a end-to-end tests we have broken APIs in
undetectable ways by changing Mojo service registration configuration.

b) See if any of the browser tests being removed can be added as layout tests.
Perhaps they are all duplicates of those.

[ke.he, 2017-04-18 03:38:03]

Description was changed from

==========
Replace device_sensor browsertest by service unittest.

To decouple device_sensor browsertest from //content, we have to port it into
//service/device as a service unittest.

All the test cases of service unittest run in the same process, while the IO
thread will be re-created when setup each test case. But in the product code,
there are many thread_checkers to make sure the DeviceSensorService instance
runs in
the same thread. So we have to use UI thread to simulate the IO thread in order
to not breaking those DCHECKs.

The shared_memory_seqlock_reader.{cc|h} is used by both render side and service
unittest, so move it into //device/sensors/public/cpp/ folder.

BUG=694888
==========

to

==========
Replace device_sensor browsertest by service unittest.

To decouple device_sensor browsertest from //content, we have to port it into
//service/device as a service unittest.

Browsertest is an end to end test but browsertest cannot be decoupled out of
//content. So we use 1)Service unittest plus 2)Layouttest to replace the
browsertest.

Service unittest simulates how render side call the mojo interface which is
provided by browser side. Layouttest verifys the logics from JS API of blink to
render layer. So Service unittest plus Layouttest have the same test effect as
browsertest.

The layouttests for device/sensors already exist, so here we only add the
service unittests.

All the test cases of service unittest run in the same process, while the IO
thread will be re-created when setup each test case. But in the product code,
there are many thread_checkers to make sure the DeviceSensorService instance
runs in
the same thread. So we have to use UI thread to simulate the IO thread in order
to not breaking those DCHECKs.

The shared_memory_seqlock_reader.{cc|h} is used by both render side and service
unittest, so move it into //device/sensors/public/cpp/ folder.

BUG=694888
==========

[ke.he, 2017-04-18 03:49:11]

On 2017/04/17 15:52:31, Reilly Grant wrote:
> On 2017/04/17 05:29:45, ke.he wrote:
> > Reillyg@ is working on the design doc to replace //device/sensor by
> > //device/generic_sensor.
> > 
> > Hi, Reillyg@, I think this won't effect your design much, right? Our purpose
> is
> > to decouple device sensor from //content. After this CL, it is quite
> > straightforward to move all code of //device/sensor into //service/device,
> 
> This is fine work to do. I only recommend that:
> 
> a) We keep one browser test around to make sure that the Mojo service is
> properly registered. Without a end-to-end tests we have broken APIs in
> undetectable ways by changing Mojo service registration configuration.
> 
> b) See if any of the browser tests being removed can be added as layout tests.
> Perhaps they are all duplicates of those.

Hi, Reilly@, Thanks! 
The CL description is updated. I didn't mean just to replace the browsertest by
service unittest only. For device sensor I found the layouttests for device
sensors already exist, so I just add the service unittest in this CL. Sorry I
didn't describe it clearly at the beginning.

[Reilly Grant (use Gerrit), 2017-04-18 19:36:18]

https://codereview.chromium.org/2812223006/diff/180001/services/device/device...
File services/device/device_service_test_base.cc (right):

https://codereview.chromium.org/2812223006/diff/180001/services/device/device...
services/device/device_service_test_base.cc:98: // each test case. So we decide
to use main thread to simulate IO thread.
// Instead of starting a new IO thread, we use the main thread to simulate 
// the IO thread. This is because there are some sub-services in device
// (DeviceSensorService, for example) which are of type
// base::Singleton<base::LeakySingletonTraits>, this means the singleton
// instance is never deleted. When all the service unit test cases run in
// same process the singleton service instance stays alive across multiple
// unit test cases. There are some thread_checkers in the singleton
// instance so we have to make sure passing the same IO thread to
// each test case. We use the main thread to provide this consistency.

https://codereview.chromium.org/2812223006/diff/180001/services/device/device...
File services/device/device_service_test_base.h (right):

https://codereview.chromium.org/2812223006/diff/180001/services/device/device...
services/device/device_service_test_base.h:9: #include
"base/memory/ref_counted.h"
This header is unused.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
File services/device/sensors/device_sensor_service_unittest.cc (right):

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:53: } break;
Instead of break, return true here (inside the {}).

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:82: return false;
Replace the return true at the end of this function with this return false.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:113: return true;
Same here, return true instead of break and return false at the end of the
function.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:142: void
SetOrientationBufferReady(
This function is identical to SetMotionBufferReady.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:205:
buffer->data.value = lux;
Why not set all_available_sensors_are_active like above?

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:211: // The
polling_stopped_callbacks should be run on the main thread.
|polling_stopped_callback_| should be run on the main thread.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:242: new
SharedMemorySeqLockReader<device::DeviceLightData>());
light_reader_ =
base::MakeUnique<SharedMemorySeqLockReader<device::DeviceLightData>>();

Also if this is wrapped onto two lines it should be within {}.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:257:
motion_reader_.reset(new SharedMemorySeqLockReader<device::MotionData>());
And here.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:273: new
SharedMemorySeqLockReader<device::OrientationData>());
And here.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:299: // In Non-Android
platform, SetUpFetcher() should be called in IO thread,
// On non-Android platforms, SetUpFetcher() should be called on the IO thread.
// In service unit tests the IO thread is the main thread so it is okay to call
// it directly. See comments in DeviceServiceTestBase::CreateService() for why
// the main thread is used as the IO thread.

[ke.he, 2017-04-19 02:57:09]

Patchset #8 (id:200001) has been deleted

[ke.he, 2017-04-19 02:58:11]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 02:58:30]

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, 2017-04-19 03:02:25]

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

[commit-bot: I haz the power, 2017-04-19 03:02:27]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ke.he, 2017-04-19 03:11:19]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 03:11:30]

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, 2017-04-19 03:55:20]

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

[commit-bot: I haz the power, 2017-04-19 03:55:20]

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

[ke.he, 2017-04-19 05:08:39]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 05:08:56]

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, 2017-04-19 06:14:49]

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

[commit-bot: I haz the power, 2017-04-19 06:14:50]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-04-19 06:49:02]

Reilly, Thanks very much!
CL updated, PTAL.

https://codereview.chromium.org/2812223006/diff/180001/services/device/device...
File services/device/device_service_test_base.cc (right):

https://codereview.chromium.org/2812223006/diff/180001/services/device/device...
services/device/device_service_test_base.cc:98: // each test case. So we decide
to use main thread to simulate IO thread.
On 2017/04/18 19:36:17, Reilly Grant wrote:
> // Instead of starting a new IO thread, we use the main thread to simulate 
> // the IO thread. This is because there are some sub-services in device
> // (DeviceSensorService, for example) which are of type
> // base::Singleton<base::LeakySingletonTraits>, this means the singleton
> // instance is never deleted. When all the service unit test cases run in
> // same process the singleton service instance stays alive across multiple
> // unit test cases. There are some thread_checkers in the singleton
> // instance so we have to make sure passing the same IO thread to
> // each test case. We use the main thread to provide this consistency.

Done.

https://codereview.chromium.org/2812223006/diff/180001/services/device/device...
File services/device/device_service_test_base.h (right):

https://codereview.chromium.org/2812223006/diff/180001/services/device/device...
services/device/device_service_test_base.h:9: #include
"base/memory/ref_counted.h"
On 2017/04/18 19:36:18, Reilly Grant wrote:
> This header is unused.

It should be moved into device_service_test_base.cc.
Done.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
File services/device/sensors/device_sensor_service_unittest.cc (right):

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:53: } break;
On 2017/04/18 19:36:18, Reilly Grant wrote:
> Instead of break, return true here (inside the {}).

Done.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:82: return false;
On 2017/04/18 19:36:18, Reilly Grant wrote:
> Replace the return true at the end of this function with this return false.

I also removed the "default: return false" above, otherwise the "return" at the
end of this function causes "unreached code" warning.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:113: return true;
On 2017/04/18 19:36:18, Reilly Grant wrote:
> Same here, return true instead of break and return false at the end of the
> function.

Done.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:142: void
SetOrientationBufferReady(
On 2017/04/18 19:36:18, Reilly Grant wrote:
> This function is identical to SetMotionBufferReady.

The parameter is different, one is "device::DeviceMotionHardwareBuffer", another
is "device::DeviceOrientationHardwareBuffer".

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:205:
buffer->data.value = lux;
On 2017/04/18 19:36:18, Reilly Grant wrote:
> Why not set all_available_sensors_are_active like above?

The buffer->data is actually in type of DeviceLightData. There is no
all_available_sensors_are_active member in DeviceLightData.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:211: // The
polling_stopped_callbacks should be run on the main thread.
On 2017/04/18 19:36:18, Reilly Grant wrote:
> |polling_stopped_callback_| should be run on the main thread.

Done.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:242: new
SharedMemorySeqLockReader<device::DeviceLightData>());
On 2017/04/18 19:36:18, Reilly Grant wrote:
> light_reader_ =
> base::MakeUnique<SharedMemorySeqLockReader<device::DeviceLightData>>();
> 
> Also if this is wrapped onto two lines it should be within {}.

Done.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:257:
motion_reader_.reset(new SharedMemorySeqLockReader<device::MotionData>());
On 2017/04/18 19:36:18, Reilly Grant wrote:
> And here.

Done.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:273: new
SharedMemorySeqLockReader<device::OrientationData>());
On 2017/04/18 19:36:18, Reilly Grant wrote:
> And here.

Done.

https://codereview.chromium.org/2812223006/diff/180001/services/device/sensor...
services/device/sensors/device_sensor_service_unittest.cc:299: // In Non-Android
platform, SetUpFetcher() should be called in IO thread,
On 2017/04/18 19:36:18, Reilly Grant wrote:
> // On non-Android platforms, SetUpFetcher() should be called on the IO thread.
> // In service unit tests the IO thread is the main thread so it is okay to
call
> // it directly. See comments in DeviceServiceTestBase::CreateService() for why
> // the main thread is used as the IO thread.

Done.

[blundell, 2017-04-19 09:15:54]

I'm slightly confused about what will happen to this test once the device
sensors impl and interface are eliminated. Reilly, is your vision that you will
port it to be a similar test of generic sensors rather than just eliminating it?
Otherwise, it seems like we could just leave the browsertest and then eliminate
that when the time comes.

[ke.he, 2017-04-19 09:57:18]

On 2017/04/19 09:15:54, blundell wrote:
> I'm slightly confused about what will happen to this test once the device
> sensors impl and interface are eliminated. Reilly, is your vision that you
will
> port it to be a similar test of generic sensors rather than just eliminating
it?
> Otherwise, it seems like we could just leave the browsertest and then
eliminate
> that when the time comes.

Hi, Colin and Reilly, The related work status is:

For //device/sensor, there are 2 tasks ongoing, but almost done.
1) Replace device sensor browsertest by service unittest. --- in reviewing.
2) Move //device/sensors into //services/device/. (it is the last step of device
sensor servicification)   --- depend on task 1) above, but almost done,
See(https://codereview.chromium.org/2819273006/#).

For //device/generic_sensor, similarly 2 tasks in plan:
3) Replace device generic sensor browsertest by service unittest. --- just
started.
4) Move //device/generic_sensors into //services/device/.  --- not start.

Should we finished the servification of device sensor first? because it is
almost done. And, should I continue the task 3)?

Thanks very much.

[blundell, 2017-04-19 15:10:40]

On 2017/04/19 09:57:18, ke.he wrote:
> On 2017/04/19 09:15:54, blundell wrote:
> > I'm slightly confused about what will happen to this test once the device
> > sensors impl and interface are eliminated. Reilly, is your vision that you
> will
> > port it to be a similar test of generic sensors rather than just eliminating
> it?
> > Otherwise, it seems like we could just leave the browsertest and then
> eliminate
> > that when the time comes.
> 
> Hi, Colin and Reilly, The related work status is:
> 
> For //device/sensor, there are 2 tasks ongoing, but almost done.
> 1) Replace device sensor browsertest by service unittest. --- in reviewing.
> 2) Move //device/sensors into //services/device/. (it is the last step of
device
> sensor servicification)   --- depend on task 1) above, but almost done,
> See(https://codereview.chromium.org/2819273006/#).
> 
> For //device/generic_sensor, similarly 2 tasks in plan:
> 3) Replace device generic sensor browsertest by service unittest. --- just
> started.
> 4) Move //device/generic_sensors into //services/device/.  --- not start.
> 
> Should we finished the servification of device sensor first? because it is
> almost done. And, should I continue the task 3)?
> 
> Thanks very much.

Based on comments #31 and #32 in crbug.com/612322, I would have said that we
should mark the device sensor servicification bugs as obsolete and replace them
with a bug to eliminate usage of device sensors in favor of usage of generic
sensors (Reilly, does such a bug exist?). However, I'm curious for Reilly's
response to my question above, as proceeding with this CL doesn't match my
understanding based on those comments.

Steps (3) and (4) seem like great work to pursue independent of the resolution
of how/whether to proceed on device sensors work at this point. Reilly, is there
any coordination to do between Ke He pursuing servicification of generic sensors
and your work to replace device sensors usage with generic sensors usage?

Thanks,

Colin

[ke.he, 2017-04-20 08:32:03]

On 2017/04/19 15:10:40, blundell wrote:
> On 2017/04/19 09:57:18, ke.he wrote:
> > On 2017/04/19 09:15:54, blundell wrote:
> > > I'm slightly confused about what will happen to this test once the device
> > > sensors impl and interface are eliminated. Reilly, is your vision that you
> > will
> > > port it to be a similar test of generic sensors rather than just
eliminating
> > it?
> > > Otherwise, it seems like we could just leave the browsertest and then
> > eliminate
> > > that when the time comes.
> > 
> > Hi, Colin and Reilly, The related work status is:
> > 
> > For //device/sensor, there are 2 tasks ongoing, but almost done.
> > 1) Replace device sensor browsertest by service unittest. --- in reviewing.
> > 2) Move //device/sensors into //services/device/. (it is the last step of
> device
> > sensor servicification)   --- depend on task 1) above, but almost done,
> > See(https://codereview.chromium.org/2819273006/#).
> > 
> > For //device/generic_sensor, similarly 2 tasks in plan:
> > 3) Replace device generic sensor browsertest by service unittest. --- just
> > started.
> > 4) Move //device/generic_sensors into //services/device/.  --- not start.
> > 
> > Should we finished the servification of device sensor first? because it is
> > almost done. And, should I continue the task 3)?
> > 
> > Thanks very much.
> 
> Based on comments #31 and #32 in crbug.com/612322, I would have said that we
> should mark the device sensor servicification bugs as obsolete and replace
them
> with a bug to eliminate usage of device sensors in favor of usage of generic
> sensors (Reilly, does such a bug exist?). However, I'm curious for Reilly's
> response to my question above, as proceeding with this CL doesn't match my
> understanding based on those comments.
> 
> Steps (3) and (4) seem like great work to pursue independent of the resolution
> of how/whether to proceed on device sensors work at this point. Reilly, is
there
> any coordination to do between Ke He pursuing servicification of generic
sensors
> and your work to replace device sensors usage with generic sensors usage?
> 
> Thanks,
> 
> Colin

Colin, Thanks! I'll stop proceeding with this CL.
I see the discussions that the current solution of replacing browsertest is not
good enough, so I'll also temporarily stop steps 3) and 4).