Move Device Sensors client files from Blink to //device/sensors client lib

Move Device Sensors client files from Blink to //device/sensors client lib

Currently, several Device Sensors files are part of the WebKit public
API. These files define POD structs that the Device Sensors implementation
and its clients use to consistently interpret shared memory.

As part of servicificying Device Sensors and enabling it to be used
by clients other than Blink, this CL moves those files into the Device
Sensors client library. It also updates naming conventions etc. to match
their new location.

Once concern in making this move is that Blink's modules implementations
cannot in general depend on Chromium code due to performance concerns. In
this particular case, the dependence is both necessary (for the
interpretation of shared memory) and safe (since the structs are POD and
must remain so). To avoid unwanted dependencies creeping in in the future,
the moved files are isolated in their own target within
//device/sensors/public/cpp, with a clear explanation in the BUILD.gn file
of the constraints on this target.

BUG=612322
TBR=jam

Review-Url: https://codereview.chromium.org/2415083002
Cr-Commit-Position: refs/heads/master@{#458372}
Committed: https://chromium.googlesource.com/chromium/src/+/241fad6fdac74b820fa21e29652bbae49239840e

[blundell, 2016-10-13 14:49:53]

Patchset #2 (id:20001) has been deleted

[blundell, 2016-10-13 14:52:33]

Patchset #1 (id:1) has been deleted

[blundell, 2016-10-13 14:55:51]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 14:56:05]

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-13 14:59:00]

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

[commit-bot: I haz the power, 2016-10-13 14:59:01]

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

[blundell, 2016-10-13 15:27:12]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 15:27:37]

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-13 15:32:17]

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

[commit-bot: I haz the power, 2016-10-13 15:32:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  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_...)

[blundell, 2016-10-13 15:36:10]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 15:36:27]

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-13 17:10:46]

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

[commit-bot: I haz the power, 2016-10-13 17:10:47]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-10-14 07:32:51]

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

[blundell, 2016-10-14 07:32:52]

Hi Tim,

PS#1 shows the diff in the moved files (in the final patchset they show up as
new because the license change moved them too far away from the old versions of
the files).

https://codereview.chromium.org/2415083002/diff/40001/device/sensors/public/c...
File device/sensors/public/cpp/motion_data.h (right):

https://codereview.chromium.org/2415083002/diff/40001/device/sensors/public/c...
device/sensors/public/cpp/motion_data.h:41: MotionData(const MotionData& other);
Had to define the default copy constructor out-of-line because of a
chromium-style clang check that's disabled for Blink (purely for historical "was
Apple code" reasons).

[blundell, 2016-10-21 20:04:08]

ping :)

[timvolodine, 2016-10-24 16:23:43]

two comments/questions below, otherwise looks fine to me.

https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
File device/sensors/public/cpp/motion_data.h (right):

https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
device/sensors/public/cpp/motion_data.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
would it be possible to tweak the '--similarity' parameter in order to have a
proper diff and history for the motion_data* and orientation_data* files?

https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
File device/sensors/public/cpp/orientation_data.h (right):

https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
device/sensors/public/cpp/orientation_data.h:14: OrientationData();
so here we don't need a copy-constructor like in motion_data.h, MotionData(const
MotionData& other) ?

[blundell, 2016-10-24 16:25:59]

https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
File device/sensors/public/cpp/motion_data.h (right):

https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
device/sensors/public/cpp/motion_data.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2016/10/24 16:23:43, timvolodine wrote:
> would it be possible to tweak the '--similarity' parameter in order to have a
> proper diff and history for the motion_data* and orientation_data* files?

Unfortunately not, as they're more similar to other random files than to the
files they're copied from :P.

https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
File device/sensors/public/cpp/orientation_data.h (right):

https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
device/sensors/public/cpp/orientation_data.h:14: OrientationData();
On 2016/10/24 16:23:43, timvolodine wrote:
> so here we don't need a copy-constructor like in motion_data.h,
MotionData(const
> MotionData& other) ?

That's correct. MotionData is copied but OrientationData is not.

[timvolodine, 2016-10-24 18:13:20]

