Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699643003:
Show an infobar when VR services need to be installed or updated. (Closed)
DescriptionShow an infobar when VR services need to be installed or updated.
BUG=670166
Review-Url: https://codereview.chromium.org/2699643003
Cr-Commit-Position: refs/heads/master@{#451372}
Committed: https://chromium.googlesource.com/chromium/src/+/c4911c48c072e710d9948a470936030ccb105945
Patch Set 1 #Patch Set 2 : DO NO SUBMIT. place holder icons and other clean up #Patch Set 3 : Add install vs update check, final icon resources. #
Total comments: 7
Patch Set 4 : Updated button text resources and adjusted version check. #
Total comments: 10
Patch Set 5 : Optimized pngs #Patch Set 6 : address comments #Patch Set 7 : Make enum android specific. #
Total comments: 3
Patch Set 8 : rebase #Patch Set 9 : update text resources, fix naming mismatches #Messages
Total messages: 39 (16 generated)
amp@chromium.org changed reviewers: + bshe@chromium.org
I initially thought we needed our own infobar delegate, but we really don't need any custom logic at this point. When we support desktop we probably will need our own custom delegate to support the varying vr sdk's. The images are copies (with one bit modified to pass lint duplication check) of the 3d api blocked image until we can get an official icon for vr services that can be open sourced. Also still need to update the logic to differentiate between update and install and get the correct version for VrCore to check against.
amp@chromium.org changed reviewers: + bauerb@chromium.org, mthiesse@chromium.org, rkaplow@chromium.org
rkaplow: Please review histogram changes. bauerb: Please review resrouce changes/additions. bshe/mtheisse: Please review vr changes. Screenshot of what this looks like: https://screenshot.googleplex.com/QdPCdt9DuZp
I assume you have run the image files through tools/resources/optimize-png-files.sh? https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:87: private static final int VR_CORE_MIN_VERSION = 160723800; // recent: 160722870,160723800 What does that comment mean? https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2747: install According to https://material.io/guidelines/components/buttons.html#buttons-style, text on buttons should be capitalized (even if the button happens to render the text in all-caps).
https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:87: private static final int VR_CORE_MIN_VERSION = 160723800; // recent: 160722870,160723800 On 2017/02/16 12:12:51, Bernhard Bauer wrote: > What does that comment mean? These were recent versions of the package. I still need to verify which one is the correct one to use, but thanks for reminding me to update this. https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2747: install On 2017/02/16 12:12:51, Bernhard Bauer wrote: > According to > https://material.io/guidelines/components/buttons.html#buttons-style, text on > buttons should be capitalized (even if the button happens to render the text in > all-caps). I wondered if there might be some rule like that. Thanks for the clarification. So I'll go ahead and make 4 entries, 2 for the lower case used in the infobar text, and 2 for the buttons which will be the same thing, just upper case. Is that the right way to do something like this?
On 2017/02/16 12:12:51, Bernhard Bauer wrote: > I assume you have run the image files through > tools/resources/optimize-png-files.sh? > Nope, I'm trying to do that now (requires installing a lot of things). The files came from the shared folder for infobar exported icon assets.
lgtm
On 2017/02/16 18:54:53, amp wrote: > On 2017/02/16 12:12:51, Bernhard Bauer wrote: > > I assume you have run the image files through > > tools/resources/optimize-png-files.sh? > > > Nope, I'm trying to do that now (requires installing a lot of things). The > files came from the shared folder for infobar exported icon assets. I wasn't able to get this working on my machine. How critical is it to have optimized the png files?
PTAL There are some flows which are blocked by http://crbug.com/693298 but those can be addressed in follow up changes once that is fixed. https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:87: private static final int VR_CORE_MIN_VERSION = 160723800; // recent: 160722870,160723800 On 2017/02/16 18:41:35, amp wrote: > On 2017/02/16 12:12:51, Bernhard Bauer wrote: > > What does that comment mean? > > These were recent versions of the package. I still need to verify which one is > the correct one to use, but thanks for reminding me to update this. Done. https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2747: install On 2017/02/16 18:41:35, amp wrote: > On 2017/02/16 12:12:51, Bernhard Bauer wrote: > > According to > > https://material.io/guidelines/components/buttons.html#buttons-style, text on > > buttons should be capitalized (even if the button happens to render the text > in > > all-caps). > > I wondered if there might be some rule like that. Thanks for the clarification. > > So I'll go ahead and make 4 entries, 2 for the lower case used in the infobar > text, and 2 for the buttons which will be the same thing, just upper case. Is > that the right way to do something like this? Done.
The CQ bit was checked by amp@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...
amp@chromium.org changed reviewers: + pkasting@chromium.org
+pkasting please review components/infobars/core/infobar_delegate.h and any infobar related use
LGTM https://codereview.chromium.org/2699643003/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2699643003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2743: <message name="IDS_VR_SERVICES_CHECK_INFOBAR_TEXT" desc="Text to displayed in the VR Services check infobar."> Nit: What is the "VR services check infobar"? Make sure to write this message text for a translator who knows nothing about the product and can't see screenshots, so it has to give context of what the user is doing and what the infobar is trying to ask them to do. https://codereview.chromium.org/2699643003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2746: <message name="IDS_VR_SERVICES_CHECK_INFOBAR_INSTALL_ACTION" desc="Text to be displayed in the VR Services check infobar text for the install action"> Nit: It wasn't immediately clear to me that this gets substituted into the sentence above, or what the conditions are when users would see this text versus the below text. Make sure to spell out these details clearly. https://codereview.chromium.org/2699643003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2752: <message name="IDS_VR_SERVICES_CHECK_INFOBAR_INSTALL_BUTTON" desc="Text to be displayed in the VR Services check infobar confirm button for the install action"> Nit: If this needs to follow a certain capitalization scheme or something, that needs to be stated explicitly here. https://codereview.chromium.org/2699643003/diff/60001/components/infobars/cor... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2699643003/diff/60001/components/infobars/cor... components/infobars/core/infobar_delegate.h:148: VR_SERVICES_UPGRADE = 74, Nit: This should be VR_SERVICES_UPGRADE_ANDROID, and similarly in the histogram.
On 2017/02/17 01:48:49, amp wrote: > On 2017/02/16 18:54:53, amp wrote: > > On 2017/02/16 12:12:51, Bernhard Bauer wrote: > > > I assume you have run the image files through > > > tools/resources/optimize-png-files.sh? > > > > > Nope, I'm trying to do that now (requires installing a lot of things). The > > files came from the shared folder for infobar exported icon assets. > > I wasn't able to get this working on my machine. How critical is it to have > optimized the png files? mtheisse@ was able to update them. They should be good in the latest patch.
https://codereview.chromium.org/2699643003/diff/60001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2699643003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2743: <message name="IDS_VR_SERVICES_CHECK_INFOBAR_TEXT" desc="Text to displayed in the VR Services check infobar."> On 2017/02/17 02:34:49, Peter Kasting wrote: > Nit: What is the "VR services check infobar"? Make sure to write this message > text for a translator who knows nothing about the product and can't see > screenshots, so it has to give context of what the user is doing and what the > infobar is trying to ask them to do. Done. https://codereview.chromium.org/2699643003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2746: <message name="IDS_VR_SERVICES_CHECK_INFOBAR_INSTALL_ACTION" desc="Text to be displayed in the VR Services check infobar text for the install action"> On 2017/02/17 02:34:49, Peter Kasting wrote: > Nit: It wasn't immediately clear to me that this gets substituted into the > sentence above, or what the conditions are when users would see this text versus > the below text. Make sure to spell out these details clearly. Done. https://codereview.chromium.org/2699643003/diff/60001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2752: <message name="IDS_VR_SERVICES_CHECK_INFOBAR_INSTALL_BUTTON" desc="Text to be displayed in the VR Services check infobar confirm button for the install action"> On 2017/02/17 02:34:49, Peter Kasting wrote: > Nit: If this needs to follow a certain capitalization scheme or something, that > needs to be stated explicitly here. I initially just had two messages here, but per bauerb@ and https://material.io/guidelines/components/buttons.html#buttons-style they are supposed to be capitalized for all translations. I added a reference in the description. https://codereview.chromium.org/2699643003/diff/60001/components/infobars/cor... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2699643003/diff/60001/components/infobars/cor... components/infobars/core/infobar_delegate.h:148: VR_SERVICES_UPGRADE = 74, On 2017/02/17 02:34:49, Peter Kasting wrote: > Nit: This should be VR_SERVICES_UPGRADE_ANDROID, and similarly in the histogram. The check is android only at this point, but we will eventually need to support a similar check on other platforms as well. It doesn't look easy to rename these enums either. I'll stick with the current name unless you feel strongly about it.
https://codereview.chromium.org/2699643003/diff/60001/components/infobars/cor... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2699643003/diff/60001/components/infobars/cor... components/infobars/core/infobar_delegate.h:148: VR_SERVICES_UPGRADE = 74, On 2017/02/17 02:52:29, amp wrote: > On 2017/02/17 02:34:49, Peter Kasting wrote: > > Nit: This should be VR_SERVICES_UPGRADE_ANDROID, and similarly in the > histogram. > > The check is android only at this point, but we will eventually need to support > a similar check on other platforms as well. It doesn't look easy to rename > these enums either. Oh? You just change the name and the histogram name. As long as you're not reordering, that's all that's necessary. I'd change it, since it's not clear when that other check will happen and if it will be implemented using an infobar.
https://codereview.chromium.org/2699643003/diff/60001/components/infobars/cor... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/2699643003/diff/60001/components/infobars/cor... components/infobars/core/infobar_delegate.h:148: VR_SERVICES_UPGRADE = 74, On 2017/02/17 02:55:52, Peter Kasting wrote: > On 2017/02/17 02:52:29, amp wrote: > > On 2017/02/17 02:34:49, Peter Kasting wrote: > > > Nit: This should be VR_SERVICES_UPGRADE_ANDROID, and similarly in the > > histogram. > > > > The check is android only at this point, but we will eventually need to > support > > a similar check on other platforms as well. It doesn't look easy to rename > > these enums either. > > Oh? You just change the name and the histogram name. As long as you're not > reordering, that's all that's necessary. > > I'd change it, since it's not clear when that other check will happen and if it > will be implemented using an infobar. Done. Doh, here I was thinking they were hard coded strings even though they have the int reference right next to them... cue cube of shame meme...
lgtm https://codereview.chromium.org/2699643003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2699643003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:161: int vrCoreVersion = PackageUtils.getPackageVersion(mActivity, VR_CORE_PACKAGE_ID); Is there any reason not to adapt this function for our purpose? https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
The CQ bit was checked by amp@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...
https://codereview.chromium.org/2699643003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2699643003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:161: int vrCoreVersion = PackageUtils.getPackageVersion(mActivity, VR_CORE_PACKAGE_ID); On 2017/02/17 13:59:01, bshe wrote: > Is there any reason not to adapt this function for our purpose? > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... I wasn't sure how the other one could tell the difference between not installed vs out of date. I think we can combine them, but I'm not sure how and if it needs to be in this change. WDYT?
https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2747: install On 2017/02/16 18:41:35, amp wrote: > On 2017/02/16 12:12:51, Bernhard Bauer wrote: > > According to > > https://material.io/guidelines/components/buttons.html#buttons-style, text on > > buttons should be capitalized (even if the button happens to render the text > in > > all-caps). > > I wondered if there might be some rule like that. Thanks for the clarification. > > So I'll go ahead and make 4 entries, 2 for the lower case used in the infobar > text, and 2 for the buttons which will be the same thing, just upper case. Is > that the right way to do something like this? No, you don't need the full capitalization for the button, as the UI will do that. What I would do however is split the IDS_VR_SERVICES_CHECK_INFOBAR_TEXT pattern up into two full strings -- it's only marginally bigger (which might be compressed away anyway) and reduces the chance for translation mistakes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/02/17 16:14:16, Bernhard Bauer wrote: > https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/str... > File chrome/android/java/strings/android_chrome_strings.grd (right): > > https://codereview.chromium.org/2699643003/diff/40001/chrome/android/java/str... > chrome/android/java/strings/android_chrome_strings.grd:2747: install > On 2017/02/16 18:41:35, amp wrote: > > On 2017/02/16 12:12:51, Bernhard Bauer wrote: > > > According to > > > https://material.io/guidelines/components/buttons.html#buttons-style, text > on > > > buttons should be capitalized (even if the button happens to render the text > > in > > > all-caps). > > > > I wondered if there might be some rule like that. Thanks for the > clarification. > > > > So I'll go ahead and make 4 entries, 2 for the lower case used in the infobar > > text, and 2 for the buttons which will be the same thing, just upper case. Is > > that the right way to do something like this? > > No, you don't need the full capitalization for the button, as the UI will do > that. What I would do however is split the IDS_VR_SERVICES_CHECK_INFOBAR_TEXT > pattern up into two full strings -- it's only marginally bigger (which might be > compressed away anyway) and reduces the chance for translation mistakes. Done. It does look easier to translate this way. Thanks!
The CQ bit was checked by amp@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...
lgtm
lgtm https://codereview.chromium.org/2699643003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2699643003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:161: int vrCoreVersion = PackageUtils.getPackageVersion(mActivity, VR_CORE_PACKAGE_ID); On 2017/02/17 16:13:04, amp wrote: > On 2017/02/17 13:59:01, bshe wrote: > > Is there any reason not to adapt this function for our purpose? > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > I wasn't sure how the other one could tell the difference between not installed > vs out of date. I think we can combine them, but I'm not sure how and if it > needs to be in this change. WDYT? The api throws an exception if Vr service isn't installed. Anyhow, we can chat offline on how to reuse some of the logics in a follow up CL.
The CQ bit was unchecked by amp@chromium.org
The CQ bit was checked by amp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2699643003/#ps160001 (title: "update text resources, fix naming mismatches")
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": 160001, "attempt_start_ts": 1487359887258850,
"parent_rev": "2d2154016e7698707f9cafd55efe508948fd170c", "commit_rev":
"c4911c48c072e710d9948a470936030ccb105945"}
Message was sent while issue was closed.
Description was changed from ========== Show an infobar when VR services need to be installed or updated. BUG=670166 ========== to ========== Show an infobar when VR services need to be installed or updated. BUG=670166 Review-Url: https://codereview.chromium.org/2699643003 Cr-Commit-Position: refs/heads/master@{#451372} Committed: https://chromium.googlesource.com/chromium/src/+/c4911c48c072e710d9948a470936... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/c4911c48c072e710d9948a470936... |
