Control visibility of 2D VR menu buttons using native state.

Control visibility of 2D VR menu buttons using native state.

BUG=641508
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b3a203c74e4caa36d85c2ef70f680449fb295276
Cr-Commit-Position: refs/heads/master@{#426639}

[cjgrant, 2016-10-19 18:37:04]

Description was changed from

==========
Control visibility of 2D VR menu buttons using native state.

BUG=641508
==========

to

==========
Control visibility of 2D VR menu buttons using native state.

BUG=641508
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[cjgrant, 2016-10-19 18:38:35]

cjgrant@chromium.org changed reviewers:
+ bshe@chromium.org, mthiesse@chromium.org

[mthiesse, 2016-10-19 18:53:50]

lgtm % comment

https://codereview.chromium.org/2437823002/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2437823002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:71: createMenuButtons() {
Not to be super picky about this (I'm being super picky about this), but this is
a conceptually strange way to structure this. Why is createMenuButtons() a
function on UiState?

Up to you, but maybe a MenuButtons class would structure this better? With
functions like MenuButtons.show(), MenuButtons.hide()

[cjgrant, 2016-10-19 19:37:39]

https://codereview.chromium.org/2437823002/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2437823002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:71: createMenuButtons() {
On 2016/10/19 18:53:50, mthiesse wrote:
> Not to be super picky about this (I'm being super picky about this), but this
is
> a conceptually strange way to structure this. Why is createMenuButtons() a
> function on UiState?
> 
> Up to you, but maybe a MenuButtons class would structure this better? With
> functions like MenuButtons.show(), MenuButtons.hide()

Picky is good.  :)  I was minimizing the code changes WRT buttons, but you're
right that this is a good time to house them in a Menu class (as they're
changing owner already).  I'll change this.

[bshe, 2016-10-20 18:12:40]

lgtm with nit

https://codereview.chromium.org/2437823002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2437823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:116: this.mode = -1;
nit: perhaps create a invalid mode enum to replace -1 here?

[cjgrant, 2016-10-20 20:27:30]

https://codereview.chromium.org/2437823002/diff/1/chrome/browser/resources/vr...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2437823002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:71: createMenuButtons() {
On 2016/10/19 19:37:39, cjgrant wrote:
> On 2016/10/19 18:53:50, mthiesse wrote:
> > Not to be super picky about this (I'm being super picky about this), but
this
> is
> > a conceptually strange way to structure this. Why is createMenuButtons() a
> > function on UiState?
> > 
> > Up to you, but maybe a MenuButtons class would structure this better? With
> > functions like MenuButtons.show(), MenuButtons.hide()
> 
> Picky is good.  :)  I was minimizing the code changes WRT buttons, but you're
> right that this is a good time to house them in a Menu class (as they're
> changing owner already).  I'll change this.

Done.

https://codereview.chromium.org/2437823002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.js (right):

https://codereview.chromium.org/2437823002/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:116: this.mode = -1;
On 2016/10/20 18:12:40, bshe wrote:
> nit: perhaps create a invalid mode enum to replace -1 here?

Done.

[cjgrant, 2016-10-20 20:27:46]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2016-10-20 20:27:46]

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/2437823002/#ps40001
(title: "Address nits.")

[commit-bot: I haz the power, 2016-10-20 20:28:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 20:28:12]

This CL has an open dependency (Issue 2434013002 Patch 80001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[cjgrant, 2016-10-20 21:37:36]

The CQ bit was checked by cjgrant@chromium.org

[commit-bot: I haz the power, 2016-10-20 21:38:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 23:13:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:24:17]

Description was changed from

==========
Control visibility of 2D VR menu buttons using native state.

BUG=641508
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Control visibility of 2D VR menu buttons using native state.

BUG=641508
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b3a203c74e4caa36d85c2ef70f680449fb295276
Cr-Commit-Position: refs/heads/master@{#426639}
==========

[commit-bot: I haz the power, 2016-10-21 13:24:18]

Patchset 3 (id:??) landed as
https://crrev.com/b3a203c74e4caa36d85c2ef70f680449fb295276
Cr-Commit-Position: refs/heads/master@{#426639}