one more comment below, non-blocking so lgtm, thanks!

https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
File device/sensors/public/cpp/orientation_data.h (right):

https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
device/sensors/public/cpp/orientation_data.h:14: OrientationData();
On 2016/10/24 16:25:58, blundell wrote:
> On 2016/10/24 16:23:43, timvolodine wrote:
> > so here we don't need a copy-constructor like in motion_data.h,
> MotionData(const
> > MotionData& other) ?
> 
> That's correct. MotionData is copied but OrientationData is not.

hmm that's weird, not sure where that would happen, i.e. what's different
between MotionData and OrientationData. any ideas?

[blundell, 2016-10-26 15:34:52]

On 2016/10/24 18:13:20, timvolodine wrote:
> one more comment below, non-blocking so lgtm, thanks!
> 
>
https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
> File device/sensors/public/cpp/orientation_data.h (right):
> 
>
https://codereview.chromium.org/2415083002/diff/100001/device/sensors/public/...
> device/sensors/public/cpp/orientation_data.h:14: OrientationData();
> On 2016/10/24 16:25:58, blundell wrote:
> > On 2016/10/24 16:23:43, timvolodine wrote:
> > > so here we don't need a copy-constructor like in motion_data.h,
> > MotionData(const
> > > MotionData& other) ?
> > 
> > That's correct. MotionData is copied but OrientationData is not.
> 
> hmm that's weird, not sure where that would happen, i.e. what's different
> between MotionData and OrientationData. any ideas?

It looks like MotionData is copied for convenience in some tests (I don't recall
the details offhand). Probably with some effort this could be eliminated.

[blundell, 2016-10-26 15:37:18]

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

[blundell, 2016-10-26 15:37:18]

esprehn@: Could you review the removal of files from
//third_party/WebKit/Source/platform/exported?

[haraken, 2016-10-26 15:49:39]

LGTM

[blundell, 2016-10-26 16:00:57]

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

[blundell, 2016-10-26 16:00:58]

Thanks, Kentaro!

TBR=jam for the changes in //content outside of //content/*/device_sensors.
These are just mechanical changes to clients of the classes being moved/renamed.

[blundell, 2016-10-26 16:01:10]

Description was changed from

==========
Move Device Sensors client files from Blink to //device/sensors client lib

Currently, several Device Sensors client files are part of the WebKit public
API. As part of servicificying Device Sensors and enabling it to be used
by clients other than Blink, this CL moves those files into the Device
Sensors client library. It also updates naming conventions etc. to match
their new location.

BUG=612322
==========

to

==========
Move Device Sensors client files from Blink to //device/sensors client lib

Currently, several Device Sensors client files are part of the WebKit public
API. As part of servicificying Device Sensors and enabling it to be used
by clients other than Blink, this CL moves those files into the Device
Sensors client library. It also updates naming conventions etc. to match
their new location.

BUG=612322
TBR=jam
==========

[blundell, 2016-10-26 16:01:23]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2016-10-26 16:01:52]

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

[commit-bot: I haz the power, 2016-10-26 16:01:54]

