Refactor HTML UI handling of native commands.

Refactor HTML UI handling of native commands.

Move the definition of the native-to-JS API alongside the existing
JS-to-native API definitions, and implement UI's handling of commands
using overrides.

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

Review-Url: https://codereview.chromium.org/2689583002
Cr-Commit-Position: refs/heads/master@{#449794}
Committed: https://chromium.googlesource.com/chromium/src/+/11d15bc350ba529ff3c37ba761b2b51954f5afff

[cjgrant, 2017-02-09 18:44:07]

Description was changed from

==========
Refactor HTML UI handling of native commands.

Move the definition of the native-to-JS API alongside the existing
JS-to-native API definitions, and implement UI's handling of commands
using overrides.

BUG=None
==========

to

==========
Refactor HTML UI handling of native commands.

Move the definition of the native-to-JS API alongside the existing
JS-to-native API definitions, and implement UI's handling of commands
using overrides.

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

[cjgrant, 2017-02-09 19:37:59]

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

[cjgrant, 2017-02-09 19:38:00]

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right):

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui_api.js:503: * @param
{Array<Object>} suggestions Array of suggestions with string members
I'd like to be specifying the Object format of suggestions (and tabs), but I'm
not sure of the cleanest way to do this yet with Javadocs (create separate
class, typedef, inline it here, etc).

