Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2772183002:
[DeviceService] Cleanup of android library dependencies. (Closed)
|
Created:
3 years, 9 months ago by leonhsl(Using Gerrit) Modified:
3 years, 9 months ago Reviewers:
blundell CC:
chromium-reviews, mlamouri+watch-screen-orientation_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[DeviceService] Cleanup of android library dependencies.
This cleanup CL moves java files of screen_orientaion and
time_zone_monitor to their own android library targets, rather than
mixing all of them into the root target //services/device:java,
This CL also restricts //services/device:{java, lib} to be visible
only to embedders of Device Service.
BUG=none
Review-Url: https://codereview.chromium.org/2772183002
Cr-Commit-Position: refs/heads/master@{#459757}
Committed: https://chromium.googlesource.com/chromium/src/+/fa99300307c0b45de83ff66cd5f938143e8bbc9a
Patch Set 1 #
Total comments: 4
Patch Set 2 : content/public/android:content_java #Patch Set 3 : content/public/android:* #
Total comments: 6
Patch Set 4 : Add comments for changes #
Messages
Total messages: 29 (20 generated)
The CQ bit was checked by leon.han@intel.com 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.
leon.han@intel.com changed reviewers: + blundell@chromium.org
Hi, Colin, I think maybe this way would be clearer? PTAL, Thanks.
Good idea. LGTM. https://codereview.chromium.org/2772183002/diff/1/services/device/BUILD.gn File services/device/BUILD.gn (right): https://codereview.chromium.org/2772183002/diff/1/services/device/BUILD.gn#ne... services/device/BUILD.gn:87: android_library("java") { can we restrict the visibility here to just the corresponding java target of the //content embedder? We could do a similiar thing with //services/device:lib actually. https://codereview.chromium.org/2772183002/diff/1/services/device/screen_orie... File services/device/screen_orientation/BUILD.gn (right): https://codereview.chromium.org/2772183002/diff/1/services/device/screen_orie... services/device/screen_orientation/BUILD.gn:43: visibility = [ "//services/device:*" ] maybe just //services/device:java for clarity?
The CQ bit was checked by leon.han@intel.com 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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) 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 leon.han@intel.com 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...
Uploaded ps#3, PTAnL, Thanks! https://codereview.chromium.org/2772183002/diff/1/services/device/BUILD.gn File services/device/BUILD.gn (right): https://codereview.chromium.org/2772183002/diff/1/services/device/BUILD.gn#ne... services/device/BUILD.gn:87: android_library("java") { On 2017/03/27 07:43:42, blundell wrote: > can we restrict the visibility here to just the corresponding java target of the > //content embedder? We could do a similiar thing with //services/device:lib > actually. Done. https://codereview.chromium.org/2772183002/diff/1/services/device/screen_orie... File services/device/screen_orientation/BUILD.gn (right): https://codereview.chromium.org/2772183002/diff/1/services/device/screen_orie... services/device/screen_orientation/BUILD.gn:43: visibility = [ "//services/device:*" ] On 2017/03/27 07:43:43, blundell wrote: > maybe just //services/device:java for clarity? Yeah I tried with //services/device:java before, but trybots complain that some //services/device:java_XXX targets also need to depend on //services/device/screen_orientation:java, and seems in GN we can't write like //services/device:java_*, so I have to use //services/device:* here. We can see ps#2 fails for such reason and ps#3 is OK.
still lgtm, thanks! The Device Service is shaping up really well. https://codereview.chromium.org/2772183002/diff/40001/services/device/BUILD.gn File services/device/BUILD.gn (right): https://codereview.chromium.org/2772183002/diff/40001/services/device/BUILD.g... services/device/BUILD.gn:14: visibility = [ "//content/browser" ] Nit: Add a comment here saying that this should be made visible only to embedders of the Device Service, and that the dependence should only be for the purpose of embedding the Device Service. https://codereview.chromium.org/2772183002/diff/40001/services/device/BUILD.g... services/device/BUILD.gn:89: visibility = [ "//content/public/android:*" ] similar nit to above. https://codereview.chromium.org/2772183002/diff/40001/services/device/screen_... File services/device/screen_orientation/BUILD.gn (right): https://codereview.chromium.org/2772183002/diff/40001/services/device/screen_... services/device/screen_orientation/BUILD.gn:43: visibility = [ "//services/device:*" ] nit: Add a comment like: "Conceptually, this should be visible only to //services/device:java. However, various generated targets also need to see this target as a result of //services/device:java depending on it."
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Uploaded ps#4 to add more comments to the changes, will send to CQ now, Thank you ;-) https://codereview.chromium.org/2772183002/diff/40001/services/device/BUILD.gn File services/device/BUILD.gn (right): https://codereview.chromium.org/2772183002/diff/40001/services/device/BUILD.g... services/device/BUILD.gn:14: visibility = [ "//content/browser" ] On 2017/03/27 08:45:11, blundell wrote: > Nit: Add a comment here saying that this should be made visible only to > embedders of the Device Service, and that the dependence should only be for the > purpose of embedding the Device Service. Done. https://codereview.chromium.org/2772183002/diff/40001/services/device/BUILD.g... services/device/BUILD.gn:89: visibility = [ "//content/public/android:*" ] On 2017/03/27 08:45:11, blundell wrote: > similar nit to above. Done. https://codereview.chromium.org/2772183002/diff/40001/services/device/screen_... File services/device/screen_orientation/BUILD.gn (right): https://codereview.chromium.org/2772183002/diff/40001/services/device/screen_... services/device/screen_orientation/BUILD.gn:43: visibility = [ "//services/device:*" ] On 2017/03/27 08:45:11, blundell wrote: > nit: Add a comment like: > > "Conceptually, this should be visible only to //services/device:java. However, > various generated targets also need to see this target as a result of > //services/device:java depending on it." Done and Thanks.
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2772183002/#ps60001 (title: "Add comments for changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
[DeviceService] Cleanup of android library dependencies.
This cleanup CL moves java files of screen_orientaion and
time_zone_monitor to their own android library targets, rather than
mixing all of them into the root target //services/device:java,
BUG=none
==========
to
==========
[DeviceService] Cleanup of android library dependencies.
This cleanup CL moves java files of screen_orientaion and
time_zone_monitor to their own android library targets, rather than
mixing all of them into the root target //services/device:java,
This CL also restricts //services/device:{java, lib} to be visible
only to embedders of Device Service.
BUG=none
==========
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by leon.han@intel.com
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": 60001, "attempt_start_ts": 1490611435625010,
"parent_rev": "9d1e105b5f2624e29d22969aaad31838e76ed274", "commit_rev":
"fa99300307c0b45de83ff66cd5f938143e8bbc9a"}
Message was sent while issue was closed.
Description was changed from
==========
[DeviceService] Cleanup of android library dependencies.
This cleanup CL moves java files of screen_orientaion and
time_zone_monitor to their own android library targets, rather than
mixing all of them into the root target //services/device:java,
This CL also restricts //services/device:{java, lib} to be visible
only to embedders of Device Service.
BUG=none
==========
to
==========
[DeviceService] Cleanup of android library dependencies.
This cleanup CL moves java files of screen_orientaion and
time_zone_monitor to their own android library targets, rather than
mixing all of them into the root target //services/device:java,
This CL also restricts //services/device:{java, lib} to be visible
only to embedders of Device Service.
BUG=none
Review-Url: https://codereview.chromium.org/2772183002
Cr-Commit-Position: refs/heads/master@{#459757}
Committed:
https://chromium.googlesource.com/chromium/src/+/fa99300307c0b45de83ff66cd5f9...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/fa99300307c0b45de83ff66cd5f9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