This CL has an open dependency (Issue 2410123002 Patch 80001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[blundell, 2016-10-26 16:18:17]

The CQ bit was checked by blundell@chromium.org

[blundell, 2016-10-26 16:18:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/2415083002/#ps140001
(title: "Really rebase")

[commit-bot: I haz the power, 2016-10-26 16:18:46]

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

[jam, 2016-10-26 17:11:38]

The CQ bit was unchecked by jam@chromium.org

[jam, 2016-10-26 17:13:41]

https://codereview.chromium.org/2415083002/diff/140001/device/sensors/public/...
File device/sensors/public/cpp/motion_data.h (right):

https://codereview.chromium.org/2415083002/diff/140001/device/sensors/public/...
device/sensors/public/cpp/motion_data.h:18: double accelerationX;
this file should use chromium naming convention, i.e. accelearation_x

https://codereview.chromium.org/2415083002/diff/140001/device/sensors/public/...
File device/sensors/public/cpp/orientation_data.h (right):

https://codereview.chromium.org/2415083002/diff/140001/device/sensors/public/...
device/sensors/public/cpp/orientation_data.h:21: bool hasAlpha : 1;
ditto

[jam, 2016-10-26 17:46:20]

oh and lgtm with these changes

[esprehn, 2016-11-04 23:44:58]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org

[esprehn, 2016-11-04 23:44:59]

This is pulling in headers from device/sensors/public/cpp and not generated mojo
ones, which isn't allowed in core/ or modules/

This should use the generated objects from the mojoms inside blink, not C++
files directly.

[blundell, 2016-11-07 14:29:28]

On 2016/11/04 23:44:59, esprehn wrote:
> This is pulling in headers from device/sensors/public/cpp and not generated
mojo
> ones, which isn't allowed in core/ or modules/
> 
> This should use the generated objects from the mojoms inside blink, not C++
> files directly.

Hi Elliott,

Thanks. Is there documentation somewhere about the dependency rules of core/ and
modules/?

In this case the C++ files are not wrapping mojoms. They're definitions of
structs that the device sensors impl and client share in order to interpret the
shared memory over which they're communicating. The mojo interface is just
passing the handle to that shared memory. Would your recommendation in this case
be to define the struct in Mojo instead of C++?

[blundell, 2016-11-07 14:30:03]

On 2016/11/07 14:29:28, blundell wrote:
> On 2016/11/04 23:44:59, esprehn wrote:
> > This is pulling in headers from device/sensors/public/cpp and not generated
> mojo
> > ones, which isn't allowed in core/ or modules/
> > 
> > This should use the generated objects from the mojoms inside blink, not C++
> > files directly.
> 
> Hi Elliott,
> 
> Thanks. Is there documentation somewhere about the dependency rules of core/
and
> modules/?
> 
> In this case the C++ files are not wrapping mojoms. They're definitions of
> structs that the device sensors impl and client share in order to interpret
the
> shared memory over which they're communicating. The mojo interface is just
> passing the handle to that shared memory. Would your recommendation in this
case
> be to define the struct in Mojo instead of C++?

(Note that I haven't looked into whether there would be any complications that
would arise in trying to do that).

[blundell, 2017-03-16 14:52:45]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 14:53:05]

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

[blundell, 2017-03-16 16:36:24]

Description was changed from

==========
Move Device Sensors client files from Blink to //device/sensors client lib

Currently, several Device Sensors client files are part of the WebKit public
API. As part of servicificying Device Sensors and enabling it to be used
by clients other than Blink, this CL moves those files into the Device
Sensors client library. It also updates naming conventions etc. to match
their new location.

BUG=612322
TBR=jam
==========

to

==========
Move Device Sensors client files from Blink to //device/sensors client lib

Currently, several Device Sensors files are part of the WebKit public
API. These files define POD structs that the Device Sensors implementation
and its clients use to consistently interpret shared memory.

As part of servicificying Device Sensors and enabling it to be used
by clients other than Blink, this CL moves those files into the Device
Sensors client library. It also updates naming conventions etc. to match
their new location.

Once concern in making this move is that Blink's modules implementations
cannot in general depend on Chromium code due to performance concerns. In
this particular case, the dependence is both necessary (for the
interpretation of shared memory) and safe (since the structs are POD and
must remain so). To avoid unwanted dependencies creeping in in the future,
the moved files are isolated in their own target within
//device/sensors/public/cpp, with a clear explanation in the BUILD.gn file
of the constraints on this target.

BUG=612322
TBR=jam
==========

[blundell, 2017-03-16 16:43:18]

Hi,

Elliott, per the discussion on platform-architecture-dev@, I've isolated the
moved files in their own target (see the diff between PS7 and PS8). PTAL,
thanks.

John, would you be OK with me changing the moved structs to Chromium style in an
immediate followup to avoid growing this CL further, or do you really want the
change as part of the move?

Thanks,

Colin

[blundell, 2017-03-16 16:43:25]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 16:44:00]

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-03-16 18:22:58]

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

[commit-bot: I haz the power, 2017-03-16 18:22:59]

Dry run: This issue passed the CQ dry run.

[jam, 2017-03-20 14:44:00]

