Move //device/generic_sensor to be part of the internal implementation of the Device Service.

Remove the *.export.h, change the gn target 'component' to 'source_set', which makes the generic sensor to be the internal implementation of //services.

BUG=689384
Review-Url: https://codereview.chromium.org/2865263002
Cr-Commit-Position: refs/heads/master@{#473160}
Committed: https://chromium.googlesource.com/chromium/src/+/9b5cf7f93a4b2f246d08208ccd17804856c38f6a

[ke.he, 2017-05-10 07:24:38]

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

[commit-bot: I haz the power, 2017-05-10 07:25: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, 2017-05-10 08:13:57]

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

[commit-bot: I haz the power, 2017-05-10 08:13:58]

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-05-10 08:28:24]

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

[commit-bot: I haz the power, 2017-05-10 08:29:07]

Dry run: CQ is trying da patch.

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

[ke.he, 2017-05-10 09:00:09]

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

[commit-bot: I haz the power, 2017-05-10 09:00:42]

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-05-10 10:13:36]

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

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

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

[ke.he, 2017-05-11 02:37:01]

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

[commit-bot: I haz the power, 2017-05-11 02:38:03]

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-05-11 04:03:45]

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

[commit-bot: I haz the power, 2017-05-11 04:03:46]

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

[ke.he, 2017-05-11 09:30:42]

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

[commit-bot: I haz the power, 2017-05-11 09:31:19]

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-05-11 09:42:53]

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

[commit-bot: I haz the power, 2017-05-11 09:42:55]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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-05-16 06:23:31]

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

[commit-bot: I haz the power, 2017-05-16 06:23:42]

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-05-16 07:00:14]

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

[commit-bot: I haz the power, 2017-05-16 07:00:15]

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

[ke.he, 2017-05-16 08:30:48]

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

[commit-bot: I haz the power, 2017-05-16 08:31:03]

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-05-16 10:18:01]

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

[commit-bot: I haz the power, 2017-05-16 10:18:03]

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-05-17 05:20:15]

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

[commit-bot: I haz the power, 2017-05-17 05:21:06]

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-05-17 05:26:22]

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

[commit-bot: I haz the power, 2017-05-17 05:26:24]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-05-17 07:29:50]

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

[commit-bot: I haz the power, 2017-05-17 07:30:27]

Dry run: CQ is trying da patch.

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

[ke.he, 2017-05-17 09:08:18]

Description was changed from

==========
Move //device/generic_sensor to be part of the internal implementation of the
Device Service.

BUG=689384
==========

to

==========
Remove the *.export.h, change the gn target 'component' to 'source_set', which
makes the generic sensor to be the internal implementation of //services.

BUG=689384
==========

[ke.he, 2017-05-17 09:32:03]

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

[ke.he, 2017-05-17 09:32:06]

Hi, Colin, many files were touched but the real modification is not so huge:)
PTAL. Thanks!

