Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2435623003:
Implement (but do not enable) HTML UI-driven WebVR security warnings. (Closed)
DescriptionImplement (but do not enable) HTML UI-driven WebVR security warnings.
These will be enabled in a follow-on change that adjusts VR shell to
render UI elements whether or not we're in WebVR mode.
BUG=641508
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/07570419278e71a3bdaf60b581b18895d13942a7
Cr-Commit-Position: refs/heads/master@{#426897}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Refactor warnings into a class; constify constants. #Patch Set 3 : Address more comments. #
Total comments: 2
Patch Set 4 : Fix CONST_CASE #Messages
Total messages: 15 (5 generated)
Description was changed from ========== Implement (but do not enable) HTML UI-driven WebVR security warnings. These will be enabled in a follow-on change that adjusts VR shell to render UI elements whether or not we're in WebVR mode. BUG=641508 ========== to ========== Implement (but do not enable) HTML UI-driven WebVR security warnings. These will be enabled in a follow-on change that adjusts VR shell to render UI elements whether or not we're in WebVR mode. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
cjgrant@chromium.org changed reviewers: + bshe@chromium.org, mthiesse@chromium.org
Michael, your previous advice to house menu buttons in a class could apply here too. Personally I'd rather let the UI grow before choosing the best way to modularize it, but as always, opinions welcome.
Michael, your previous advice to house menu buttons in a class could apply here too. Personally I'd rather let the UI grow before choosing the best way to modularize it, but as always, opinions welcome.
lgtm lgtm % nits https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (left): https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:64: class UiState { No preference for where or when you do this, but we should call this class Scene, or SceneManger to be less confusing. https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:163: this.secureOrigin = secure; nit: isSecureOrigin
https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:72: var angleUp = 16.3 * Math.PI / 180.0; Should the above two variables be a const? Similar to this style: /** @const */ var DISTANCE = 0.7; https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:148: this.mode = mode; This got me wonder which will be called first from native side? setMode or setSecureOrigin? Should we consider wait until we know the state of origin and mode before try to update? https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:155: this.secureOriginTimer = setTimeout( Do you need to clear the previous timer if not fired? It is probably fine right now. But once we support exiting webvr fullscreen in vr shell. The timer for previous content might fire if the user navigate to a new insecure site. https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:173: var visible = (this.mode == 1 && !this.secureOrigin); nit: prefer to use an enum instead of 1.
https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (left): https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:64: class UiState { On 2016/10/19 20:11:56, mthiesse wrote: > No preference for where or when you do this, but we should call this class > Scene, or SceneManger to be less confusing. Done. https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:72: var angleUp = 16.3 * Math.PI / 180.0; On 2016/10/20 18:01:53, bshe wrote: > Should the above two variables be a const? Similar to this style: > /** @const */ var DISTANCE = 0.7; Done (with additional refactoring). https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:148: this.mode = mode; On 2016/10/20 18:01:53, bshe wrote: > This got me wonder which will be called first from native side? setMode or > setSecureOrigin? Should we consider wait until we know the state of origin and > mode before try to update? In 2D mode, we don't get a call to VrShell to set mode. Presumably we only get a call when switching to WebVR or back. In the WebVR case, fortunately, we get the webVR mode change before the HTML UI is ready, so our default of 2D mode is overridden for WebVR before the UI starts up. https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:155: this.secureOriginTimer = setTimeout( On 2016/10/20 18:01:53, bshe wrote: > Do you need to clear the previous timer if not fired? It is probably fine right > now. But once we support exiting webvr fullscreen in vr shell. The timer for > previous content might fire if the user navigate to a new insecure site. Done. Good catch. https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:163: this.secureOrigin = secure; On 2016/10/19 20:11:56, mthiesse wrote: > nit: isSecureOrigin Done. https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:173: var visible = (this.mode == 1 && !this.secureOrigin); On 2016/10/20 18:01:53, bshe wrote: > nit: prefer to use an enum instead of 1. Done.
lgtm with nits https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2435623003/diff/1/chrome/browser/resources/vr... chrome/browser/resources/vr_shell/vr_shell_ui.js:148: this.mode = mode; On 2016/10/21 17:40:48, cjgrant wrote: > On 2016/10/20 18:01:53, bshe wrote: > > This got me wonder which will be called first from native side? setMode or > > setSecureOrigin? Should we consider wait until we know the state of origin and > > mode before try to update? > > In 2D mode, we don't get a call to VrShell to set mode. Presumably we only get > a call when switching to WebVR or back. In the WebVR case, fortunately, we get > the webVR mode change before the HTML UI is ready, so our default of 2D mode is > overridden for WebVR before the UI starts up. This can be in a separate CL if it make sense. But would it make sense to update |mode| and |secure| in one call to javascript? This way we don't need to worry about order. https://codereview.chromium.org/2435623003/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2435623003/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:67: /** @const */ var descriptors = [ nit: const name should be CONSTANT_CASE here and all other places.
As discussed offline, lets chat about the Javascript command details on Monday in-person. https://codereview.chromium.org/2435623003/diff/40001/chrome/browser/resource... File chrome/browser/resources/vr_shell/vr_shell_ui.js (right): https://codereview.chromium.org/2435623003/diff/40001/chrome/browser/resource... chrome/browser/resources/vr_shell/vr_shell_ui.js:67: /** @const */ var descriptors = [ On 2016/10/21 18:10:47, bshe wrote: > nit: const name should be CONSTANT_CASE > here and all other places. Done.
The CQ bit was checked by cjgrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2435623003/#ps60001 (title: "Fix CONST_CASE")
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Implement (but do not enable) HTML UI-driven WebVR security warnings. These will be enabled in a follow-on change that adjusts VR shell to render UI elements whether or not we're in WebVR mode. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implement (but do not enable) HTML UI-driven WebVR security warnings. These will be enabled in a follow-on change that adjusts VR shell to render UI elements whether or not we're in WebVR mode. BUG=641508 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/07570419278e71a3bdaf60b581b18895d13942a7 Cr-Commit-Position: refs/heads/master@{#426897} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/07570419278e71a3bdaf60b581b18895d13942a7 Cr-Commit-Position: refs/heads/master@{#426897} |