On 2017/03/16 16:43:18, blundell wrote:
> Hi,
> 
> Elliott, per the discussion on platform-architecture-dev@, I've isolated the
> moved files in their own target (see the diff between PS7 and PS8). PTAL,
> thanks.
> 
> John, would you be OK with me changing the moved structs to Chromium style in
an
> immediate followup to avoid growing this CL further, or do you really want the
> change as part of the move?

sgtm

> 
> Thanks,
> 
> Colin

[blundell, 2017-03-20 15:21:57]

Elliott: ping.

On 2017/03/16 16:43:18, blundell wrote:
> Hi,
> 
> Elliott, per the discussion on platform-architecture-dev@, I've isolated the
> moved files in their own target (see the diff between PS7 and PS8). PTAL,
> thanks.
>

[dglazkov, 2017-03-20 22:44:30]

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

[dglazkov, 2017-03-20 22:44:32]

lgtm

[esprehn, 2017-03-20 23:13:06]

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

[blundell, 2017-03-21 08:17:43]

The CQ bit was checked by blundell@chromium.org

[blundell, 2017-03-21 08:17:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jam@chromium.org, timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/2415083002/#ps180001
(title: "Isolate moved files in their own target")

[commit-bot: I haz the power, 2017-03-21 08:18:02]

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-03-21 08:20:36]

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

[commit-bot: I haz the power, 2017-03-21 08:20:38]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[blundell, 2017-03-21 08:42:53]

The CQ bit was checked by blundell@chromium.org

[blundell, 2017-03-21 08:42:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jam@chromium.org, dglazkov@chromium.org,
timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/2415083002/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-21 08:43:06]

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-03-21 10:18:14]

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

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

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[blundell, 2017-03-21 10:23:28]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2017-03-21 10:23:45]

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-03-21 11:14:42]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490091808926040,
"parent_rev": "3a3ff3e332e4667ca5424370e3d7823aebbd82db", "commit_rev":
"241fad6fdac74b820fa21e29652bbae49239840e"}

[commit-bot: I haz the power, 2017-03-21 11:15:25]

Description was changed from

==========
Move Device Sensors client files from Blink to //device/sensors client lib

Currently, several Device Sensors files are part of the WebKit public
API. These files define POD structs that the Device Sensors implementation
and its clients use to consistently interpret shared memory.

As part of servicificying Device Sensors and enabling it to be used
by clients other than Blink, this CL moves those files into the Device
Sensors client library. It also updates naming conventions etc. to match
their new location.

Once concern in making this move is that Blink's modules implementations
cannot in general depend on Chromium code due to performance concerns. In
this particular case, the dependence is both necessary (for the
interpretation of shared memory) and safe (since the structs are POD and
must remain so). To avoid unwanted dependencies creeping in in the future,
the moved files are isolated in their own target within
//device/sensors/public/cpp, with a clear explanation in the BUILD.gn file
of the constraints on this target.

BUG=612322
TBR=jam
==========

to

==========
Move Device Sensors client files from Blink to //device/sensors client lib

Currently, several Device Sensors files are part of the WebKit public
API. These files define POD structs that the Device Sensors implementation
and its clients use to consistently interpret shared memory.

As part of servicificying Device Sensors and enabling it to be used
by clients other than Blink, this CL moves those files into the Device
Sensors client library. It also updates naming conventions etc. to match
their new location.

Once concern in making this move is that Blink's modules implementations
cannot in general depend on Chromium code due to performance concerns. In
this particular case, the dependence is both necessary (for the
interpretation of shared memory) and safe (since the structs are POD and
must remain so). To avoid unwanted dependencies creeping in in the future,
the moved files are isolated in their own target within
//device/sensors/public/cpp, with a clear explanation in the BUILD.gn file
of the constraints on this target.

BUG=612322
TBR=jam

Review-Url: https://codereview.chromium.org/2415083002
Cr-Commit-Position: refs/heads/master@{#458372}
Committed:
https://chromium.googlesource.com/chromium/src/+/241fad6fdac74b820fa21e29652b...
==========

[commit-bot: I haz the power, 2017-03-21 11:15:27]

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