Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2451193002:
Fix imcompatible app warning when insert to DayDream headset (Closed)
DescriptionFix imcompatible app warning when insert to DayDream headset
Normally, Daydream API categorize activity as Cardboard only, Cardboard
and Daydream hybrid or Daydream only activity. If an activity is Cardboard
only, an imcompatible app warning will show up.
Daydream use intent filter to determine the category of an Activity. Chrome
wants to support both Cardboard and Daydream. So add intent filter to CTA
which is our main VR activity.
BUG=656723
Committed: https://crrev.com/0ff0fbe023a9fa054dd5e2e8cab64003ffbc7a37
Cr-Commit-Position: refs/heads/master@{#431424}
Patch Set 1 #Patch Set 2 : Add webvr flag check #
Total comments: 10
Patch Set 3 : add comment #
Messages
Total messages: 17 (6 generated)
bshe@chromium.org changed reviewers: + mariakhomenko@chromium.org, mthiesse@chromium.org
Hi Maria. Do you mind to take a look at this CL? +mthiesse when we have VrActivity, we would need to do the same to declare that it is a Cardboard and Daydream hybrid activity.
Sorry, not at all familiar with this, so just asking lots of questions. https://codereview.chromium.org/2451193002/diff/20001/chrome/android/chrome_p... File chrome/android/chrome_public_apk_tmpl.gni (right): https://codereview.chromium.org/2451193002/diff/20001/chrome/android/chrome_p... chrome/android/chrome_public_apk_tmpl.gni:34: "enable_webvr=$enable_webvr", What's the difference between enable_vr_shell and enable_webvr? https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:360: <action android:name="org.chromium.chrome.browser.dummy.action" /> Why does a dummy name allow to match intent without any action? Why wouldn't there be an action on the intent? https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:361: <category android:name="com.google.intent.category.DAYDREAM" /> Not too familiar with this -- I have a few questions: 1) Wouldn't this put this activity into Daydream launcher? The comment on ChromeLauncherActivity seems to imply that's a side effect of adding the filter. 2) I imagine only ChromeLauncherActivity and Chrome internally sends intents against ChromeTabbedActivity, so it's kind of weird that we have an intent filter here.
https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:361: <category android:name="com.google.intent.category.DAYDREAM" /> On 2016/10/26 20:10:27, Maria wrote: > Not too familiar with this -- I have a few questions: > > 1) Wouldn't this put this activity into Daydream launcher? The comment on > ChromeLauncherActivity seems to imply that's a side effect of adding the filter. > > 2) I imagine only ChromeLauncherActivity and Chrome internally sends intents > against ChromeTabbedActivity, so it's kind of weird that we have an intent > filter here. I actually just had a discussion with Biao, and we don't want to be in the Daydream launcher at all just yet (the cold launch experience is brutal). Is there any way to fix the incompatible warning without being on the launcher?
https://codereview.chromium.org/2451193002/diff/20001/chrome/android/chrome_p... File chrome/android/chrome_public_apk_tmpl.gni (right): https://codereview.chromium.org/2451193002/diff/20001/chrome/android/chrome_p... chrome/android/chrome_public_apk_tmpl.gni:34: "enable_webvr=$enable_webvr", On 2016/10/26 20:10:27, Maria wrote: > What's the difference between enable_vr_shell and enable_webvr? enable_webvr will enable WebVR related web API and code. enable_vr_shell will enable VrShell system UI and functionality. We also calL VrShell ChromeVR. Mostly, we refer to our VR browser system UI controls(such as navigation button) and pages(such as bookmark). Hope I explained them clearly, if not, feel free to ping me directly. And we are probably going to ship WebVR first and then ChromeVR. They share a lot of VR related code (such as code under vr_shell folder), which makes them confusing. But we are hoping to remove these flags soon. https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:360: <action android:name="org.chromium.chrome.browser.dummy.action" /> On 2016/10/26 20:10:27, Maria wrote: > Why does a dummy name allow to match intent without any action? Based on this doc: https://developer.android.com/guide/components/intents-filters.html#ActionTest It sounds like in order to pass action test for intent with no action, we need to have a dummy action here. > Why wouldn't there be an action on the intent? The way Daydream figure out which category an activity belongs to is to test if a Daydream category intent can be resolved. And they didn't add any action when create the intent. https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:361: <category android:name="com.google.intent.category.DAYDREAM" /> On 2016/10/26 20:15:21, mthiesse wrote: > On 2016/10/26 20:10:27, Maria wrote: > > Not too familiar with this -- I have a few questions: > > > > 1) Wouldn't this put this activity into Daydream launcher? The comment on > > ChromeLauncherActivity seems to imply that's a side effect of adding the > filter. > > > > 2) I imagine only ChromeLauncherActivity and Chrome internally sends intents > > against ChromeTabbedActivity, so it's kind of weird that we have an intent > > filter here. > > I actually just had a discussion with Biao, and we don't want to be in the > Daydream launcher at all just yet (the cold launch experience is brutal). > > Is there any way to fix the incompatible warning without being on the launcher? This won't make Chrome appear in the launcher. To appear in launcher, it requires <action android:name="android.intent.action.MAIN" /> in addition to category. https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:361: <category android:name="com.google.intent.category.DAYDREAM" /> On 2016/10/26 20:10:27, Maria wrote: > Not too familiar with this -- I have a few questions: > > 1) Wouldn't this put this activity into Daydream launcher? The comment on > ChromeLauncherActivity seems to imply that's a side effect of adding the filter. > > 2) I imagine only ChromeLauncherActivity and Chrome internally sends intents > against ChromeTabbedActivity, so it's kind of weird that we have an intent > filter here. So Daydream will try to test if their intents can be resolved by the foreground activity. And based on the result, it categorize the activity to Cardboard only, hybrid or Daydream only. ChromeLauncherActivity is a transit activity. So I have to add intent filter here since this is the activity that actually use Daydream api.
lgtm https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:361: <category android:name="com.google.intent.category.DAYDREAM" /> On 2016/10/26 20:51:46, bshe wrote: > On 2016/10/26 20:10:27, Maria wrote: > > Not too familiar with this -- I have a few questions: > > > > 1) Wouldn't this put this activity into Daydream launcher? The comment on > > ChromeLauncherActivity seems to imply that's a side effect of adding the > filter. > > > > 2) I imagine only ChromeLauncherActivity and Chrome internally sends intents > > against ChromeTabbedActivity, so it's kind of weird that we have an intent > > filter here. > > So Daydream will try to test if their intents can be resolved by the foreground > activity. > And based on the result, it categorize the activity to Cardboard only, hybrid or > Daydream only. > ChromeLauncherActivity is a transit activity. So I have to add intent filter > here since this > is the activity that actually use Daydream api. Ok, can you add this as a comment here -- I think people reading this later may find it very surprising that there's an intent filter on a non-exported activity.
https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2451193002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:361: <category android:name="com.google.intent.category.DAYDREAM" /> On 2016/10/26 21:07:24, Maria wrote: > On 2016/10/26 20:51:46, bshe wrote: > > On 2016/10/26 20:10:27, Maria wrote: > > > Not too familiar with this -- I have a few questions: > > > > > > 1) Wouldn't this put this activity into Daydream launcher? The comment on > > > ChromeLauncherActivity seems to imply that's a side effect of adding the > > filter. > > > > > > 2) I imagine only ChromeLauncherActivity and Chrome internally sends intents > > > against ChromeTabbedActivity, so it's kind of weird that we have an intent > > > filter here. > > > > So Daydream will try to test if their intents can be resolved by the > foreground > > activity. > > And based on the result, it categorize the activity to Cardboard only, hybrid > or > > Daydream only. > > ChromeLauncherActivity is a transit activity. So I have to add intent filter > > here since this > > is the activity that actually use Daydream api. > > Ok, can you add this as a comment here -- I think people reading this later may > find it very surprising that there's an intent filter on a non-exported > activity. Done.
The CQ bit was checked by bshe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mariakhomenko@chromium.org Link to the patchset: https://codereview.chromium.org/2451193002/#ps40001 (title: "add comment")
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by bshe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix imcompatible app warning when insert to DayDream headset Normally, Daydream API categorize activity as Cardboard only, Cardboard and Daydream hybrid or Daydream only activity. If an activity is Cardboard only, an imcompatible app warning will show up. Daydream use intent filter to determine the category of an Activity. Chrome wants to support both Cardboard and Daydream. So add intent filter to CTA which is our main VR activity. BUG=656723 ========== to ========== Fix imcompatible app warning when insert to DayDream headset Normally, Daydream API categorize activity as Cardboard only, Cardboard and Daydream hybrid or Daydream only activity. If an activity is Cardboard only, an imcompatible app warning will show up. Daydream use intent filter to determine the category of an Activity. Chrome wants to support both Cardboard and Daydream. So add intent filter to CTA which is our main VR activity. BUG=656723 Committed: https://crrev.com/0ff0fbe023a9fa054dd5e2e8cab64003ffbc7a37 Cr-Commit-Position: refs/heads/master@{#431424} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0ff0fbe023a9fa054dd5e2e8cab64003ffbc7a37 Cr-Commit-Position: refs/heads/master@{#431424} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