https://codereview.chromium.org/2865263002/diff/160001/services/device/public...
File services/device/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2865263002/diff/160001/services/device/public...
services/device/public/interfaces/BUILD.gn:25: mojom("generic_sensor") {
The reason I didn't merge "generic_sensor" target into "interfaces" because
there is cycle dependency between its mojom files and cpp library files.

generic_sensor's mojom depends on their traits file "sensor_struct_traits.cc";
The "sensor_struct_traits.cc" includes its cpp library file
"platform_sensor_configuration.h";
The "platform_sensor_configuraton.cpp" includes "sensor.mojom.h". It is a cycle.
So If the target that depends on generic_sensor:interface must also depends on
generic_sensor's cpp library if it wants to do link.

In //service/device/fingerprints/BUILD.gn, there is target
component("fingerprint") deps on //service/device/public/interfaces, it doesn't
depend on generic_sensor's cpp library.

[commit-bot: I haz the power, 2017-05-17 11:38:38]

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

[commit-bot: I haz the power, 2017-05-17 11:38:39]

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

[blundell, 2017-05-17 15:26:20]

blundell@chromium.org changed reviewers:
+ rockot@chromium.org

[blundell, 2017-05-17 15:26:21]

blundell@ -> rockot@

I'm about to go OOO for the rest of the week, sending over to Ken. Thanks!

[Ken Rockot(use gerrit already), 2017-05-17 17:40:08]

lgtm

[ke.he, 2017-05-18 05:16:14]

ke.he@intel.com changed reviewers:
+ alexander.shalamov@intel.com, jam@chromium.org, tsepez@chromium.org

[ke.he, 2017-05-18 05:16:15]

Hi, 
Jam@, Could you PTAL on //content/ and
	mojo/public/tools/bindings/chromium_bindings_configuration.gni.

Alexander, Could you PTAL on the Webkit.

Tom, Could you PTAL on the *.mojom, *.typemap and
//services/device/public/cpp/generic_sensor/

Thanks very much to all of you!

[ke.he, 2017-05-18 05:24:18]

https://codereview.chromium.org/2865263002/diff/160001/services/device/public...
File services/device/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2865263002/diff/160001/services/device/public...
services/device/public/interfaces/BUILD.gn:25: mojom("generic_sensor") {
On 2017/05/17 09:32:05, ke.he wrote:
> The reason I didn't merge "generic_sensor" target into "interfaces" because
> there is cycle dependency between its mojom files and cpp library files.
> 
> generic_sensor's mojom depends on their traits file "sensor_struct_traits.cc";
> The "sensor_struct_traits.cc" includes its cpp library file
> "platform_sensor_configuration.h";
> The "platform_sensor_configuraton.cpp" includes "sensor.mojom.h". It is a
cycle.
> So If the target that depends on generic_sensor:interface must also depends on
> generic_sensor's cpp library if it wants to do link.
> 
> In //service/device/fingerprints/BUILD.gn, there is target
> component("fingerprint") deps on //service/device/public/interfaces, it
doesn't
> depend on generic_sensor's cpp library. 

Correction:
The GN target that depends on ".../cpp/generic_sensor:interface" must also
depend on
generic_sensor's cpp library if it wants to link to generate a *.so.

[ke.he, 2017-05-18 10:02:33]

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

[ke.he, 2017-05-18 10:02:34]

And reillyg@ who is planing to merge /sensor and /generic_sensor.

[jam, 2017-05-18 14:23:39]

lgtm

[Tom Sepez, 2017-05-18 15:58:31]

lgtm

RS on renaming files.  If there's another change buried in here, please let me
know where to look.

[Reilly Grant (use Gerrit), 2017-05-18 15:58:50]

lgtm

https://codereview.chromium.org/2865263002/diff/160001/services/device/public...
File services/device/public/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2865263002/diff/160001/services/device/public...
services/device/public/interfaces/BUILD.gn:25: mojom("generic_sensor") {
On 2017/05/17 09:32:05, ke.he wrote:
> The reason I didn't merge "generic_sensor" target into "interfaces" because
> there is cycle dependency between its mojom files and cpp library files.
> 
> generic_sensor's mojom depends on their traits file "sensor_struct_traits.cc";
> The "sensor_struct_traits.cc" includes its cpp library file
> "platform_sensor_configuration.h";
> The "platform_sensor_configuraton.cpp" includes "sensor.mojom.h". It is a
cycle.
> So If the target that depends on generic_sensor:interface must also depends on
> generic_sensor's cpp library if it wants to do link.
> 
> In //service/device/fingerprints/BUILD.gn, there is target
> component("fingerprint") deps on //service/device/public/interfaces, it
doesn't
> depend on generic_sensor's cpp library. 

I resolved a similar cycle in //device/usb/BUILD.gn by depending on header
generation instead of binary compilation:

https://cs.chromium.org/chromium/src/device/usb/BUILD.gn?q=usb/BUILD.gn&l=70

[ke.he, 2017-05-18 16:23:10]

Tom, basically just file moving and renaming. While there is also a change that
I removed the "dllexport" attributes from the mojom classes. Because I think
they should be internal part of Device Service.

Reilly, thanks, I'll have a study on your CL. How about I leave it(resolve the
dependency cycle) as a follow up, and land it in next CL? I don't want to make
this CL too complex:)

[Reilly Grant (use Gerrit), 2017-05-18 16:55:24]

On 2017/05/18 16:23:10, ke.he wrote:
> Reilly, thanks, I'll have a study on your CL. How about I leave it(resolve the
> dependency cycle) as a follow up, and land it in next CL? I don't want to make
> this CL too complex:)

