Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721203003:
Background correctly adapts to state in Chrome VR. (Closed)
DescriptionMake background correctly adapt to state in Chrome VR.
State consists of the mode (standard, WebVR), menu mode (yes, no) and fullscreen (yes, no).
BUG=697180
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2721203003
Cr-Commit-Position: refs/heads/master@{#454416}
Committed: https://chromium.googlesource.com/chromium/src/+/39ba5c3de7eb2dca22553ab2664c578d9c5a41ac
Patch Set 1 #
Total comments: 6
Patch Set 2 : Dark background for fullscreen menu mode #Messages
Total messages: 18 (8 generated)
Description was changed from ========== Background correctly adapts to state in Chrome VR. - State consists of the mode (standard, WebVR), menu mode (yes, no) and fullscreen (yes, no). BUG=697180 ========== to ========== Background correctly adapts to state in Chrome VR. - State consists of the mode (standard, WebVR), menu mode (yes, no) and fullscreen (yes, no). BUG=697180 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2721203003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2721203003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:615: setElementVisible(elementId, visible) { This method should probably be in some general utility class or in a dedicated UI element class. Due to the future ownership of this file I'm hesitant to do this refactor now.
tiborg@chromium.org changed reviewers: + cjgrant@chromium.org
tiborg@chromium.org changed reviewers: + mthiesse@chromium.org
https://codereview.chromium.org/2721203003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2721203003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:615: setElementVisible(elementId, visible) { On 2017/03/01 18:28:15, tiborg wrote: > This method should probably be in some general utility class or in a dedicated > UI element class. Due to the future ownership of this file I'm hesitant to do > this refactor now. Agreed. https://codereview.chromium.org/2721203003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:644: case api.Mode.STANDARD: I could be wrong, but this appears to be: if (menu) { Light } else if (webVr) { Hidden } else if (fullscreen) { Dark } else { Light } Is that simpler? Also, if in fullscreen, when we enter menu, do we want the menu to stay dark? https://codereview.chromium.org/2721203003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:644: case api.Mode.STANDARD: For another change (possibly by me) - Mode used to have more options, but now it's WebVR or not. I think we should return this to a boolean. There may be some extra bit needed to make sure that we don't draw one or the other, before we know which mode we're in initially.
Also, a few nits on the change description to set habits: - Should be a blank line between first line and others (see Contributing Code doc). - First line should be the action taken, not the result. Here, it'd be "Make background correctly adapt to WebVR state", or something similar. "Make ...", "fix ...", "add ...", etc. - The bulleted list has only one item, so it doesn't have to be a list.
Description was changed from ========== Background correctly adapts to state in Chrome VR. - State consists of the mode (standard, WebVR), menu mode (yes, no) and fullscreen (yes, no). BUG=697180 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Makes background correctly adapt to state in Chrome VR. State consists of the mode (standard, WebVR), menu mode (yes, no) and fullscreen (yes, no). BUG=697180 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2721203003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2721203003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:644: case api.Mode.STANDARD: On 2017/03/01 22:30:48, cjgrant wrote: > I could be wrong, but this appears to be: > > if (menu) { > Light > } else if (webVr) { > Hidden > } else if (fullscreen) { > Dark > } else { > Light > } > > Is that simpler? Also, if in fullscreen, when we enter menu, do we want the > menu to stay dark? I played around with it a bit and I agree that the background should stay dark for menu mode in fullscreen so that it's not so disruptive when you quickly go to menu mode and back. Changed it accordingly. https://codereview.chromium.org/2721203003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:644: case api.Mode.STANDARD: On 2017/03/01 22:30:48, cjgrant wrote: > For another change (possibly by me) - Mode used to have more options, but now > it's WebVR or not. I think we should return this to a boolean. There may be > some extra bit needed to make sure that we don't draw one or the other, before > we know which mode we're in initially. Regarding the mode boolean: sounds good to me.
lgtm
Description was changed from ========== Makes background correctly adapt to state in Chrome VR. State consists of the mode (standard, WebVR), menu mode (yes, no) and fullscreen (yes, no). BUG=697180 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make background correctly adapt to state in Chrome VR. State consists of the mode (standard, WebVR), menu mode (yes, no) and fullscreen (yes, no). BUG=697180 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tiborg@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": 20001, "attempt_start_ts": 1488491063947440,
"parent_rev": "0c90f31b11fd24f325c2621c3e11ece1ccc8f9a0", "commit_rev":
"39ba5c3de7eb2dca22553ab2664c578d9c5a41ac"}
Message was sent while issue was closed.
Description was changed from ========== Make background correctly adapt to state in Chrome VR. State consists of the mode (standard, WebVR), menu mode (yes, no) and fullscreen (yes, no). BUG=697180 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make background correctly adapt to state in Chrome VR. State consists of the mode (standard, WebVR), menu mode (yes, no) and fullscreen (yes, no). BUG=697180 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2721203003 Cr-Commit-Position: refs/heads/master@{#454416} Committed: https://chromium.googlesource.com/chromium/src/+/39ba5c3de7eb2dca22553ab2664c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/39ba5c3de7eb2dca22553ab2664c... |
