Exiting menu mode when content preview is clicked

Exiting menu mode when content preview is clicked

An extra invisible quad added on top of the content quad, which is only enabled
while in menu mode and when clicked exits to normal mode

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

Review-Url: https://codereview.chromium.org/2740013003
Cr-Commit-Position: refs/heads/master@{#456070}
Committed: https://chromium.googlesource.com/chromium/src/+/8eb622992e35db417300399e399c7d0fbe17fee7

[acondor_, 2017-03-09 17:11:38]

Description was changed from

==========
Exiting menu mode when content preview is clicked

An extra invisible quad added on top of the content quad, which is only enabled
while in menu mode and when clicked exits to normal mode

BUG=692319
==========

to

==========
Exiting menu mode when content preview is clicked

An extra invisible quad added on top of the content quad, which is only enabled
while in menu mode and when clicked exits to normal mode

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

[acondor_, 2017-03-09 18:12:42]

acondor@google.com changed reviewers:
+ cjgrant@chromium.org, mthiesse@chromium.org

[acondor_, 2017-03-09 18:12:42]

Please review

[mthiesse, 2017-03-09 18:47:46]

lgtm. Please wait for Chris to review before committing though.

[cjgrant, 2017-03-09 20:03:01]

A few minor things, looks good otherwise.

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

https://codereview.chromium.org/2740013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:43: /** @const */
this.DOM_INTERCEPTOR_ELEVATION = 0.1;
10 cm seems big, since this quad is like a skin over the content.  Possibly 0.01
or 0.001?  I think we could visually see the cursor hover at 10 cm.

https://codereview.chromium.org/2740013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:67: // while on menu mode in
order to exit it.
optional: 'in order to exit it' - you could leave this out, as it's the kind of
detail that gets stale easily, and is clear in the code below.

https://codereview.chromium.org/2740013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:69: let interceptorUpdate = new
api.UiElementUpdate();
'let update' = is sufficient, or you could reuse a previous 'update' variable if
one exists.

https://codereview.chromium.org/2740013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:129: // show/hide the
interceptor.
Comments should always be full sentences, ie "// Show or hide the interceptor."
However, I don't think this comment is needed.  :)

https://codereview.chromium.org/2740013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:130: let interceptorUpdate =
new api.UiElementUpdate();
'let update' =  or  'update =' is cool here too (makes the code easier to read,
as we almost always call these objects 'update'.

[acondor_, 2017-03-09 20:40:38]

Ready :)

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

https://codereview.chromium.org/2740013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:43: /** @const */
this.DOM_INTERCEPTOR_ELEVATION = 0.1;
On 2017/03/09 20:03:00, cjgrant wrote:
> 10 cm seems big, since this quad is like a skin over the content.  Possibly
0.01
> or 0.001?  I think we could visually see the cursor hover at 10 cm.

Done.

https://codereview.chromium.org/2740013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:67: // while on menu mode in
order to exit it.
On 2017/03/09 20:03:00, cjgrant wrote:
> optional: 'in order to exit it' - you could leave this out, as it's the kind
of
> detail that gets stale easily, and is clear in the code below.

Done.

https://codereview.chromium.org/2740013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:69: let interceptorUpdate = new
api.UiElementUpdate();
On 2017/03/09 20:03:00, cjgrant wrote:
> 'let update' = is sufficient, or you could reuse a previous 'update' variable
if
> one exists.

Done.

https://codereview.chromium.org/2740013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:129: // show/hide the
interceptor.
On 2017/03/09 20:03:00, cjgrant wrote:
> Comments should always be full sentences, ie "// Show or hide the
interceptor."
> However, I don't think this comment is needed.  :)

Done.

https://codereview.chromium.org/2740013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:130: let interceptorUpdate =
new api.UiElementUpdate();
On 2017/03/09 20:03:00, cjgrant wrote:
> 'let update' =  or  'update =' is cool here too (makes the code easier to
read,
> as we almost always call these objects 'update'.

Done.

[cjgrant, 2017-03-09 22:21:03]

lgtm

[acondor_, 2017-03-10 01:07:46]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-03-10 01:07:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org
Link to the patchset: https://codereview.chromium.org/2740013003/#ps40001
(title: "Addressing suggestions on variable names")

[commit-bot: I haz the power, 2017-03-10 01:08:43]

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, 2017-03-10 01:12:19]

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

[commit-bot: I haz the power, 2017-03-10 01:12:20]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[acondor_, 2017-03-10 15:14:11]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-03-10 15:14:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, cjgrant@chromium.org
Link to the patchset: https://codereview.chromium.org/2740013003/#ps60001
(title: "Resolving conflict")

[commit-bot: I haz the power, 2017-03-10 15:14:29]

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, 2017-03-10 16:03:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489158851246580,
"parent_rev": "414ca9dc0a03a61b6c2ce1be65b2a2ab402f67ea", "commit_rev":
"8eb622992e35db417300399e399c7d0fbe17fee7"}

[commit-bot: I haz the power, 2017-03-10 16:04:01]

Description was changed from

==========
Exiting menu mode when content preview is clicked

An extra invisible quad added on top of the content quad, which is only enabled
while in menu mode and when clicked exits to normal mode

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

to

==========
Exiting menu mode when content preview is clicked

An extra invisible quad added on top of the content quad, which is only enabled
while in menu mode and when clicked exits to normal mode

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

Review-Url: https://codereview.chromium.org/2740013003
Cr-Commit-Position: refs/heads/master@{#456070}
Committed:
https://chromium.googlesource.com/chromium/src/+/8eb622992e35db417300399e399c...
==========

[commit-bot: I haz the power, 2017-03-10 16:04:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8eb622992e35db417300399e399c...