That's fine.

[ke.he, 2017-05-19 05:29:30]

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

[commit-bot: I haz the power, 2017-05-19 05:29:52]

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-05-19 07:27:21]

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

[commit-bot: I haz the power, 2017-05-19 07:27:22]

Dry run: This issue passed the CQ dry run.

[shalamov, 2017-05-19 07:40:51]

lgtm

[ke.he, 2017-05-19 07:41:49]

The CQ bit was checked by ke.he@intel.com

[ke.he, 2017-05-19 07:41:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
reillyg@chromium.org, jam@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2865263002/#ps180001
(title: "code rebase")

[commit-bot: I haz the power, 2017-05-19 07:42:34]

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-05-19 07:55:05]

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

[commit-bot: I haz the power, 2017-05-19 07:55:08]

Failed to apply patch for content/test/BUILD.gn:
While running git apply --index -3 -p1;
  error: patch failed: content/test/BUILD.gn:778
  Falling back to three-way merge...
  Applied patch to 'content/test/BUILD.gn' with conflicts.
  U content/test/BUILD.gn

Patch:       content/test/BUILD.gn
Index: content/test/BUILD.gn
diff --git a/content/test/BUILD.gn b/content/test/BUILD.gn
index
00ec80541d2bb4d59a1c8bc6a059d671fa6518ee..e682d347f98e1ac2c25039a3e265a676d7831c35
100644
--- a/content/test/BUILD.gn
+++ b/content/test/BUILD.gn
@@ -757,7 +757,6 @@ test("content_browsertests") {
     "//content/shell:pak",
     "//content/test:test_support",
     "//device/base/synchronization",
-    "//device/generic_sensor",
     "//device/power_save_blocker",
     "//device/screen_orientation/public/interfaces",
     "//device/sensors",
@@ -778,6 +777,7 @@ test("content_browsertests") {
     "//net:test_support",
     "//ppapi/features",
     "//services/catalog:lib",
+    "//services/device/generic_sensor",
     "//services/service_manager/public/cpp",
     "//services/ui/gpu/interfaces",
     "//services/ui/public/cpp/gpu",

[ke.he, 2017-05-19 08:00:54]

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

[commit-bot: I haz the power, 2017-05-19 08:01:29]

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-05-19 10:24:32]

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

[commit-bot: I haz the power, 2017-05-19 10:24:35]

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-05-19 11:25:52]

The CQ bit was checked by ke.he@intel.com

[ke.he, 2017-05-19 11:25:53]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
reillyg@chromium.org, alexander.shalamov@intel.com, jam@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2865263002/#ps200001
(title: "code rebase")

[commit-bot: I haz the power, 2017-05-19 11:26:10]

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-05-19 11:34:45]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1495193152018700,
"parent_rev": "c3a8b7831c0598f303b34495a7396f06d4760d8e", "commit_rev":
"9b5cf7f93a4b2f246d08208ccd17804856c38f6a"}

[commit-bot: I haz the power, 2017-05-19 11:34:57]

Description was changed from

==========
Remove the *.export.h, change the gn target 'component' to 'source_set', which
makes the generic sensor to be the internal implementation of //services.

BUG=689384
==========

to

