Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2500283003:
Allow the VR omnibox to be transient. (Closed)
DescriptionAllow the VR omnibox to be transient.
Let the omnibox appear when its content changes, show for a few seconds,
then fade away. Any changes to page loading state or the URL extend the
time for which the box stays visible.
This fading behaviour can be disabled by setting the timeout to 0.
Currently, CSS transitions are used to hide and drop the omnibox when it
disappears. These transitions may appear choppy when content is
loading. Hence, this change may be a stepping stone towards a
native-driven transition.
BUG=641508
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/dc4752355449dc6452ad51d45f534f633abad7f8
Cr-Commit-Position: refs/heads/master@{#433008}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rename timer method. #
Total comments: 9
Patch Set 3 : Address some nits; explain others. #
Total comments: 2
Messages
Total messages: 15 (4 generated)
Description was changed from ========== Allow the VR omnibox to be transient. Let the omnibox appear when its content changes, show for a few seconds, then fade away. Any changes to page loading state or the URL extend the time for which the box stays visible. This fading behaviour can be disabled by setting the timeout to 0. Currently, CSS transitions are used to hide and drop the omnibox when it disappears. These transitions may appear choppy when content is loading. Hence, this change may be a stepping stone towards a native-driven transition. BUG=641508 ========== to ========== Allow the VR omnibox to be transient. Let the omnibox appear when its content changes, show for a few seconds, then fade away. Any changes to page loading state or the URL extend the time for which the box stays visible. This fading behaviour can be disabled by setting the timeout to 0. Currently, CSS transitions are used to hide and drop the omnibox when it disappears. These transitions may appear choppy when content is loading. Hence, this change may be a stepping stone towards a native-driven transition. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
cjgrant@chromium.org changed reviewers: + bajones@chromium.org, bshe@chromium.org, klausw@chromium.org, mthiesse@chromium.org
Brandon and Klaus, this read-only omnibox is currently hidden in WebVR mode, but at this point, it's easy to enable when we're ready.
https://codereview.chromium.org/2500283003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2500283003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:301: kickVisibilityTimer() { nit: resetVisibilityTimer()? https://codereview.chromium.org/2500283003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:323: updateState() { Why have an enabled boolean on top of the NativeVisibility state? Why not set NativeVisibility directly, rather than updating this.enabled? Also, this function seems like it's doing too many things. Why not have an startTimer, resetTimer, and onTimerEnd as separate functions?
https://codereview.chromium.org/2500283003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2500283003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:301: kickVisibilityTimer() { On 2016/11/15 19:42:48, mthiesse wrote: > nit: resetVisibilityTimer()? Done. https://codereview.chromium.org/2500283003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:323: updateState() { On 2016/11/15 19:42:48, mthiesse wrote: > Why have an enabled boolean on top of the NativeVisibility state? Why not set > NativeVisibility directly, rather than updating this.enabled? this.enabled -> Whether or not there's an omnibox in the scene. nativeVisibility -> Whether or not the omnibox is visible at the moment, subject to idle timeout. > Also, this function seems like it's doing too many things. Why not have an > startTimer, resetTimer, and onTimerEnd as separate functions? Several things change the omnibox appearance/content, only one of which is a timer expiry or reset. I prefer to keep the "state to appearance" logic as centralized as possible, even if it makes for a bigger function. Some docs could help here, but probably not much.
lgtm with nits https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:259: this.visibilityTimeout = VISIBILITY_TIMEOUT_MS; did you change the value of visibilityTimeout? if not, perhaps use VISIBILITY_TIMEOUT_MS directly. https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:275: show(enabled) { nit: It is possibly easier to understand if you change this to show/hide pair https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:282: this.loading = loading; Should all the functions that might change the state of ominibox also enable omnibox? Maybe call this.show(true)? https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:336: if (this.visibilityTimeout > 0 && !this.visibilityTimer) { Doesn't look like you changed the value of visibilityTimeout, so "this.visibilityTimeout > 0" always return true? https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:348: if (visible != this.nativeState.visible) { nit: prefer early return.
https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:259: this.visibilityTimeout = VISIBILITY_TIMEOUT_MS; On 2016/11/17 15:18:19, bshe wrote: > did you change the value of visibilityTimeout? if not, perhaps use > VISIBILITY_TIMEOUT_MS directly. Changing the timeout to 0 disables the transient behaviour. I didn't add a setter for this yet, but I'd like to leave it build this way so it's easy to tweak later. https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:275: show(enabled) { On 2016/11/17 15:18:19, bshe wrote: > nit: It is possibly easier to understand if you change this to show/hide pair I think show/hide will generate more duplicate code, but for consistency with other members (and the other classes in this module), I'll change show() to setEnabled(). https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:282: this.loading = loading; On 2016/11/17 15:18:19, bshe wrote: > Should all the functions that might change the state of ominibox also enable > omnibox? Maybe call this.show(true)? I don't want that to happen, no. If the omnibox is disabled, the only thing that should enable it is an explicit call to do so. The reason I still handle setLoading(), setURL(), etc., is to keep the state current so that if enabled, the omnibox has current information to display. Let me know if you don't like this approach. https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:336: if (this.visibilityTimeout > 0 && !this.visibilityTimer) { On 2016/11/17 15:18:19, bshe wrote: > Doesn't look like you changed the value of visibilityTimeout, so > "this.visibilityTimeout > 0" always return true? As above, setting a non-zero timeout enables the transient behaviour. I'll just add the setter now for clarity.
On 2016/11/17 20:17:43, cjgrant wrote: > https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... > File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): > > https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... > chrome/browser/resources/vr_shell/vr_shell_ui.js:259: this.visibilityTimeout = > VISIBILITY_TIMEOUT_MS; > On 2016/11/17 15:18:19, bshe wrote: > > did you change the value of visibilityTimeout? if not, perhaps use > > VISIBILITY_TIMEOUT_MS directly. > > Changing the timeout to 0 disables the transient behaviour. I didn't add a > setter for this yet, but I'd like to leave it build this way so it's easy to > tweak later. > > https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... > chrome/browser/resources/vr_shell/vr_shell_ui.js:275: show(enabled) { > On 2016/11/17 15:18:19, bshe wrote: > > nit: It is possibly easier to understand if you change this to show/hide pair > > I think show/hide will generate more duplicate code, but for consistency with > other members (and the other classes in this module), I'll change show() to > setEnabled(). > > https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... > chrome/browser/resources/vr_shell/vr_shell_ui.js:282: this.loading = loading; > On 2016/11/17 15:18:19, bshe wrote: > > Should all the functions that might change the state of ominibox also enable > > omnibox? Maybe call this.show(true)? > > I don't want that to happen, no. If the omnibox is disabled, the only thing > that should enable it is an explicit call to do so. The reason I still handle > setLoading(), setURL(), etc., is to keep the state current so that if enabled, > the omnibox has current information to display. Let me know if you don't like > this approach. > > https://codereview.chromium.org/2500283003/diff/20001/chrome/browser/resource... > chrome/browser/resources/vr_shell/vr_shell_ui.js:336: if (this.visibilityTimeout > > 0 && !this.visibilityTimer) { > On 2016/11/17 15:18:19, bshe wrote: > > Doesn't look like you changed the value of visibilityTimeout, so > > "this.visibilityTimeout > 0" always return true? > > As above, setting a non-zero timeout enables the transient behaviour. I'll just > add the setter now for clarity. lgtm
The CQ bit was checked by cjgrant@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2500283003/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2500283003/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:254: /** @const */ var VISIBILITY_TIMEOUT_MS = 3000; Not affecting the review (which looks like it's already being submitted), but does using 'const' here (instead of 'let' or 'var') not work? I saw this in a few other places (as I'm starting to implement cinema mode) and was wondering if there was a reason for marking these with the const comment but then using var.
https://codereview.chromium.org/2500283003/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2500283003/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:254: /** @const */ var VISIBILITY_TIMEOUT_MS = 3000; On 2016/11/17 21:00:35, amp wrote: > Not affecting the review (which looks like it's already being submitted), but > does using 'const' here (instead of 'let' or 'var') not work? > > I saw this in a few other places (as I'm starting to implement cinema mode) and > was wondering if there was a reason for marking these with the const comment but > then using var. It keeps the presubmit style checker happy - no other reason. I originally used 'const', until the presubmit checker yelled at me. I think the style used here is a bit gross looking, but it's consistent with code elsewhere in the codebase.
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Allow the VR omnibox to be transient. Let the omnibox appear when its content changes, show for a few seconds, then fade away. Any changes to page loading state or the URL extend the time for which the box stays visible. This fading behaviour can be disabled by setting the timeout to 0. Currently, CSS transitions are used to hide and drop the omnibox when it disappears. These transitions may appear choppy when content is loading. Hence, this change may be a stepping stone towards a native-driven transition. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Allow the VR omnibox to be transient. Let the omnibox appear when its content changes, show for a few seconds, then fade away. Any changes to page loading state or the URL extend the time for which the box stays visible. This fading behaviour can be disabled by setting the timeout to 0. Currently, CSS transitions are used to hide and drop the omnibox when it disappears. These transitions may appear choppy when content is loading. Hence, this change may be a stepping stone towards a native-driven transition. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/dc4752355449dc6452ad51d45f534f633abad7f8 Cr-Commit-Position: refs/heads/master@{#433008} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/dc4752355449dc6452ad51d45f534f633abad7f8 Cr-Commit-Position: refs/heads/master@{#433008} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
