Let HTML UI manage menu mode.

Let HTML UI fully manage menu mode.

- Have native pass app button presses up to the UI.
- The UI decides whether or not to be in menu mode.
- The UI may call back down to native to pause content, if desired.

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

Review-Url: https://codereview.chromium.org/2664973002
Cr-Commit-Position: refs/heads/master@{#448017}
Committed: https://chromium.googlesource.com/chromium/src/+/cb6d69f2601372a0fbc8760fbb62c7f6e0cc3cd8

[cjgrant, 2017-01-30 21:46:29]

Description was changed from

==========
Let HTML UI fully manage menu mode.

- Have native pass app button presses up to the UI.
- The UI decides whether or not to be in menu mode.
- The UI may call back down to native to pause content, if desired.

BUG=644565
==========

to

==========
Let HTML UI fully manage menu mode.

- Have native pass app button presses up to the UI.
- The UI decides whether or not to be in menu mode.
- The UI may call back down to native to pause content, if desired.

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

[cjgrant, 2017-01-30 21:55:56]

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

[cjgrant, 2017-01-30 21:55:57]

There are a couple points for discussion in this CL.  Please see comments...

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:173: // TODO(mthiesse): The page is
no longer visible when in menu mode. We
This may need updating.  Conventional pages stay visible, but the quad shrinks. 
WebVR freezes the pose (etc), but is still visible.

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:177: if
(delegate_provider_->device_provider()) {
I removed a check here for "if in WebVR".  Do we really want to pass this state
only if in WebVR?  Seems safer to pass it over always.

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:244: html_interface_->SetMode(
Thus far our modes are "2d" and "webVR".  I wonder if we should make this a
boolean SetWebVRMode(), rather than use the mode enum.

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.h (right):

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.h:58: SET_CONTENT_PAUSED,
We need a consistent name for what the UI does to content in menu mode.  Pause? 
Blur?  Disable?

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.h:170: bool webvr_mode_ = false;
Reintroducing this member.  IMO, VR shell should not be asking the HTML UI
whether VR shell is in webVR mode.

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

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:574:
api.doAction(api.Action.SET_CONTENT_PAUSED, {'paused': this.menuMode});
On second thought, this should not be here, it should be called in
updateState().  App button clicks won't be the only way out of menu mode.

[asimjour1, 2017-01-31 15:34:00]

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:367: case SET_CONTENT_PAUSED: {
nit: please remove the brace

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

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:574:
api.doAction(api.Action.SET_CONTENT_PAUSED, {'paused': this.menuMode});
On 2017/01/30 21:55:57, cjgrant wrote:
> On second thought, this should not be here, it should be called in
> updateState().  App button clicks won't be the only way out of menu mode.

I agree.

[bshe, 2017-01-31 17:03:32]

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:175: if (content_paused_ != paused)
{
nit: prefer early return
e.g. if (content_paused_ ==  paused)
        return;

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

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:579: /** @const */ var
VISIBILITY_TIMEOUT_MS = 5000;
nit: URL_INDICATOR_VISIBILITY_TIMEOUT_MS ?

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:582: let menuMode =
this.menuMode && mode == api.Mode.STANDARD;
it is a little bit confuse that menuMode isn't this.menuMode. Also, linke 586
and line 591 still has mode == api.Mode.STANDARD check. Perhaps revert change in
line 578 and let menuMode = this.menuMode?

[cjgrant, 2017-02-02 16:58:59]

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:175: if (content_paused_ != paused)
{
On 2017/01/31 17:03:31, bshe wrote:
> nit: prefer early return
> e.g. if (content_paused_ ==  paused)
>         return;

Done.

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:367: case SET_CONTENT_PAUSED: {
On 2017/01/31 15:34:00, asimjour1 wrote:
> nit: please remove the brace

This keeps 'bool paused' scoped to this case (same as LOAD_URL).  Why remove?

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

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:574:
api.doAction(api.Action.SET_CONTENT_PAUSED, {'paused': this.menuMode});
On 2017/01/31 15:34:00, asimjour1 wrote:
> On 2017/01/30 21:55:57, cjgrant wrote:
> > On second thought, this should not be here, it should be called in
> > updateState().  App button clicks won't be the only way out of menu mode.
> 
> I agree.

Done.

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:579: /** @const */ var
VISIBILITY_TIMEOUT_MS = 5000;
On 2017/01/31 17:03:32, bshe wrote:
> nit: URL_INDICATOR_VISIBILITY_TIMEOUT_MS ?

Done.  Yes, thanks, terrible rebase on my part.

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/vr_shell_ui.js:582: let menuMode =
this.menuMode && mode == api.Mode.STANDARD;
On 2017/01/31 17:03:31, bshe wrote:
> it is a little bit confuse that menuMode isn't this.menuMode. Also, linke 586
> and line 591 still has mode == api.Mode.STANDARD check. Perhaps revert change
in
> line 578 and let menuMode = this.menuMode?

Done. Agreed. Since we currently cannot access menu mode in WebVR (the button is
disabled), I just got rid of the standard mode check for the menu elements. Let
me know if anything still looks odd to you.

[mthiesse, 2017-02-02 17:01:33]

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2664973002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell.cc:177: if
(delegate_provider_->device_provider()) {
On 2017/01/30 21:55:56, cjgrant wrote:
> I removed a check here for "if in WebVR".  Do we really want to pass this
state
> only if in WebVR?  Seems safer to pass it over always.

sgtm

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:183:
delegate_provider_->device_provider()->OnDisplayBlur();
Note: on ToT, this would now be
delegate_provider_->device_provider()->Device()->OnBlur();

[cjgrant, 2017-02-02 17:05:56]

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:183:
delegate_provider_->device_provider()->OnDisplayBlur();
On 2017/02/02 17:01:33, mthiesse wrote:
> Note: on ToT, this would now be
> delegate_provider_->device_provider()->Device()->OnBlur();

Thanks!

[bshe, 2017-02-02 17:11:57]

On 2017/02/02 17:05:56, cjgrant wrote:
>
https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
> File chrome/browser/android/vr_shell/vr_shell.cc (right):
> 
>
https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
> chrome/browser/android/vr_shell/vr_shell.cc:183:
> delegate_provider_->device_provider()->OnDisplayBlur();
> On 2017/02/02 17:01:33, mthiesse wrote:
> > Note: on ToT, this would now be
> > delegate_provider_->device_provider()->Device()->OnBlur();
> 
> Thanks!

lgtm

[mthiesse, 2017-02-02 18:01:11]

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (left):

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:288: #if defined(ENABLE_VR_SHELL)
I know you didn't write this, but can you please update this ifdef to runtime
flag check? ENABLE_VR_SHELL should always be true now.

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:369: case SET_CONTENT_PAUSED: {
nit: We should probably use braces on call cases, or not at all? I'm not sure
what value they add here, so my preference is to remove them.

[cjgrant1, 2017-02-02 19:25:38]

cjgrant@google.com changed reviewers:
+ cjgrant@google.com

[cjgrant1, 2017-02-02 19:25:39]

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (left):

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:288: #if defined(ENABLE_VR_SHELL)
On 2017/02/02 18:01:10, mthiesse wrote:
> I know you didn't write this, but can you please update this ifdef to runtime
> flag check? ENABLE_VR_SHELL should always be true now.

Yep, sure, but preferably in a follow-on CL as I need to dig into that.

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell.cc (right):

https://codereview.chromium.org/2664973002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell.cc:369: case SET_CONTENT_PAUSED: {
On 2017/02/02 18:01:10, mthiesse wrote:
> nit: We should probably use braces on call cases, or not at all? I'm not sure
> what value they add here, so my preference is to remove them.

So here, they're keeping "bool paused" limited to the scope of the case.  
Putting them in all cases just bloats the code, so I use them where they
actually do something.

[cjgrant1, 2017-02-02 19:25:45]

The CQ bit was checked by cjgrant@google.com

[cjgrant1, 2017-02-02 19:25:45]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2664973002/#ps40001
(title: "Rebase to ToT.")

[commit-bot: I haz the power, 2017-02-02 19:26:00]

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-02-02 21:10:47]

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

[commit-bot: I haz the power, 2017-02-02 21:10:52]

Failed to apply patch for chrome/browser/android/vr_shell/ui_interface.h:
While running git apply --index -p1;
  error: patch failed: chrome/browser/android/vr_shell/ui_interface.h:33
  error: chrome/browser/android/vr_shell/ui_interface.h: patch does not apply

Patch:       chrome/browser/android/vr_shell/ui_interface.h
Index: chrome/browser/android/vr_shell/ui_interface.h
diff --git a/chrome/browser/android/vr_shell/ui_interface.h
b/chrome/browser/android/vr_shell/ui_interface.h
index
d31995d8a5062006fff4b25248f2ec1b665bc4e4..f2c969ac28ea562daaba7d6e3cf5ee46b536525c
100644
--- a/chrome/browser/android/vr_shell/ui_interface.h
+++ b/chrome/browser/android/vr_shell/ui_interface.h
@@ -33,24 +33,21 @@ class UiInterface {
   explicit UiInterface(Mode initial_mode, bool fullscreen);
   virtual ~UiInterface();
 
+  // Set HTML UI state or pass events.
   void SetMode(Mode mode);
-  Mode GetMode() { return mode_; }
-  void SetMenuMode(bool enabled);
-  bool GetMenuMode() { return menu_mode_; }
   void SetFullscreen(bool enabled);
-  bool GetFullscreen() { return fullscreen_; }
   void SetSecurityLevel(int level);
   void SetWebVRSecureOrigin(bool secure);
   void SetLoading(bool loading);
   void SetLoadProgress(double progress);
   void SetURL(const GURL& url);
-
-  // Omnibox input and output handling.
-  void HandleOmniboxInput(const base::DictionaryValue& input);
   void SetOmniboxSuggestions(std::unique_ptr<base::Value> suggestions);
+  void HandleAppButtonClicked();
 
-  // Called by WebUI when starting VR.
+  // Handlers for HTML UI commands and notifications.
   void OnDomContentsLoaded();
+  void HandleOmniboxInput(const base::DictionaryValue& input);
+
   void SetUiCommandHandler(UiCommandHandler* handler);
 
  private:
@@ -58,7 +55,6 @@ class UiInterface {
   void FlushModeState();
 
   Mode mode_;
-  bool menu_mode_ = false;
   bool fullscreen_ = false;
   UiCommandHandler* handler_;
   bool loaded_ = false;

[cjgrant, 2017-02-03 16:05:00]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-02-03 16:05:01]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2664973002/#ps60001
(title: "Rebase again to resolve ToT conflict.")

[commit-bot: I haz the power, 2017-02-03 16:05:25]

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-02-03 17:38:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486137900977210,
"parent_rev": "5f067c6fa15e295d45c51946470567d5cd1baca3", "commit_rev":
"cb6d69f2601372a0fbc8760fbb62c7f6e0cc3cd8"}

[commit-bot: I haz the power, 2017-02-03 17:38:35]

Description was changed from

==========
Let HTML UI fully manage menu mode.

- Have native pass app button presses up to the UI.
- The UI decides whether or not to be in menu mode.
- The UI may call back down to native to pause content, if desired.

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

to

==========
Let HTML UI fully manage menu mode.

- Have native pass app button presses up to the UI.
- The UI decides whether or not to be in menu mode.
- The UI may call back down to native to pause content, if desired.

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

Review-Url: https://codereview.chromium.org/2664973002
Cr-Commit-Position: refs/heads/master@{#448017}
Committed:
https://chromium.googlesource.com/chromium/src/+/cb6d69f2601372a0fbc8760fbb62...
==========

[commit-bot: I haz the power, 2017-02-03 17:38:36]

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