Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1231153002:
Add VR desktop activity structure (Closed)
|
Created:
5 years, 5 months ago by shichengfeng Modified:
5 years, 5 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid Chromoting: Add VR desktop activity structure.
1. Add dependency to the Cardboard SDK, which will be used to render 3D scenes.
2. Add new permissions: android.permission.NFC(required by the Cardboard SDK to access Cardboard's NFC tag), android.permission.READ_EXTERNAL_STORAGE and android.permission.WRITE_EXTERNAL_STORAGE(these permissions are required by the Cardboard SDK to pair the user's phone to their VR viewer).
3. Create VRDesktop activity enabling user to interact with the VR Chromoting desktop.
4. Create CardboardView renderer that will be used to generate 3D graphics.
Committed: https://crrev.com/0cb1dabcd1c0c2cd802880a72f7ffa48c5cca2b3
Cr-Commit-Position: refs/heads/master@{#338730}
Patch Set 1 : add vr desktop basic structure #
Total comments: 16
Patch Set 2 : improve documentation #
Total comments: 4
Patch Set 3 : improve documentation #
Total comments: 6
Patch Set 4 : fix naming convention #
Total comments: 6
Patch Set 5 : add decider for Cardboard permission in Manifest #
Total comments: 4
Patch Set 6 : improve documentation #
Total comments: 2
Patch Set 7 : add cardboard desktop activity into conditional case #
Messages
Total messages: 29 (6 generated)
Patchset #1 (id:1) has been deleted
shichengfeng@google.com changed reviewers: + lambroslambrou@chromium.org
create vr desktop basic structure
https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/A... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:53: <category android:name="com.google.intent.category.CARDBOARD" /> Could you explain what this intent-filter will be used for? We have to be careful about adding new filters, as they open up the app to communication from external apps. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/r... File remoting/android/java/res/layout/vr_desktop.xml (right): https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/r... remoting/android/java/res/layout/vr_desktop.xml:3: <!-- Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/r... remoting/android/java/res/layout/vr_desktop.xml:8: <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" You have only 1 child, so I think you can change this RelativeLayout to <merge>. And then you don't need layout_width or layout_height here (though you still need them in the CardboardView). https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/r... remoting/android/java/res/layout/vr_desktop.xml:9: android:layout_width="match_parent" Use spaces instead of tab. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/VRDesktop.java (right): https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktop.java:13: * A simple screen that does nothing except display a VR DesktopView and notify it of rotations. "screen that does nothing"? That might be true currently, but I guess you will add more stuff to it later? Will this be the main Activity for showing a VR view of the desktop? https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktop.java:22: // Associate a CardboardView.StereoRenderer with cardboardView. Blank line before comment. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktop.java:24: // Associate the cardboardView with this activity. Blank line before comment. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/VRDesktopRenderer.java (right): https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktopRenderer.java:17: * Renderer for Cardboard view +1 space indent. Period at end of sentence.
Description should be a grammatical English sentence (beginning with capital letter, ending with period). Also, I usually add "Android Chromoting: " at the beginning, to make it obvious to someone reading the commit message that this CL is for the Android CRD app. Also, s/vr/VR/. After the description, add a blank line, then add some more detail about what the CL does and why. For example, say that this adds a new Activity class that handles VR rendering for the remote desktop. And it also adds a dependency on the Android Cardboard SDK.
Improve documentation https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/A... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:53: <category android:name="com.google.intent.category.CARDBOARD" /> On 2015/07/10 22:25:03, Lambros wrote: > Could you explain what this intent-filter will be used for? We have to be > careful about adding new filters, as they open up the app to communication from > external apps. The intent-filter and specifically com.google.intent.category.CARDBOARD state that this activity is compatible with Cardboard-like viewers. This category is used by the Cardboard app to list compatible apps installed on the user's phone. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/r... File remoting/android/java/res/layout/vr_desktop.xml (right): https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/r... remoting/android/java/res/layout/vr_desktop.xml:3: <!-- Copyright 2014 The Chromium Authors. All rights reserved. On 2015/07/10 22:25:03, Lambros wrote: > 2015 Done. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/r... remoting/android/java/res/layout/vr_desktop.xml:8: <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2015/07/10 22:25:03, Lambros wrote: > You have only 1 child, so I think you can change this RelativeLayout to <merge>. > And then you don't need layout_width or layout_height here (though you still > need them in the CardboardView). Done. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/r... remoting/android/java/res/layout/vr_desktop.xml:9: android:layout_width="match_parent" On 2015/07/10 22:25:03, Lambros wrote: > Use spaces instead of tab. Done. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/VRDesktop.java (right): https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktop.java:13: * A simple screen that does nothing except display a VR DesktopView and notify it of rotations. On 2015/07/10 22:25:03, Lambros wrote: > "screen that does nothing"? > That might be true currently, but I guess you will add more stuff to it later? > Will this be the main Activity for showing a VR view of the desktop? Done. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktop.java:22: // Associate a CardboardView.StereoRenderer with cardboardView. On 2015/07/10 22:25:03, Lambros wrote: > Blank line before comment. Done. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktop.java:24: // Associate the cardboardView with this activity. On 2015/07/10 22:25:03, Lambros wrote: > Blank line before comment. Done. https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/VRDesktopRenderer.java (right): https://codereview.chromium.org/1231153002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktopRenderer.java:17: * Renderer for Cardboard view On 2015/07/10 22:25:03, Lambros wrote: > +1 space indent. Period at end of sentence. Done.
Description is much better, thanks! Just a few tweaks: s/the Cardboard/the Cardboard SDK/ s/sences/scenes./ I suggest re-phrasing part 2: "Create VRDesktop activity enabling user to interact with the VR Chromoting desktop." And each sentence should end with a period. https://codereview.chromium.org/1231153002/diff/40001/remoting/android/java/A... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1231153002/diff/40001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:11: <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" /> Could you edit the CL description to say that you're adding new permissions to the app? And explain what they are used for? Is NFC needed for the Cardboard button? Why do we need read/write for external storage? https://codereview.chromium.org/1231153002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/VRDesktop.java (right): https://codereview.chromium.org/1231153002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktop.java:13: * VR Remote Desktop activity nit: End sentence with period.
improve documentation https://codereview.chromium.org/1231153002/diff/40001/remoting/android/java/A... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1231153002/diff/40001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:11: <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" /> On 2015/07/11 00:32:13, Lambros wrote: > Could you edit the CL description to say that you're adding new permissions to > the app? And explain what they are used for? > Is NFC needed for the Cardboard button? > Why do we need read/write for external storage? Done. https://codereview.chromium.org/1231153002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/VRDesktop.java (right): https://codereview.chromium.org/1231153002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktop.java:13: * VR Remote Desktop activity On 2015/07/11 00:32:13, Lambros wrote: > nit: End sentence with period. Done.
lambroslambrou@chromium.org changed reviewers: + sergeyu@chromium.org
+sergeyu LG, but I'd like another pair of eyes.
https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/A... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:12: <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" /> nit: sort permissions alphabetically? https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/VRDesktop.java (right): https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktop.java:15: public class VRDesktop extends CardboardActivity { I don't like this name. Style guide discourages abbreviations like VR. Why not call it CardboardDesktop? Or actually I think CardboardDesktopActivity is a better name to make it clear that it's an activity. https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/VRDesktopRenderer.java (right): https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktopRenderer.java:19: public class VRDesktopRenderer implements CardboardView.StereoRenderer { CardboardDesktopRenderer?
rename files and reoder permissions https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/A... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:12: <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" /> On 2015/07/13 19:26:19, Sergey Ulanov wrote: > nit: sort permissions alphabetically? Done. https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/VRDesktop.java (right): https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktop.java:15: public class VRDesktop extends CardboardActivity { On 2015/07/13 19:26:19, Sergey Ulanov wrote: > I don't like this name. Style guide discourages abbreviations like VR. Why not > call it CardboardDesktop? Or actually I think CardboardDesktopActivity is a > better name to make it clear that it's an activity. Done. https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/VRDesktopRenderer.java (right): https://codereview.chromium.org/1231153002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/VRDesktopRenderer.java:19: public class VRDesktopRenderer implements CardboardView.StereoRenderer { On 2015/07/13 19:26:19, Sergey Ulanov wrote: > CardboardDesktopRenderer? Done.
Please see my comments about new permissions - I think we need a compile-time flag to enable/disable this feature. https://codereview.chromium.org/1231153002/diff/80001/remoting/android/java/A... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1231153002/diff/80001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:9: <uses-permission android:name="android.permission.NFC" /> Looking again at this I think these new permissions will trigger permission prompt for the user when updating. We want to avoid it as much as possible. So I think we want a compile-time flag to keep these permissions disabled until we are ready to ship this feature. This is already a jinja2 template, so adding these permissions conditionally should be easy. E.g. see remoting/webapp/crd/manifest.json.jinja2 for an example of how you can include some parts of the file depending on a compile-time flag. https://codereview.chromium.org/1231153002/diff/80001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:47: <activity android:name="org.chromium.chromoting.VRDesktop" Update class name here too? https://codereview.chromium.org/1231153002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java (right): https://codereview.chromium.org/1231153002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:13: * VR Remote Desktop activity. Update the comment as well please. E.g. something like: "Activity that renders virtual desktop for Cardboard."
+1 to putting this behind a GYP flag. The feature needs to be disabled until it is in a shippable state. It's OK to add code for this feature, but anything that is user-facing needs to be disabled until it's ready.
add decider for Cardboard permission in Manifest https://codereview.chromium.org/1231153002/diff/80001/remoting/android/java/A... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1231153002/diff/80001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:9: <uses-permission android:name="android.permission.NFC" /> On 2015/07/13 20:21:51, Sergey Ulanov wrote: > Looking again at this I think these new permissions will trigger permission > prompt for the user when updating. We want to avoid it as much as possible. So I > think we want a compile-time flag to keep these permissions disabled until we > are ready to ship this feature. This is already a jinja2 template, so adding > these permissions conditionally should be easy. E.g. see > remoting/webapp/crd/manifest.json.jinja2 for an example of how you can include > some parts of the file depending on a compile-time flag. Done. https://codereview.chromium.org/1231153002/diff/80001/remoting/android/java/A... remoting/android/java/AndroidManifest.xml.jinja2:47: <activity android:name="org.chromium.chromoting.VRDesktop" On 2015/07/13 20:21:51, Sergey Ulanov wrote: > Update class name here too? Done. https://codereview.chromium.org/1231153002/diff/80001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java (right): https://codereview.chromium.org/1231153002/diff/80001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopActivity.java:13: * VR Remote Desktop activity. On 2015/07/13 20:21:51, Sergey Ulanov wrote: > Update the comment as well please. E.g. something like: "Activity that renders > virtual desktop for Cardboard." Done.
https://codereview.chromium.org/1231153002/diff/100001/remoting/remoting_andr... File remoting/remoting_android.gypi (right): https://codereview.chromium.org/1231153002/diff/100001/remoting/remoting_andr... remoting/remoting_android.gypi:108: '../third_party/cardboard-java/cardboard.gyp:cardboard_jar', Does this dependency affect package size or anything else? If it does then maybe we should also enable it only when the feature is enabled? https://codereview.chromium.org/1231153002/diff/100001/remoting/remoting_opti... File remoting/remoting_options.gypi (right): https://codereview.chromium.org/1231153002/diff/100001/remoting/remoting_opti... remoting/remoting_options.gypi:18: # Set this to enable Chromoting Cardboard Activity. indicate that this is for Android, I suggest: # Enables Cardboard support on Android.
https://codereview.chromium.org/1231153002/diff/100001/remoting/remoting_andr... File remoting/remoting_android.gypi (right): https://codereview.chromium.org/1231153002/diff/100001/remoting/remoting_andr... remoting/remoting_android.gypi:108: '../third_party/cardboard-java/cardboard.gyp:cardboard_jar', On 2015/07/13 23:58:05, Sergey Ulanov wrote: > Does this dependency affect package size or anything else? If it does then maybe > we should also enable it only when the feature is enabled? It will affect package size, but not very much. Cardboard+protobuf is only about 200KB. We'll have to take this size hit eventually anyway. If we don't add this dependency now, it means that the Cardboard-related Java code won't compile. So all the Java implementation will have to be cordoned off into a separate android/cardboard directory which is only included conditionally, and none of the trybots will compile it. I think it's better to take the size hit now, and have the build-bots actually build and check the Java code, even though it won't be called at runtime. We'll get the benefit of tools like Lint/FindBugs checking our code. WDYT?
improve documentation
lgtm https://codereview.chromium.org/1231153002/diff/100001/remoting/remoting_andr... File remoting/remoting_android.gypi (right): https://codereview.chromium.org/1231153002/diff/100001/remoting/remoting_andr... remoting/remoting_android.gypi:108: '../third_party/cardboard-java/cardboard.gyp:cardboard_jar', On 2015/07/14 01:18:17, Lambros wrote: > On 2015/07/13 23:58:05, Sergey Ulanov wrote: > > Does this dependency affect package size or anything else? If it does then > maybe > > we should also enable it only when the feature is enabled? > > It will affect package size, but not very much. Cardboard+protobuf is only about > 200KB. We'll have to take this size hit eventually anyway. > > If we don't add this dependency now, it means that the Cardboard-related Java > code won't compile. So all the Java implementation will have to be cordoned off > into a separate android/cardboard directory which is only included > conditionally, and none of the trybots will compile it. > > I think it's better to take the size hit now, and have the build-bots actually > build and check the Java code, even though it won't be called at runtime. We'll > get the benefit of tools like Lint/FindBugs checking our code. > > WDYT? I agree as long as this doesn't bloat the package very significantly.
lgtm At some point, you will need to detect from Java whether the GYP enable_cardboard option is set. For example, you might want to only display a Cardboard button if the flag is set. One solution would be to add another <meta-data...> section to the AndroidManifest.xml, and use the Jinja template to set the content according to the enable_cardboard flag. Another solution might be to look for the presence of a cardboard-specific resource.
On 2015/07/14 18:16:03, Lambros wrote: > lgtm > > At some point, you will need to detect from Java whether the GYP > enable_cardboard option is set. For example, you might want to only display a > Cardboard button if the flag is set. > > One solution would be to add another <meta-data...> section to the > AndroidManifest.xml, and use the Jinja template to set the content according to > the enable_cardboard flag. > > Another solution might be to look for the presence of a cardboard-specific > resource. Please rename vr_desktop.xml to cardboard_desktop.xml (and update CardboardDesktopActivity.onCreate()).
https://codereview.chromium.org/1231153002/diff/110001/remoting/android/java/... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1231153002/diff/110001/remoting/android/java/... remoting/android/java/AndroidManifest.xml.jinja2:55: <intent-filter> This intent filter needs to be in the ENABLE_CARDBOARD conditional.
Patchset #7 (id:130001) has been deleted
The CQ bit was checked by shichengfeng@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/1231153002/#ps150001 (title: "add cardboard desktop activity into conditional case")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1231153002/150001
Add Cardboard desktop activity into conditional case in Manifest https://codereview.chromium.org/1231153002/diff/110001/remoting/android/java/... File remoting/android/java/AndroidManifest.xml.jinja2 (right): https://codereview.chromium.org/1231153002/diff/110001/remoting/android/java/... remoting/android/java/AndroidManifest.xml.jinja2:55: <intent-filter> On 2015/07/14 18:20:57, Lambros wrote: > This intent filter needs to be in the ENABLE_CARDBOARD conditional. Done.
Message was sent while issue was closed.
Committed patchset #7 (id:150001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0cb1dabcd1c0c2cd802880a72f7ffa48c5cca2b3 Cr-Commit-Position: refs/heads/master@{#338730} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
