Implement a means of letting native VR Shell control the HTML UI.

Implement a means of letting native VR Shell control the HTML UI.

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

Committed: https://crrev.com/59c4b66c37827da7e3853a0d5e8c6947e627c7fa
Cr-Commit-Position: refs/heads/master@{#426601}

[cjgrant, 2016-10-19 17:25:05]

Description was changed from

==========
Impelement a means of letting native VR Shell control the HTML UI.

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

to

==========
Impelement a means of letting native VR Shell control the HTML UI.

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

[cjgrant, 2016-10-19 17:33:41]

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

[mthiesse, 2016-10-19 18:04:10]

lgtm % nits

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

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.cc:25: updates_.SetInteger("mode",
static_cast<int>(mode));
Maybe store the mode we're in locally, and only update on change? This class
could manage the shared state between UI and backend.

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.cc:30:
updates_.SetBoolean("secureOrigin", static_cast<int>(secure));
Same comment here, you can store whether we're in secure origin mode locally and
only update on change.

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

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:115: function command(dict) {
Put in a TODO to implement this?

[bshe, 2016-10-19 21:02:40]

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

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.cc:8: #include
"base/memory/weak_ptr.h"
nit: remove the above two header as you included in .h already

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.cc:41:
handler_->GetWebUi()->CallJavascriptFunctionUnsafe(
Calling "CallJavascriptFunctionUnsafe" directly is discouraged. Perhaps you can
create a public function in vr_shell_ui_message_handler.cc and wrap
WebUiMessageHanlder:CallJavascriptFunction in it. This way, you don't need to
check |loaded_| too.  Unless there is a case that you might try to call js
before webui is ready.

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_interface.h (right):

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.h:39:
base::WeakPtr<VrShellUIMessageHandler> handler_;
do you need a weakptr here? My guess is that this class will out live
VrShellUIMessageHandler. And you can probably set it to null in the dtor of
VrShellUIMessageHandler since you are able to access this through
vr_shell->GetUiInterface. If VrShellUIMessageHandler out live this class, you
don't need a weakptr either.

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.h:40: base::DictionaryValue
updates_;
nit: looks like this is the global states. Perhaps rename it to states_. And I
would also prefer only do update on change.

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

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:5:
cr.define('chrome.vrShellUi', function() {
now that you have cr.js, perhaps do the same for vr_shell_ui_api and
vr_shell_ui_scene?

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/vr_shell/vr_shell_ui_message_handler.cc (right):

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/vr_shell/vr_shell_ui_message_handler.cc:18:
VrShellUIMessageHandler::VrShellUIMessageHandler() :
nit: : move colon to next line

[cjgrant, 2016-10-20 14:32:40]

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

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.cc:41:
handler_->GetWebUi()->CallJavascriptFunctionUnsafe(
On 2016/10/19 21:02:40, bshe wrote:
> Calling "CallJavascriptFunctionUnsafe" directly is discouraged. Perhaps you
can
> create a public function in vr_shell_ui_message_handler.cc and wrap
> WebUiMessageHanlder:CallJavascriptFunction in it. This way, you don't need to
> check |loaded_| too.  Unless there is a case that you might try to call js
> before webui is ready.

As of now, VrShell gets calls to set the mode to WebVR before we see
OnDomContentsLoaded().  So there needs to be some caching of this state, such
that when we see the HTML UI finish loading, we can push any accumulated
information up to it.

Interestingly, for secure origin, I think we actually want to relay calls to the
HTML UI every time VR shell gets this call.  If we go from insecure site to
insecure site, the HTML UI should know so that it can restart the temporary
warning timer.

I'll think about this a bit more...

[cjgrant, 2016-10-20 15:25:39]

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

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.cc:8: #include
"base/memory/weak_ptr.h"
On 2016/10/19 21:02:40, bshe wrote:
> nit: remove the above two header as you included in .h already

Done.

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.cc:25: updates_.SetInteger("mode",
static_cast<int>(mode));
On 2016/10/19 18:04:10, mthiesse wrote:
> Maybe store the mode we're in locally, and only update on change? This class
> could manage the shared state between UI and backend.

Possibly, but at this point we shouldn't get a mode set more than once per page,
and some things we need to send even if they don't change (secure origin calls
should restart the 30-second temporary warning timer).  Will certainly cache
state in the future if we need to.

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.cc:30:
updates_.SetBoolean("secureOrigin", static_cast<int>(secure));
On 2016/10/19 18:04:10, mthiesse wrote:
> Same comment here, you can store whether we're in secure origin mode locally
and
> only update on change.

See above.

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.cc:41:
handler_->GetWebUi()->CallJavascriptFunctionUnsafe(
On 2016/10/20 14:32:40, cjgrant wrote:
> On 2016/10/19 21:02:40, bshe wrote:
> > Calling "CallJavascriptFunctionUnsafe" directly is discouraged. Perhaps you
> can
> > create a public function in vr_shell_ui_message_handler.cc and wrap
> > WebUiMessageHanlder:CallJavascriptFunction in it. This way, you don't need
to
> > check |loaded_| too.  Unless there is a case that you might try to call js
> > before webui is ready.
> 
> As of now, VrShell gets calls to set the mode to WebVR before we see
> OnDomContentsLoaded().  So there needs to be some caching of this state, such
> that when we see the HTML UI finish loading, we can push any accumulated
> information up to it.
> 
> Interestingly, for secure origin, I think we actually want to relay calls to
the
> HTML UI every time VR shell gets this call.  If we go from insecure site to
> insecure site, the HTML UI should know so that it can restart the temporary
> warning timer.
> 
> I'll think about this a bit more...

Done.

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_interface.h (right):

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.h:39:
base::WeakPtr<VrShellUIMessageHandler> handler_;
On 2016/10/19 21:02:40, bshe wrote:
> do you need a weakptr here? My guess is that this class will out live
> VrShellUIMessageHandler. And you can probably set it to null in the dtor of
> VrShellUIMessageHandler since you are able to access this through
> vr_shell->GetUiInterface. If VrShellUIMessageHandler out live this class, you
> don't need a weakptr either.

Done.  Good call on use of the handler destructor.

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_interface.h:40: base::DictionaryValue
updates_;
On 2016/10/19 21:02:40, bshe wrote:
> nit: looks like this is the global states. Perhaps rename it to states_. And I
> would also prefer only do update on change.

As discussed, the dictionary isn't maintaining state, other than when VR
launches.  I missed clearing it after a flush.  Thanks for catching that.

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

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:5:
cr.define('chrome.vrShellUi', function() {
On 2016/10/19 21:02:40, bshe wrote:
> now that you have cr.js, perhaps do the same for vr_shell_ui_api and
> vr_shell_ui_scene?

Why do that if native is calling only to the UI?  We don't need to expose
api/scene stuff to native, as they are helper libraries for the UI.  Let me know
if I'm missing something.

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/vr_shell/vr_shell_ui_message_handler.cc (right):

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/vr_shell/vr_shell_ui_message_handler.cc:18:
VrShellUIMessageHandler::VrShellUIMessageHandler() :
On 2016/10/19 21:02:40, bshe wrote:
> nit: : move colon to next line 

Done.

[cjgrant, 2016-10-20 15:26:54]

Description was changed from

==========
Impelement a means of letting native VR Shell control the HTML UI.

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

to

==========
Implement a means of letting native VR Shell control the HTML UI.

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

[bshe, 2016-10-20 18:07:48]

lgtm

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

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:5:
cr.define('chrome.vrShellUi', function() {
On 2016/10/20 15:25:39, cjgrant wrote:
> On 2016/10/19 21:02:40, bshe wrote:
> > now that you have cr.js, perhaps do the same for vr_shell_ui_api and
> > vr_shell_ui_scene?
> 
> Why do that if native is calling only to the UI?  We don't need to expose
> api/scene stuff to native, as they are helper libraries for the UI.  Let me
know
> if I'm missing something.

I don't think it is related to native. I believe cr.define is just a helper to
define a namespace. So you don't need to chrome= {} an dchrome.vrShellUi = {}.
It would be nice to have all the js under the same namespace to avoid potential
conflict. Although unlikely this is going to happen. So if you prefer to leave
it this way, I am fine.
I am fine.

[cjgrant, 2016-10-20 19:59:59]

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

https://codereview.chromium.org/2434013002/diff/20001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.js:5:
cr.define('chrome.vrShellUi', function() {
On 2016/10/20 18:07:48, bshe wrote:
> On 2016/10/20 15:25:39, cjgrant wrote:
> > On 2016/10/19 21:02:40, bshe wrote:
> > > now that you have cr.js, perhaps do the same for vr_shell_ui_api and
> > > vr_shell_ui_scene?
> > 
> > Why do that if native is calling only to the UI?  We don't need to expose
> > api/scene stuff to native, as they are helper libraries for the UI.  Let me
> know
> > if I'm missing something.
> 
> I don't think it is related to native. I believe cr.define is just a helper to
> define a namespace. So you don't need to chrome= {} an dchrome.vrShellUi = {}.
> It would be nice to have all the js under the same namespace to avoid
potential
> conflict. Although unlikely this is going to happen. So if you prefer to leave
> it this way, I am fine.
> I am fine.

As discussed, removed use of cr.define().

[cjgrant, 2016-10-20 20:00:04]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2016-10-20 20:00:05]

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/2434013002/#ps80001
(title: "Remove use of cr.define()")

[commit-bot: I haz the power, 2016-10-20 20:00: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, 2016-10-20 21:25:40]

Description was changed from

==========
Implement a means of letting native VR Shell control the HTML UI.

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

to

==========
Implement a means of letting native VR Shell control the HTML UI.

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

[commit-bot: I haz the power, 2016-10-20 21:25:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-21 13:22:46]

Description was changed from

==========
Implement a means of letting native VR Shell control the HTML UI.

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

to

==========
Implement a means of letting native VR Shell control the HTML UI.

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

Committed: https://crrev.com/59c4b66c37827da7e3853a0d5e8c6947e627c7fa
Cr-Commit-Position: refs/heads/master@{#426601}
==========

[commit-bot: I haz the power, 2016-10-21 13:22:47]

Patchset 5 (id:??) landed as
https://crrev.com/59c4b66c37827da7e3853a0d5e8c6947e627c7fa
Cr-Commit-Position: refs/heads/master@{#426601}