Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2891423002:
Disable NTP URL bar while in VR on Android. (Closed)
|
Created:
3 years, 7 months ago by mthiesse Modified:
3 years, 6 months ago CC:
chromium-reviews, jdonnelly+watch_chromium.org, feature-vr-reviews_chromium.org, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org, asimjour1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable NTP URL bar while in VR on Android.
Unfortunately we don't support keyboard or microphone input while in VR just yet, so we need to disable them to prevent the 2D popups from obscuring VR content.
BUG=724551
Review-Url: https://codereview.chromium.org/2891423002
Cr-Commit-Position: refs/heads/master@{#476779}
Committed: https://chromium.googlesource.com/chromium/src/+/c8c17ea5f62b4d647f2cb4e892333927d8e37779
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : javadoc #
Total comments: 3
Patch Set 4 : Make focusSearchBox aware of VR #Messages
Total messages: 18 (8 generated)
Description was changed from ========== Disable NTP URL bar while in VR on Android. Unfortunately we don't support keyboard or microphone input while in VR just yet, so we need to disable them to prevent the 2D popups from obscuring VR content. BUG= ========== to ========== Disable NTP URL bar while in VR on Android. Unfortunately we don't support keyboard or microphone input while in VR just yet, so we need to disable them to prevent the 2D popups from obscuring VR content. BUG=724551 ==========
mthiesse@chromium.org changed reviewers: + asimjour@chromium.org, tedchoc@chromium.org, ymalik@chromium.org
ymalik, asimjour: Please review VrShellImpl changes. tedchoc: Please review omnibox/ and ntp/ changes.
tedchoc@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb for ntp thoughts https://codereview.chromium.org/2891423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2891423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:433: mSearchProviderHasLogo = TemplateUrlService.getInstance().isDefaultSearchEngineGoogle(); Is it better UI if VR would to fall into this path? Set your search engine to something other than google and see if all the UI that you can't interact with is hidden. On a tangent, I wonder if we should try to converge on something like VrTabHelper in Java as well where here we would have something like VrTabHelper.isInVr(webContents()). The gotcha is that does add VR specific stuff else where, but maybe that is better in terms of consistency.
Amir or Yash, feel free to pick this CL up from me while I'm out this week. Otherwise I'll get back to this on Monday.
https://codereview.chromium.org/2891423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2891423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:433: mSearchProviderHasLogo = TemplateUrlService.getInstance().isDefaultSearchEngineGoogle(); On 2017/05/23 14:37:45, Ted C wrote: > Is it better UI if VR would to fall into this path? Set your search engine to > something other than google and see if all the UI that you can't interact with > is hidden. > > On a tangent, I wonder if we should try to converge on something like > VrTabHelper in Java as well where here we would have something like > VrTabHelper.isInVr(webContents()). The gotcha is that does add VR specific > stuff else where, but maybe that is better in terms of consistency. Hmm I don't personally think the UI is better when we change the search engine. Mainly I like the consistency of the NTP looking the same in and out of VR, and the Google Doodle is great (the NTP looks so barren without it). It might look better if the NTP made it visually apparent that URL bar input was disabled, but I think that's very non-urgent. The search input field also being present/selectable without the keyboard popping up is consistent with the rest of VR at the moment. As for VrTabHelper.isInVr(webContents()), I don't really see the point of this here. We already have VrShellDelegate.isInVr(), and I think from the browser's perspective we're in VR or we aren't (at least on Android). You can't have some WebContents in VR and some not. I'd be okay with using VrShellDelegate.isInVr(), but we would still have to either manually call some update function, or provide a way for listeners to listen to VR-ness changing. Is this something we want? VrShellDelegate.registerVrStateListener()?
ping all :)
https://codereview.chromium.org/2891423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2891423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:161: void setUrlBarFocusable(boolean focusable); I feel there needs to be a path where this is isolated entirely within the NTP package. We shouldn't need to disable the omnibox as that view is hidden when entering VR. You can change focusSearchBox to return within the NewTabPage.java. You can set the fake box to invisible, you can disable it and the voice button.
Ah, thanks for the pointer to focusSearchBox. I think just returning early there is more than good enough for M60. We can worry about actually hiding the fakebox later. PTAL
mthiesse@chromium.org changed reviewers: - ymalik@chromium.org
mthiesse@chromium.org changed reviewers: - asimjour@chromium.org
lgtm
The CQ bit was checked by mthiesse@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": 60001, "attempt_start_ts": 1496430291580980,
"parent_rev": "94332b32bc6f35884616622584fff459e6a80830", "commit_rev":
"c8c17ea5f62b4d647f2cb4e892333927d8e37779"}
Message was sent while issue was closed.
Description was changed from ========== Disable NTP URL bar while in VR on Android. Unfortunately we don't support keyboard or microphone input while in VR just yet, so we need to disable them to prevent the 2D popups from obscuring VR content. BUG=724551 ========== to ========== Disable NTP URL bar while in VR on Android. Unfortunately we don't support keyboard or microphone input while in VR just yet, so we need to disable them to prevent the 2D popups from obscuring VR content. BUG=724551 Review-Url: https://codereview.chromium.org/2891423002 Cr-Commit-Position: refs/heads/master@{#476779} Committed: https://chromium.googlesource.com/chromium/src/+/c8c17ea5f62b4d647f2cb4e89233... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c8c17ea5f62b4d647f2cb4e89233... |