==========
Remove the *.export.h, change the gn target 'component' to 'source_set', which
makes the generic sensor to be the internal implementation of //services.

BUG=689384

Review-Url: https://codereview.chromium.org/2865263002
Cr-Commit-Position: refs/heads/master@{#473160}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b5cf7f93a4b2f246d08208ccd17...
==========

[commit-bot: I haz the power, 2017-05-19 11:35:00]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/9b5cf7f93a4b2f246d08208ccd17...

[Marc Treib, 2017-05-19 13:07:28]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2898433002/ by treib@chromium.org.

The reason for reverting is: Seems to have broken
GenericSensorBrowserTest.AmbientLightSensorTest:

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20(dbg)(1)/bu...
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20(dbg)/bu...
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....

[findit-for-me, 2017-05-19 14:17:09]

Findit (https://goo.gl/kROfz5) identified this CL at revision 473160 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[blundell, 2017-05-22 09:39:05]

On 2017/05/19 14:17:09, findit-for-me wrote:
> Findit (https://goo.gl/kROfz5) identified this CL at revision 473160 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

My guess is that this is a component build-specific issue. Ke He, can you use
the component build locally and see if you can repro the failure?

[ke.he, 2017-05-22 10:36:58]

Hi, Colin,
My args.gn is: is_component_build=true, is_debug=false, dcheck_always_on=true.
In the original //device/generic_sensor/BUILD.gn, it is
|component("generic_sensor")|,  I change it into |source_set("generic_sensor")|
and remove the "define = [DEVICE_GENERIC_SENSOR_IMPLEMENTATION]",  then the
browsertest failure reproduce. 
I'm looking on this issue now.
________________________________________
From: blundell@chromium.org [blundell@chromium.org]
Sent: Monday, May 22, 2017 5:39 PM
To: He, Ke; rockot@chromium.org; jam@chromium.org; Shalamov, Alexander;
tsepez@chromium.org; reillyg@chromium.org
Cc: aa@chromium.org; abarth@chromium.org; agrieve+watch@chromium.org; Shalamov,
Alexander; blink-reviews@chromium.org; blink-reviews-bindings@chromium.org;
chromium-reviews@chromium.org; darin@chromium.org; darin-cc@chromium.org;
haraken@chromium.org; jam@chromium.org; mac-reviews@chromium.org; Pozdnyakov,
Mikhail; mlamouri+watch-sensors@chromium.org; qsr+mojo@chromium.org; Bhaumik,
Rijubrata; timvolodine@chromium.org; viettrungluu+watch@chromium.org; Lin,
Wanming; yzshen+watch@chromium.org
Subject: Re: Move //device/generic_sensor to be part of the internal
implementation of the Device Service. (issue 2865263002 by ke.he@intel.com)

On 2017/05/19 14:17:09, findit-for-me wrote:
> Findit (https://goo.gl/kROfz5) identified this CL at revision 473160 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

My guess is that this is a component build-specific issue. Ke He, can you use
the component build locally and see if you can repro the failure?

https://codereview.chromium.org/2865263002/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[ke.he, 2017-05-22 10:36:58]

Hi, Colin,
My args.gn is: is_component_build=true, is_debug=false, dcheck_always_on=true.
In the original //device/generic_sensor/BUILD.gn, it is
|component("generic_sensor")|,  I change it into |source_set("generic_sensor")|
and remove the "define = [DEVICE_GENERIC_SENSOR_IMPLEMENTATION]",  then the
browsertest failure reproduce. 
I'm looking on this issue now.
________________________________________
From: blundell@chromium.org [blundell@chromium.org]
Sent: Monday, May 22, 2017 5:39 PM
To: He, Ke; rockot@chromium.org; jam@chromium.org; Shalamov, Alexander;
tsepez@chromium.org; reillyg@chromium.org
Cc: aa@chromium.org; abarth@chromium.org; agrieve+watch@chromium.org; Shalamov,
Alexander; blink-reviews@chromium.org; blink-reviews-bindings@chromium.org;
chromium-reviews@chromium.org; darin@chromium.org; darin-cc@chromium.org;
haraken@chromium.org; jam@chromium.org; mac-reviews@chromium.org; Pozdnyakov,
Mikhail; mlamouri+watch-sensors@chromium.org; qsr+mojo@chromium.org; Bhaumik,
Rijubrata; timvolodine@chromium.org; viettrungluu+watch@chromium.org; Lin,
Wanming; yzshen+watch@chromium.org
Subject: Re: Move //device/generic_sensor to be part of the internal
implementation of the Device Service. (issue 2865263002 by ke.he@intel.com)

On 2017/05/19 14:17:09, findit-for-me wrote:
> Findit (https://goo.gl/kROfz5) identified this CL at revision 473160 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

My guess is that this is a component build-specific issue. Ke He, can you use
the component build locally and see if you can repro the failure?

https://codereview.chromium.org/2865263002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ke.he, 2017-05-23 08:44:10]

Hi, Colin,
The root cause is: There is a global var: |g_provider_for_testing| in
"platform_sensor_provider.cc". If we change the "componnent" to "source_set",
all dependents don't share the same |g_provider_for_testing| anymore.

I plan to pending this CL for a while, and to 1)rewrite the browsertest in
//services by the new infra. 2)change the current unittest to service_unittest.
After above 2 steps, I can remove the current browsertest as well as the global
|g_provider_for_testing|. Then to land this CL at last step.

[blundell, 2017-05-23 08:51:00]

On 2017/05/23 08:44:10, ke.he wrote:
> Hi, Colin,
> The root cause is: There is a global var: |g_provider_for_testing| in
> "platform_sensor_provider.cc". If we change the "componnent" to "source_set",
> all dependents don't share the same |g_provider_for_testing| anymore.
> 
> I plan to pending this CL for a while, and to 1)rewrite the browsertest in
> //services by the new infra. 2)change the current unittest to
service_unittest.
> After above 2 steps, I can remove the current browsertest as well as the
global
> |g_provider_for_testing|. Then to land this CL at last step.

Ugh :(. Good work in tracking this down!

We can potentially leave the browsertest in place and just inject a mock impl
via Han Leon's infra, right? I don't know exactly what the browsertest is
testing offhand to know how reasonable an alteration this would be. Maybe this
is what you mean by (1) above?

[ke.he, 2017-05-23 09:10:48]

On 2017/05/23 08:51:00, blundell wrote:
> On 2017/05/23 08:44:10, ke.he wrote:
> > Hi, Colin,
> > The root cause is: There is a global var: |g_provider_for_testing| in
> > "platform_sensor_provider.cc". If we change the "componnent" to
"source_set",
> > all dependents don't share the same |g_provider_for_testing| anymore.
> > 
> > I plan to pending this CL for a while, and to 1)rewrite the browsertest in
> > //services by the new infra. 2)change the current unittest to
> service_unittest.
> > After above 2 steps, I can remove the current browsertest as well as the
> global
> > |g_provider_for_testing|. Then to land this CL at last step.
> 
> Ugh :(. Good work in tracking this down!
> 
> We can potentially leave the browsertest in place and just inject a mock impl
> via Han Leon's infra, right? I don't know exactly what the browsertest is
> testing offhand to know how reasonable an alteration this would be. Maybe this
> is what you mean by (1) above?

Yes, just discussed with Leon about his new landed infra:)

[ke.he, 2017-05-23 09:15:09]

On 2017/05/23 09:10:48, ke.he wrote:
> On 2017/05/23 08:51:00, blundell wrote:
> > On 2017/05/23 08:44:10, ke.he wrote:
> > > Hi, Colin,
> > > The root cause is: There is a global var: |g_provider_for_testing| in
> > > "platform_sensor_provider.cc". If we change the "componnent" to
> "source_set",
> > > all dependents don't share the same |g_provider_for_testing| anymore.
> > > 
> > > I plan to pending this CL for a while, and to 1)rewrite the browsertest in
> > > //services by the new infra. 2)change the current unittest to
> > service_unittest.
> > > After above 2 steps, I can remove the current browsertest as well as the
> > global
> > > |g_provider_for_testing|. Then to land this CL at last step.
> > 
> > Ugh :(. Good work in tracking this down!
> > 
> > We can potentially leave the browsertest in place and just inject a mock
impl
> > via Han Leon's infra, right? I don't know exactly what the browsertest is
> > testing offhand to know how reasonable an alteration this would be. Maybe
this
> > is what you mean by (1) above?
> 
> Yes, just discussed with Leon about his new landed infra:)

Yes, I'll create a new browsertest via leon's new infra and keep the original
browsertest. I'll finish 2 steps above first, after that then it is the right
time to remove the original browsertest.

[blundell, 2017-05-23 09:19:29]

On 2017/05/23 09:15:09, ke.he wrote:
> On 2017/05/23 09:10:48, ke.he wrote:
> > On 2017/05/23 08:51:00, blundell wrote:
> > > On 2017/05/23 08:44:10, ke.he wrote:
> > > > Hi, Colin,
> > > > The root cause is: There is a global var: |g_provider_for_testing| in
> > > > "platform_sensor_provider.cc". If we change the "componnent" to
> > "source_set",
> > > > all dependents don't share the same |g_provider_for_testing| anymore.
> > > > 
> > > > I plan to pending this CL for a while, and to 1)rewrite the browsertest
in
> > > > //services by the new infra. 2)change the current unittest to
> > > service_unittest.
> > > > After above 2 steps, I can remove the current browsertest as well as the
> > > global
> > > > |g_provider_for_testing|. Then to land this CL at last step.
> > > 
> > > Ugh :(. Good work in tracking this down!
> > > 
> > > We can potentially leave the browsertest in place and just inject a mock
> impl
> > > via Han Leon's infra, right? I don't know exactly what the browsertest is
> > > testing offhand to know how reasonable an alteration this would be. Maybe
> this
> > > is what you mean by (1) above?
> > 
> > Yes, just discussed with Leon about his new landed infra:)
> 
> Yes, I'll create a new browsertest via leon's new infra and keep the original
> browsertest. I'll finish 2 steps above first, after that then it is the right
> time to remove the original browsertest.

Could you just port the original browsertest in-place to use the new infra?

[ke.he, 2017-05-23 10:27:48]

On 2017/05/23 09:19:29, blundell wrote:
> On 2017/05/23 09:15:09, ke.he wrote:
> > On 2017/05/23 09:10:48, ke.he wrote:
> > > On 2017/05/23 08:51:00, blundell wrote:
> > > > On 2017/05/23 08:44:10, ke.he wrote:
> > > > > Hi, Colin,
> > > > > The root cause is: There is a global var: |g_provider_for_testing| in
> > > > > "platform_sensor_provider.cc". If we change the "componnent" to
> > > "source_set",
> > > > > all dependents don't share the same |g_provider_for_testing| anymore.
> > > > > 
> > > > > I plan to pending this CL for a while, and to 1)rewrite the
browsertest
> in
> > > > > //services by the new infra. 2)change the current unittest to
> > > > service_unittest.
> > > > > After above 2 steps, I can remove the current browsertest as well as
the
> > > > global
> > > > > |g_provider_for_testing|. Then to land this CL at last step.
> > > > 
> > > > Ugh :(. Good work in tracking this down!
> > > > 
> > > > We can potentially leave the browsertest in place and just inject a mock
> > impl
> > > > via Han Leon's infra, right? I don't know exactly what the browsertest
is
> > > > testing offhand to know how reasonable an alteration this would be.
Maybe
> > this
> > > > is what you mean by (1) above?
> > > 
> > > Yes, just discussed with Leon about his new landed infra:)
> > 
> > Yes, I'll create a new browsertest via leon's new infra and keep the
original
> > browsertest. I'll finish 2 steps above first, after that then it is the
right
> > time to remove the original browsertest.
> 
> Could you just port the original browsertest in-place to use the new infra?

