Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2527953002:
[gmscore] Add Google Play services documentation (Closed)
Description[gmscore] Add Google Play services documentation
BUG=671699
Committed: https://crrev.com/e56d968dd2d4ef986a3383bb95bb35ec4f966483
Cr-Commit-Position: refs/heads/master@{#436667}
Patch Set 1 #
Total comments: 12
Patch Set 2 : comments #
Total comments: 6
Patch Set 3 : comments #Messages
Total messages: 23 (11 generated)
dgn@chromium.org changed reviewers: + jbudorick@chromium.org, mcasas@chromium.org
PTAL mcasas@: does that make sense? any information I could add that would have helped you?
lgtm, thanks! (Also, if you need it, link to BUG=665150) https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.md File docs/google_play_services.md (right): https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:26: > you will need to locally download the Google Play services SDK repository. nit: probably these two lines don't need to be quoted. https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:53: [bug_link]:https://bugs.chromium.org/p/chromium/issues/entry?labels=Restrict-View-Google,pri-1,Hotlist-GooglePlayServices&owner=dgn@chromium.org&os=Android Great stuff, if you want you could also link as example the use case we're dealing with, i.e. vision APIs, where you landed the new package and BUILD.gn [1] and gmscore update in [2], to be used in [3]. For local development and before landing all that code, I essentially needed [1] and `update.py sdk` (l.28). [1] https://crrev.com/2524063002 [2] https://chrome-internal-review.googlesource.com/#/c/307576/ [3] https://crrev.com/2512123002
https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.md File docs/google_play_services.md (right): https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:26: > you will need to locally download the Google Play services SDK repository. On 2016/11/24 22:19:12, mcasas wrote: > nit: probably these two lines don't need to be quoted. I just wanted to make that more visible. (preview: https://user.git.corp.google.com/dgn/md-tests/+/master/google_play_services.md) unnecessary? https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:53: [bug_link]:https://bugs.chromium.org/p/chromium/issues/entry?labels=Restrict-View-Google,pri-1,Hotlist-GooglePlayServices&owner=dgn@chromium.org&os=Android On 2016/11/24 22:19:12, mcasas wrote: > Great stuff, if you want you could also link as example the use > case we're dealing with, i.e. vision APIs, where you landed the > new package and BUILD.gn [1] and gmscore update in [2], > to be used in [3]. > > For local development and before landing all that code, I > essentially needed [1] and `update.py sdk` (l.28). > > [1] https://crrev.com/2524063002 > [2] https://chrome-internal-review.googlesource.com/#/c/307576/ > [3] https://crrev.com/2512123002 I don't really want to explain exactly how to roll out the update yourself here because you need internal access to update the library anyway. I'll update the internal doc to include the example, there it would be more useful I think.
lgtm w/ nits Thank you for writing this! https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.md File docs/google_play_services.md (right): https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:17: (for example Cast, GCM, Android Pay, etc.) To avoid downloading a lot of data we nit: . outside ) https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:26: > you will need to locally download the Google Play services SDK repository. On 2016/11/25 16:53:52, dgn wrote: > On 2016/11/24 22:19:12, mcasas wrote: > > nit: probably these two lines don't need to be quoted. > > I just wanted to make that more visible. (preview: > https://user.git.corp.google.com/dgn/md-tests/+/master/google_play_services.md) > > unnecessary? I'm fine with this as-is, though you could also consider something like **Note**: If you are working ... https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:46: Just [file an issue][bug_link] and assign it to dgn@chromium.org Nit: I would just incorporate this into the previous sentence as [make a request][bug link] and leave your email address out of the visible text since it's already in the bug template. https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:53: [bug_link]:https://bugs.chromium.org/p/chromium/issues/entry?labels=Restrict-View-Google,pri-1,Hotlist-GooglePlayServices&owner=dgn@chromium.org&os=Android On 2016/11/25 16:53:52, dgn wrote: > On 2016/11/24 22:19:12, mcasas wrote: > > Great stuff, if you want you could also link as example the use > > case we're dealing with, i.e. vision APIs, where you landed the > > new package and BUILD.gn [1] and gmscore update in [2], > > to be used in [3]. > > > > For local development and before landing all that code, I > > essentially needed [1] and `update.py sdk` (l.28). > > > > [1] https://crrev.com/2524063002 > > [2] https://chrome-internal-review.googlesource.com/#/c/307576/ > > [3] https://crrev.com/2512123002 > > I don't really want to explain exactly how to roll out the update yourself here > because you need internal access to update the library anyway. I'll update the > internal doc to include the example, there it would be more useful I think. This would be great.
The CQ bit was checked by dgn@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...
PTAL https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.md File docs/google_play_services.md (right): https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:17: (for example Cast, GCM, Android Pay, etc.) To avoid downloading a lot of data we On 2016/11/29 16:09:35, jbudorick wrote: > nit: . outside ) Done. https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:26: > you will need to locally download the Google Play services SDK repository. On 2016/11/29 16:09:35, jbudorick wrote: > On 2016/11/25 16:53:52, dgn wrote: > > On 2016/11/24 22:19:12, mcasas wrote: > > > nit: probably these two lines don't need to be quoted. > > > > I just wanted to make that more visible. (preview: > > > https://user.git.corp.google.com/dgn/md-tests/+/master/google_play_services.md) > > > > unnecessary? > > I'm fine with this as-is, though you could also consider something like > > **Note**: If you are working ... Done. https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:46: Just [file an issue][bug_link] and assign it to dgn@chromium.org On 2016/11/29 16:09:35, jbudorick wrote: > Nit: I would just incorporate this into the previous sentence as [make a > request][bug link] and leave your email address out of the visible text since > it's already in the bug template. Done. https://codereview.chromium.org/2527953002/diff/1/docs/google_play_services.m... docs/google_play_services.md:53: [bug_link]:https://bugs.chromium.org/p/chromium/issues/entry?labels=Restrict-View-Google,pri-1,Hotlist-GooglePlayServices&owner=dgn@chromium.org&os=Android On 2016/11/29 16:09:35, jbudorick wrote: > On 2016/11/25 16:53:52, dgn wrote: > > On 2016/11/24 22:19:12, mcasas wrote: > > > Great stuff, if you want you could also link as example the use > > > case we're dealing with, i.e. vision APIs, where you landed the > > > new package and BUILD.gn [1] and gmscore update in [2], > > > to be used in [3]. > > > > > > For local development and before landing all that code, I > > > essentially needed [1] and `update.py sdk` (l.28). > > > > > > [1] https://crrev.com/2524063002 > > > [2] https://chrome-internal-review.googlesource.com/#/c/307576/ > > > [3] https://crrev.com/2512123002 > > > > I don't really want to explain exactly how to roll out the update yourself > here > > because you need internal access to update the library anyway. I'll update the > > internal doc to include the example, there it would be more useful I think. > > This would be great. Acknowledged.
still lgtm, though with more nits https://codereview.chromium.org/2527953002/diff/20001/docs/google_play_servic... File docs/google_play_services.md (right): https://codereview.chromium.org/2527953002/diff/20001/docs/google_play_servic... docs/google_play_services.md:11: Android devices, and [libraries][dev_doc] to interact with them. Chrome relies nit: no , after devices https://codereview.chromium.org/2527953002/diff/20001/docs/google_play_servic... docs/google_play_services.md:15: is to import it through the Android SDK manager, as a Maven repository. That nit: no , after manager https://codereview.chromium.org/2527953002/diff/20001/docs/google_play_servic... docs/google_play_services.md:16: repository contains multiple versions of the library, split into separate APIs nit: no , after library https://codereview.chromium.org/2527953002/diff/20001/docs/google_play_servic... docs/google_play_services.md:18: won't need to build Chrome, android checkouts of Chromium will download an nit: Use present rather than future tense here. s/won't/don't/, s/will// https://codereview.chromium.org/2527953002/diff/20001/docs/google_play_servic... docs/google_play_services.md:48: passes, it might fail on the internal bots, and result in the CL getting nit: no , after bots https://codereview.chromium.org/2527953002/diff/20001/docs/google_play_servic... docs/google_play_services.md:49: reverted. So please make sure the APIs are available to the bots before nit: s/. So/, so/
PTAL. Preview at https://go/dgn-md-tests/google_play_services.md
lgtm
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2527953002/#ps40001 (title: "comments")
thanks!
The CQ bit was unchecked by dgn@chromium.org
Description was changed from ========== [gmscore] Add Google Play services documentation BUG= ========== to ========== [gmscore] Add Google Play services documentation BUG=671699 ==========
The CQ bit was checked by dgn@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": 40001, "attempt_start_ts": 1481049928218620,
"parent_rev": "8d33da086201d3d2f8af853877d3be7899216f74", "commit_rev":
"87ead255f2590c79250e925ac046161e479601d4"}
Message was sent while issue was closed.
Description was changed from ========== [gmscore] Add Google Play services documentation BUG=671699 ========== to ========== [gmscore] Add Google Play services documentation BUG=671699 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [gmscore] Add Google Play services documentation BUG=671699 ========== to ========== [gmscore] Add Google Play services documentation BUG=671699 Committed: https://crrev.com/e56d968dd2d4ef986a3383bb95bb35ec4f966483 Cr-Commit-Position: refs/heads/master@{#436667} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e56d968dd2d4ef986a3383bb95bb35ec4f966483 Cr-Commit-Position: refs/heads/master@{#436667} |
