Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1635143002:
android: Add template to export sandboxed service (Closed)
Descriptionandroid: Add template to export sandboxed service
BUG=581380
Committed: https://crrev.com/a6300110b905d4201f69fa7a9894c3abf2eb9d9d
Cr-Commit-Position: refs/heads/master@{#371601}
Patch Set 1 #
Total comments: 3
Patch Set 2 : conditional tools:ignore #
Total comments: 2
Patch Set 3 : default #Messages
Total messages: 24 (10 generated)
mnaganov@chromium.org changed reviewers: + mnaganov@chromium.org
https://codereview.chromium.org/1635143002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1635143002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:628: tools:ignore="ExportedService" I guess this is only needed when the service is exported. But maybe it doesn't harm when it's not.
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
https://codereview.chromium.org/1635143002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1635143002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:628: tools:ignore="ExportedService" On 2016/01/26 18:25:50, mnaganov wrote: > I guess this is only needed when the service is exported. But maybe it doesn't > harm when it's not. It would be bad if android:exported="true" and andriod:externalService="false" AND we were to turn on multiprocess. But if multiprocess is always going to have externalService="true", then I think that's OK. Though it's probably safest, and best for tooling, if the tools:ignore were wrapped in a condition gated on externalService.
https://codereview.chromium.org/1635143002/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1635143002/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:628: tools:ignore="ExportedService" On 2016/01/26 18:30:39, Robert Sesek wrote: > On 2016/01/26 18:25:50, mnaganov wrote: > > I guess this is only needed when the service is exported. But maybe it doesn't > > harm when it's not. > > It would be bad if android:exported="true" and andriod:externalService="false" > AND we were to turn on multiprocess. But if multiprocess is always going to have > externalService="true", then I think that's OK. Though it's probably safest, and > best for tooling, if the tools:ignore were wrapped in a condition gated on > externalService. Couldn't figure out how to do jinja var with space in gn. So just wrapped the tools:ignore part in an if instead.
boliu@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc for stamp rsesek is "uncomfortably excited" in bug
lgtm
The CQ bit was checked by boliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1635143002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1635143002/20001
lgtm
The CQ bit was unchecked by boliu@chromium.org
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1635143002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1635143002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
https://codereview.chromium.org/1635143002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1635143002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:628: {% if sandboxed_service_exported == 'true' %} Argh. You probably need to repeat the 'default' expression here as well.
https://codereview.chromium.org/1635143002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1635143002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:628: {% if sandboxed_service_exported == 'true' %} On 2016/01/26 19:45:20, mnaganov wrote: > Argh. You probably need to repeat the 'default' expression here as well. doh!
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, mnaganov@chromium.org Link to the patchset: https://codereview.chromium.org/1635143002/#ps40001 (title: "default")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1635143002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1635143002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== android: Add template to export sandboxed service BUG=581380 ========== to ========== android: Add template to export sandboxed service BUG=581380 Committed: https://crrev.com/a6300110b905d4201f69fa7a9894c3abf2eb9d9d Cr-Commit-Position: refs/heads/master@{#371601} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a6300110b905d4201f69fa7a9894c3abf2eb9d9d Cr-Commit-Position: refs/heads/master@{#371601} |