I'm afraid maybe we can't replace the original browsertest by the new-infra
browsertest in one step. So far my understanding is:

The new-infra browsertest would be very simple, it won't mock the internal
implementations like current browsertest does, in this way we can get rid of
global |g_provider_for_testing| in browsertest. We can't reference the global
|g_provider_for_testing| in browsertest because it blocks us to move
//device/generic_sensor into //services/device. So the new-infra browsertest has
less coverage than original browsertest, the reduced coverage part will be added
into service_unittests. That's why I plan to remove the original browsertest
only after both new-infra browsertest and service_unittest are ready.

By the way, it is safe for the service_unittest to use the global
|g_provider_for_testing| because it links the |g_provider_for_testing| only
once.

Not sure everything is correct so far, some details are not clear, still
ongoing:)

[blundell, 2017-05-24 13:01:39]

On 2017/05/23 10:27:48, ke.he wrote:
> On 2017/05/23 09:19:29, blundell wrote:
> > On 2017/05/23 09:15:09, ke.he wrote:
> > > On 2017/05/23 09:10:48, ke.he wrote:
> > > > On 2017/05/23 08:51:00, blundell wrote:
> > > > > On 2017/05/23 08:44:10, ke.he wrote:
> > > > > > Hi, Colin,
> > > > > > The root cause is: There is a global var: |g_provider_for_testing|
in
> > > > > > "platform_sensor_provider.cc". If we change the "componnent" to
> > > > "source_set",
> > > > > > all dependents don't share the same |g_provider_for_testing|
anymore.
> > > > > > 
> > > > > > I plan to pending this CL for a while, and to 1)rewrite the
> browsertest
> > in
> > > > > > //services by the new infra. 2)change the current unittest to
> > > > > service_unittest.
> > > > > > After above 2 steps, I can remove the current browsertest as well as
> the
> > > > > global
> > > > > > |g_provider_for_testing|. Then to land this CL at last step.
> > > > > 
> > > > > Ugh :(. Good work in tracking this down!
> > > > > 
> > > > > We can potentially leave the browsertest in place and just inject a
mock
> > > impl
> > > > > via Han Leon's infra, right? I don't know exactly what the browsertest
> is
> > > > > testing offhand to know how reasonable an alteration this would be.
> Maybe
> > > this
> > > > > is what you mean by (1) above?
> > > > 
> > > > Yes, just discussed with Leon about his new landed infra:)
> > > 
> > > Yes, I'll create a new browsertest via leon's new infra and keep the
> original
> > > browsertest. I'll finish 2 steps above first, after that then it is the
> right
> > > time to remove the original browsertest.
> > 
> > Could you just port the original browsertest in-place to use the new infra?
> 
> I'm afraid maybe we can't replace the original browsertest by the new-infra
> browsertest in one step. So far my understanding is:
> 
> The new-infra browsertest would be very simple, it won't mock the internal
> implementations like current browsertest does, in this way we can get rid of
> global |g_provider_for_testing| in browsertest. We can't reference the global
> |g_provider_for_testing| in browsertest because it blocks us to move
> //device/generic_sensor into //services/device. So the new-infra browsertest
has
> less coverage than original browsertest, the reduced coverage part will be
added
> into service_unittests. That's why I plan to remove the original browsertest
> only after both new-infra browsertest and service_unittest are ready.
> 
> By the way, it is safe for the service_unittest to use the global
> |g_provider_for_testing| because it links the |g_provider_for_testing| only
> once.
> 
> Not sure everything is correct so far, some details are not clear, still
> ongoing:)

