Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415083002:
Move Device Sensors client files from Blink to //device/sensors client lib (Closed)
|
Created:
4 years, 2 months ago by blundell Modified:
3 years, 9 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, blundell+watchlist_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, droger+watchlist_chromium.org, einbinder+watch-test-runner_chromium.org, jochen+watch_chromium.org, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-sensors_chromium.org, mlamouri+watch-test-runner_chromium.org, Peter Beverloo, riju_, sdefresne+watchlist_chromium.org, timvolodine Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove 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
Patch Set 1 : Revert licenses change for similarity #
Total comments: 1
Patch Set 2 : update licenses/header guards #Patch Set 3 : Rebase #Patch Set 4 : Fix gn check #
Total comments: 5
Patch Set 5 : Rebase #Patch Set 6 : Really rebase #
Total comments: 2
Patch Set 7 : Rebase #Patch Set 8 : Isolate moved files in their own target #Patch Set 9 : Rebase #Messages
Total messages: 68 (42 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
blundell@chromium.org changed reviewers: + timvolodine@chromium.org
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).
ping :)
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) ?
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.
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?
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@chromium.org changed reviewers: + esprehn@chromium.org
esprehn@: Could you review the removal of files from //third_party/WebKit/Source/platform/exported?
LGTM
blundell@chromium.org changed reviewers: + jam@chromium.org
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.
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 ==========
The CQ bit was checked by blundell@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
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.
The CQ bit was checked by blundell@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jam@chromium.org
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
oh and lgtm with these changes
esprehn@chromium.org changed reviewers: + haraken@chromium.org
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.
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++?
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).
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
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
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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
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@chromium.org changed reviewers: + dglazkov@chromium.org
lgtm
esprehn@chromium.org changed reviewers: - esprehn@chromium.org
The CQ bit was checked by blundell@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by blundell@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by blundell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490091808926040,
"parent_rev": "3a3ff3e332e4667ca5424370e3d7823aebbd82db", "commit_rev":
"241fad6fdac74b820fa21e29652bbae49239840e"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/241fad6fdac74b820fa21e29652b... |