[tiborg, 2017-02-09 21:44:24]

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right):

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui_api.js:444:
api.NativeCommandHandler = class {
Maybe split this into the a command handler (basically the handleCommand
function) and a command listener (the handlers)? Maybe a better separation of
concerns. Also, maybe put this in its own file?

[cjgrant, 2017-02-09 22:02:27]

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right):

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui_api.js:444:
api.NativeCommandHandler = class {
On 2017/02/09 21:44:24, tiborg wrote:
> Maybe split this into the a command handler (basically the handleCommand
> function) and a command listener (the handlers)? Maybe a better separation of
> concerns. Also, maybe put this in its own file?

On separate file, I think that's a separate change at a later time, IMO.

Can you elaborate on the other split?  Are you suggesting a handler function
that takes the listener and dict as arguments?

[tiborg, 2017-02-10 01:33:05]

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui_api.js (right):

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui_api.js:444:
api.NativeCommandHandler = class {
On 2017/02/09 22:02:27, cjgrant wrote:
> On 2017/02/09 21:44:24, tiborg wrote:
> > Maybe split this into the a command handler (basically the handleCommand
> > function) and a command listener (the handlers)? Maybe a better separation
of
> > concerns. Also, maybe put this in its own file?
> 
> On separate file, I think that's a separate change at a later time, IMO.
> 
> Can you elaborate on the other split?  Are you suggesting a handler function
> that takes the listener and dict as arguments?

Agree, the separate file change can be made later as well.

As discussed offline, I was thinking about two classes. One (e.g.
NativeCommandManager) gets fed the native commands. At the NativeCommandManager
an object of the second class (e.g. NativeCommandListener) could be registered.
The NativeCommandListener would implement the handler functions and the
NativeCommandManager would call the handler functions when the corresponding
native command arrives. It's just a suggestion though! Feel free to discard it.
At best the command(...) function in vr_shell_ui.js should also move here. But
that is also a later change.

[tiborg, 2017-02-10 16:37:12]

lgtm

[bshe, 2017-02-10 19:46:50]

lgtm

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

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:789: console.log('Stub: Set
tabs');
Do you need to keep the logs?

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:803: onCommandHandlerFinished()
{
I am guessing you have plan to add most tasks to onCommandHandlerStarted and
onCommandHandlerFinished. It seems unnecessary to add this indirection if it is
just one line of code.

[cjgrant, 2017-02-10 20:17:45]

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

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:789: console.log('Stub: Set
tabs');
On 2017/02/10 19:46:50, bshe wrote:
> Do you need to keep the logs?

Done.

https://codereview.chromium.org/2689583002/diff/40001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:803: onCommandHandlerFinished()
{
On 2017/02/10 19:46:50, bshe wrote:
> I am guessing you have plan to add most tasks to onCommandHandlerStarted and
> onCommandHandlerFinished. It seems unnecessary to add this indirection if it
is
> just one line of code.

I'll remove the unused Started() callback.

[cjgrant, 2017-02-10 20:17:49]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-02-10 20:17:50]

The patchset sent to the CQ was uploaded after l-g-t-m from tiborg@chromium.org,
bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2689583002/#ps60001
(title: "Address Biao's nit.")

[commit-bot: I haz the power, 2017-02-10 20:18:34]

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-10 22:05:11]

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

[commit-bot: I haz the power, 2017-02-10 22:05:19]

Failed to apply patch for chrome/browser/resources/vr_shell/vr_shell_ui.js:
While running git apply --index -p1;
  error: patch failed: chrome/browser/resources/vr_shell/vr_shell_ui.js:56
  error: chrome/browser/resources/vr_shell/vr_shell_ui.js: patch does not apply

Patch:       chrome/browser/resources/vr_shell/vr_shell_ui.js
Index: chrome/browser/resources/vr_shell/vr_shell_ui.js
diff --git a/chrome/browser/resources/vr_shell/vr_shell_ui.js
b/chrome/browser/resources/vr_shell/vr_shell_ui.js
index
aa23decd1163b1fb0a6136144309ecad0f341524..fa42024c9c40d8e7e56306226d565af69047bf7b
100644
--- a/chrome/browser/resources/vr_shell/vr_shell_ui.js
+++ b/chrome/browser/resources/vr_shell/vr_shell_ui.js
@@ -7,6 +7,7 @@ var vrShellUi = (function() {
 
   let ui = new scene.Scene();
   let uiManager;
+  let nativeCommandHandler;
 
   let uiRootElement = document.querySelector('#ui');
   let uiStyle = window.getComputedStyle(uiRootElement);
@@ -56,7 +57,7 @@ var vrShellUi = (function() {
       ui.updateElement(this.elementId, update);
       if (enabled) {
         api.setContentCssSize(
-          this.CSS_WIDTH_PIXELS, this.CSS_HEIGHT_PIXELS, this.DPR);
+            this.CSS_WIDTH_PIXELS, this.CSS_HEIGHT_PIXELS, this.DPR);
       } else {
         // TODO(mthiesse): Restore the webVR resolution (which matches native
         // display resolution).
@@ -664,8 +665,12 @@ var vrShellUi = (function() {
       this.reloadUiButton = new ReloadUiButton();
     }
 
-    setMode(mode, fullscreen) {
+    setMode(mode) {
       this.mode = mode;
+      this.updateState();
+    }
+
+    setFullscreen(fullscreen) {
       this.fullscreen = fullscreen;
       this.updateState();
     }
@@ -683,6 +688,7 @@ var vrShellUi = (function() {
 
       api.doAction(api.Action.SET_CONTENT_PAUSED, {'paused': menuMode});
 
+      this.background.setEnabled(mode == api.Mode.STANDARD);
       this.contentQuad.setEnabled(mode == api.Mode.STANDARD);
       this.contentQuad.setFullscreen(this.fullscreen);
       this.contentQuad.setMenuMode(menuMode);
@@ -696,7 +702,6 @@ var vrShellUi = (function() {
           mode == api.Mode.WEB_VR && !menuMode);
 
       this.reloadUiButton.setEnabled(mode == api.Mode.STANDARD);
-      this.background.setEnabled(mode == api.Mode.STANDARD && !menuMode);
 
       api.setUiCssSize(
           uiRootElement.clientWidth, uiRootElement.clientHeight, UI_DPR);
@@ -713,51 +718,77 @@ var vrShellUi = (function() {
 
   function initialize() {
     uiManager = new UiManager();
+    nativeCommandHandler = new UiCommandHandler(uiManager);
     ui.flush();
 
     api.domLoaded();
   }
 
-  function command(dict) {
-    if ('mode' in dict) {
-      uiManager.setMode(dict['mode'], dict['fullscreen']);
+  class UiCommandHandler extends api.NativeCommandHandler {
+    constructor(uiManager) {
+      super();
+      this.manager = uiManager;
     }
-    if ('appButtonClicked' in dict) {
-      uiManager.handleAppButtonClicked();
+
+    /** @override */
+    onSetMode(mode) {
+      this.manager.setMode(mode);
     }
-    if ('securityLevel' in dict) {
-      uiManager.setSecurityLevel(dict['securityLevel']);
+
+    /** @override */
+    onSetFullscreen(fullscreen) {
+      this.manager.setFullscreen(fullscreen);
     }
-    if ('webVRSecureOrigin' in dict) {
-      uiManager.setWebVRSecureOrigin(dict['webVRSecureOrigin']);
+
+    /** @override */
+    onAppButtonClicked() {
+      this.manager.handleAppButtonClicked();
     }
-    if ('enableReloadUi' in dict) {
-      uiManager.reloadUiButton.setDevMode(dict['enableReloadUi']);
+
+    /** @override */
+    onSetSecurityLevel(level) {
+      this.manager.setSecurityLevel(level);
     }
-    if ('url' in dict) {
-      let url = dict['url'];
-      uiManager.urlIndicator.setURL(url['host'], url['path']);
-      uiManager.omnibox.setURL(url['host'] + url['path']);
+
+    /** @override */
+    onSetWebVRSecureOrigin(secure) {
+      this.manager.setWebVRSecureOrigin(secure);
     }
-    if ('loading' in dict) {
-      uiManager.urlIndicator.setLoading(dict['loading']);
+
+    /** @override */
+    onSetReloadUiCapabilityEnabled(enabled) {
+      this.manager.reloadUiButton.setDevMode(enabled);
     }
-    if ('loadProgress' in dict) {
-      uiManager.urlIndicator.setLoadProgress(dict['loadProgress']);
+
+    /** @override */
+    onSetUrl(host, path) {
+      this.manager.urlIndicator.setURL(host, path);
+      this.manager.omnibox.setURL(host, path);
     }
-    if ('suggestions' in dict) {
-      uiManager.omnibox.setSuggestions(dict['suggestions']);
+
+    /** @override */
+    onSetLoading(loading) {
+      this.manager.urlIndicator.setLoading(loading);
     }
-    if ('setTabs' in dict) {
-      console.log(dict['setTabs']);
+
+    /** @override */
+    onSetLoadingProgress(progress) {
+      this.manager.urlIndicator.setLoadProgress(progress);
     }
-    if ('updateTab' in dict) {
-      console.log(dict['updateTab']);
+
+    /** @override */
+    onSetOmniboxSuggestions(suggestions) {
+      this.manager.omnibox.setSuggestions(suggestions);
     }
-    if ('removeTab' in dict) {
-      console.log(dict['removeTab']);
+
+    /** @override */
+    onCommandHandlerFinished() {
+      ui.flush();
     }
-    ui.flush();
+  }
+
+  function command(dict) {
+    nativeCommandHandler.handleCommand(dict);
   }
 
   return {

[cjgrant, 2017-02-10 22:22:27]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2017-02-10 22:22:27]

The patchset sent to the CQ was uploaded after l-g-t-m from tiborg@chromium.org,
bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2689583002/#ps80001
(title: "Rebase to Tot.")

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

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-10 23:52:01]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486765347063400,
"parent_rev": "f70c51172084e2166d9e56acb3e13ce562a1cc45", "commit_rev":
"11d15bc350ba529ff3c37ba761b2b51954f5afff"}

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

Description was changed from

==========
Refactor HTML UI handling of native commands.

Move the definition of the native-to-JS API alongside the existing
JS-to-native API definitions, and implement UI's handling of commands
using overrides.

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

to

==========
Refactor HTML UI handling of native commands.

Move the definition of the native-to-JS API alongside the existing
JS-to-native API definitions, and implement UI's handling of commands
using overrides.

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

Review-Url: https://codereview.chromium.org/2689583002
Cr-Commit-Position: refs/heads/master@{#449794}
Committed:
https://chromium.googlesource.com/chromium/src/+/11d15bc350ba529ff3c37ba761b2...
==========

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/11d15bc350ba529ff3c37ba761b2...