Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
864663004:
Quick-start instructions for Cronet Sample Application (Closed)
DescriptionQuick-start instructions for Cronet Sample Application
BUG=none
Committed: https://crrev.com/f6016191486875a86193804ab6fa603bd18f6a91
Cr-Commit-Position: refs/heads/master@{#315102}
Patch Set 1 #
Total comments: 33
Patch Set 2 : addenda to README in cronet sample app #
Total comments: 1
Patch Set 3 : more changes to cronet sample app README, per Misha #Patch Set 4 : cross-ref to code.google.com about STUDIO_JDK env variable #Patch Set 5 : more touch-ups to cronet sample app README #Messages
Total messages: 14 (2 generated)
dougk@chromium.org changed reviewers: + mef@chromium.org, pauljensen@chromium.org
These look very helpful; thanks for putting them together! I have very little experience with Android Studio so I'm not really a good person to review this. I work almost entirely from the command line. I'd (and I think many developers would) appreciate if the sample app came with build files (doesn't look like it now) so someone can just run "ant debug installd" to build and install. https://developer.android.com/tools/building/building-cmdline-ant.html https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... File components/cronet/android/sample/README (right): https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:2: How to set up and run the sample app as an android studio project. android studio -> Android Studio https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:4: Linux (studio version 0.8.11 beta) studio -> Android Studio https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:24: (c) Copy 'cronet.jar' and 'cronet_stub.jar' to "libs" Should we explain where these files come from and provide a pointer for how to build them? (perhaps a link to another README about building cronet) https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:66: etc Should we provide a pointer for what to do if the files aren't present? https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:90: (Don't do these, they're screwy) Why are we including these if they aren't a good idea? https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:120: Ideally the two .jar files and one .so file could be delivered as one .aar Should we put this under a "SIDE NOTE" title indicating this isn't applicable to the typical user?
On 2015/02/05 15:23:58, pauljensen wrote: > These look very helpful; thanks for putting them together! +1. I think we are planning to use markdown (http://en.wikipedia.org/wiki/Markdown) for our README files, so might as well start now. > I have very little experience with Android Studio so I'm not really a good > person to review this. > I work almost entirely from the command line. I'd (and I think many developers > would) appreciate if the sample app came with build files (doesn't look like it > now) so someone can just run "ant debug installd" to build and install. > https://developer.android.com/tools/building/building-cmdline-ant.html That makes total sense, and should be more scriptable. I only wonder about ant vs gradle as the current hotness.
https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... File components/cronet/android/sample/README (right): https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:6: (1) Launch Android Studio Should line items be complete sentences with periods? https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:9: - navigate to chromium/src/components/cronet/android/sample Conceivably Sample app source code is / could be packaged with Cronet build. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:17: "Project" view, not "Android" view. "Project" models I like elaborate explanation! https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:24: (c) Copy 'cronet.jar' and 'cronet_stub.jar' to "libs" On 2015/02/05 15:23:58, pauljensen wrote: > Should we explain where these files come from and provide a pointer for how to > build them? (perhaps a link to another README about building cronet) I think link to another README about building is fine, but this README I would target towards app developers who would want to try/use pre-built Cronet. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:26: (4) Inform the IDE about the Jar files Is step 3c required if we are doing 4? I mean, could you just inform IDE about jars that sit elsewhere? https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:28: (b) Right mouse FWIW Mac doesn't have right mouse. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:50: If the project doesn't build, Should this be a 'troubleshooting' section? https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:111: and change the name to "src/main/jniLibs" I like this explanation, maybe there should be a FYI / Reference section? https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:120: Ideally the two .jar files and one .so file could be delivered as one .aar On 2015/02/05 15:23:58, pauljensen wrote: > Should we put this under a "SIDE NOTE" title indicating this isn't applicable to > the typical user? I think it is more of TODO items for ourselves to make it more friendly.
https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... File components/cronet/android/sample/README (right): https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:2: How to set up and run the sample app as an android studio project. On 2015/02/05 15:23:58, pauljensen wrote: > android studio -> Android Studio Done. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:4: Linux (studio version 0.8.11 beta) On 2015/02/05 15:23:58, pauljensen wrote: > studio -> Android Studio Done. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:24: (c) Copy 'cronet.jar' and 'cronet_stub.jar' to "libs" yes, we should. Would it be ok to have the initial checkin of this make some assumptions about what you already have rather than explain how to do a strictly bottom-up assembly of all constituent pieces? https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:66: etc I'll explain that the jniLibs directory should have a slightly different icon from the icon for a "plain old directory" in the tree view, which gives it the magic behavior of stuffing its contents as-is into the 'apk' Unfortunately if it doesn't have that behavior, I don't actually know what to do myself. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:90: (Don't do these, they're screwy) On 2015/02/05 15:23:58, pauljensen wrote: > Why are we including these if they aren't a good idea? Well, I thought it strange that the steps above involved dropping to a shell prompt. Surely there's a UI for it, right? Or so I thought. But ultimately it is no more more convenient unless you have a particular fondness for menu choices and an aversion to shell commands. So, pick your poison: whip out a shell command, or use the menu and manually override what it would do by default. That was my rationale but I'm ok with removing this section entirely, or calling it "If you prefer to use menu selections" Incidentally, this is a known problem - I chatted with a developer of the Android Studio about it, and he says that the UI lacks polish that would make this feel less clunky. The flaw is that the obvious menu choices "Add JNI" and "Add resource" mean "I want to _produce_ these", and not "I want to _use_ these". But you can coerce one into the other by counteracting the default behavior through renaming of the directory that it creates. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:120: Ideally the two .jar files and one .so file could be delivered as one .aar On 2015/02/05 15:23:58, pauljensen wrote: > Should we put this under a "SIDE NOTE" title indicating this isn't applicable to > the typical user? Done.
ptal https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... File components/cronet/android/sample/README (right): https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:6: (1) Launch Android Studio On 2015/02/05 17:02:46, mef wrote: > Should line items be complete sentences with periods? In theory the outline form itself need not have a period one the single-line pieces, but it's hard to say since I've made some paragraph-length pieces which are technically no longer outline form. And there's the question of period inside (grammatically correct) or outside of a trailing final quote. I'll be a little more generous with the periods, and start with a capital letter though. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:9: - navigate to chromium/src/components/cronet/android/sample On 2015/02/05 17:02:46, mef wrote: > Conceivably Sample app source code is / could be packaged with Cronet build. Is that a suggested addition to the text? https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:17: "Project" view, not "Android" view. "Project" models On 2015/02/05 17:02:46, mef wrote: > I like elaborate explanation! Acknowledged. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:26: (4) Inform the IDE about the Jar files On 2015/02/05 17:02:46, mef wrote: > Is step 3c required if we are doing 4? I mean, could you just inform IDE about > jars that sit elsewhere? Yes, it's fine to have the jars sitting next to '.java' files. I tried that. I wasn't sure what was canonical. But I'm fairly certain it is not idiomatic to point at places in the filesystem outside the project, with the exception of network-accessible packages. I'll say something about "you can leave them where you like" https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:28: (b) Right mouse On 2015/02/05 17:02:46, mef wrote: > FWIW Mac doesn't have right mouse. Indeed, it is control+click on the Mac. Not only that, I could not even find a keyboard equivalent for this step. I could add a parenthetical remark - [Note: the keyboard equivalent for these steps seems to be nonexistent. You may replace this step with the resolution advice for Problem #2 under "If the project does not build"] Wdyt? https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:50: If the project doesn't build, On 2015/02/05 17:02:46, mef wrote: > Should this be a 'troubleshooting' section? yes, will rename. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:111: and change the name to "src/main/jniLibs" On 2015/02/05 17:02:46, mef wrote: > I like this explanation, maybe there should be a FYI / Reference section? Will move this to a Footnotes section. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:114: But you don't want to trigger that rule because it will complains if subject/verb agreement: it will complain. https://codereview.chromium.org/864663004/diff/20001/components/cronet/androi... File components/cronet/android/sample/README (right): https://codereview.chromium.org/864663004/diff/20001/components/cronet/androi... components/cronet/android/sample/README:94: MacOS (studio version 1.0.1) oh forgot this "Android Studio"
https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... File components/cronet/android/sample/README (right): https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:9: - navigate to chromium/src/components/cronet/android/sample On 2015/02/05 22:42:36, dougk wrote: > On 2015/02/05 17:02:46, mef wrote: > > Conceivably Sample app source code is / could be packaged with Cronet build. > > Is that a suggested addition to the text? Yeah, I'd say something like 'navigate to location of sample source code'. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:28: (b) Right mouse On 2015/02/05 22:42:36, dougk wrote: > On 2015/02/05 17:02:46, mef wrote: > > FWIW Mac doesn't have right mouse. > > Indeed, it is control+click on the Mac. Not only that, I could not even find a > keyboard equivalent for this step. I could add a parenthetical remark - > [Note: the keyboard equivalent for these steps seems to be nonexistent. You may > replace this step with the resolution advice for Problem #2 under "If the > project does not build"] > > Wdyt? I expect target audience to be familiar with UI patterns specific to their platform, so maybe 'select both files, pick "Add as Library" from context menu' is sufficient?
lgtm
https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... File components/cronet/android/sample/README (right): https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:9: - navigate to chromium/src/components/cronet/android/sample On 2015/02/05 22:56:12, mef wrote: > On 2015/02/05 22:42:36, dougk wrote: > > On 2015/02/05 17:02:46, mef wrote: > > > Conceivably Sample app source code is / could be packaged with Cronet build. > > > > > Is that a suggested addition to the text? > > Yeah, I'd say something like 'navigate to location of sample source code'. Done. https://codereview.chromium.org/864663004/diff/1/components/cronet/android/sa... components/cronet/android/sample/README:28: (b) Right mouse On 2015/02/05 22:56:12, mef wrote: > On 2015/02/05 22:42:36, dougk wrote: > > On 2015/02/05 17:02:46, mef wrote: > > > FWIW Mac doesn't have right mouse. > > > > Indeed, it is control+click on the Mac. Not only that, I could not even find a > > keyboard equivalent for this step. I could add a parenthetical remark - > > [Note: the keyboard equivalent for these steps seems to be nonexistent. You > may > > replace this step with the resolution advice for Problem #2 under "If the > > project does not build"] > > > > Wdyt? > > I expect target audience to be familiar with UI patterns specific to their > platform, so maybe 'select both files, pick "Add as Library" from context menu' > is sufficient? Done.
The CQ bit was checked by dougk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/864663004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f6016191486875a86193804ab6fa603bd18f6a91 Cr-Commit-Position: refs/heads/master@{#315102} |