AFAICT, the generic sensor browsertest is essentially injecting a completely
fake implementation (the fake sensor provider creates fake sensor impls). Would
it not be possible to just extend that fake implementation to directly implement
the Mojo interfaces and then inject that as the generic sensors impl using Han
Leon's infrastructure?

Adding service tests is a good idea as well in any case, of course :).

[ke.he, 2017-05-26 08:00:23]

On 2017/05/24 13:01:39, blundell (OOO until May 29) wrote:
> On 2017/05/23 10:27:48, ke.he wrote:
> > On 2017/05/23 09:19:29, blundell wrote:
> > > On 2017/05/23 09:15:09, ke.he wrote:
> > > > On 2017/05/23 09:10:48, ke.he wrote:
> > > > > On 2017/05/23 08:51:00, blundell wrote:
> > > > > > On 2017/05/23 08:44:10, ke.he wrote:
> > > > > > > Hi, Colin,
> > > > > > > The root cause is: There is a global var: |g_provider_for_testing|
> in
> > > > > > > "platform_sensor_provider.cc". If we change the "componnent" to
> > > > > "source_set",
> > > > > > > all dependents don't share the same |g_provider_for_testing|
> anymore.
> > > > > > > 
> > > > > > > I plan to pending this CL for a while, and to 1)rewrite the
> > browsertest
> > > in
> > > > > > > //services by the new infra. 2)change the current unittest to
> > > > > > service_unittest.
> > > > > > > After above 2 steps, I can remove the current browsertest as well
as
> > the
> > > > > > global
> > > > > > > |g_provider_for_testing|. Then to land this CL at last step.
> > > > > > 
> > > > > > Ugh :(. Good work in tracking this down!
> > > > > > 
> > > > > > We can potentially leave the browsertest in place and just inject a
> mock
> > > > impl
> > > > > > via Han Leon's infra, right? I don't know exactly what the
browsertest
> > is
> > > > > > testing offhand to know how reasonable an alteration this would be.
> > Maybe
> > > > this
> > > > > > is what you mean by (1) above?
> > > > > 
> > > > > Yes, just discussed with Leon about his new landed infra:)
> > > > 
> > > > Yes, I'll create a new browsertest via leon's new infra and keep the
> > original
> > > > browsertest. I'll finish 2 steps above first, after that then it is the
> > right
> > > > time to remove the original browsertest.
> > > 
> > > Could you just port the original browsertest in-place to use the new
infra?
> > 
> > I'm afraid maybe we can't replace the original browsertest by the new-infra
> > browsertest in one step. So far my understanding is:
> > 
> > The new-infra browsertest would be very simple, it won't mock the internal
> > implementations like current browsertest does, in this way we can get rid of
> > global |g_provider_for_testing| in browsertest. We can't reference the
global
> > |g_provider_for_testing| in browsertest because it blocks us to move
> > //device/generic_sensor into //services/device. So the new-infra browsertest
> has
> > less coverage than original browsertest, the reduced coverage part will be
> added
> > into service_unittests. That's why I plan to remove the original browsertest
> > only after both new-infra browsertest and service_unittest are ready.
> > 
> > By the way, it is safe for the service_unittest to use the global
> > |g_provider_for_testing| because it links the |g_provider_for_testing| only
> > once.
> > 
> > Not sure everything is correct so far, some details are not clear, still
> > ongoing:)
> 
> AFAICT, the generic sensor browsertest is essentially injecting a completely
> fake implementation (the fake sensor provider creates fake sensor impls).
Would
> it not be possible to just extend that fake implementation to directly
implement
> the Mojo interfaces and then inject that as the generic sensors impl using Han
> Leon's infrastructure?
> 
> Adding service tests is a good idea as well in any case, of course :).

Yes, it is possible and in my plan. My point is that the to-be-added browsertest
in the new-infra cannot replace the original browsertest, because all the
browser side code are fake. That's why I plan to add service_unittest first, to
test the real production code in browser side by calling the mojo interfaces:)