VrShell: Split HTML UI Javascript into UI and support portions.

VrShell: Split HTML UI Javascript into UI and support portions.

With this change, we have:
- A native API Javascript module, hiding details of the Native API.
- A scene helper class, to manage adding and removing elements and
  animations.
- The remaining UI Javascript.

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

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

[cjgrant, 2016-10-03 19:43:30]

Description was changed from

==========
VrShell: Split HTML UI Javascript into UI and support portions.

With this change, we have:
- A native API Javascript module, hiding details of the Native API.
- A scene helper class, to manage adding and removing elements and
  animations.
- The remaining UI Javascript.

BUG=
==========

to

==========
VrShell: Split HTML UI Javascript into UI and support portions.

With this change, we have:
- A native API Javascript module, hiding details of the Native API.
- A scene helper class, to manage adding and removing elements and
  animations.
- The remaining UI Javascript.

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

[cjgrant, 2016-10-03 19:44:10]

Description was changed from

==========
VrShell: Split HTML UI Javascript into UI and support portions.

With this change, we have:
- A native API Javascript module, hiding details of the Native API.
- A scene helper class, to manage adding and removing elements and
  animations.
- The remaining UI Javascript.

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

to

==========
VrShell: Split HTML UI Javascript into UI and support portions.

With this change, we have:
- A native API Javascript module, hiding details of the Native API.
- A scene helper class, to manage adding and removing elements and
  animations.
- The remaining UI Javascript.

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

[cjgrant, 2016-10-03 19:44:10]

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

[cjgrant, 2016-10-03 19:44:52]

Guys, I'm posting this for a sniff-test only.  Please ignore style, lack of
comments, whitespace, and anything related.

I ONLY want to see if you think the split makes sense.

[cjgrant, 2016-10-03 19:50:31]

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

https://codereview.chromium.org/2392683002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/native_api.js:115: class UiElementUpdate {
Elements can be updated like this:

update = new api.UiElementUpdate();
update.setScale(4, 4, 2);
scene.updateElement(button_id, update)

https://codereview.chromium.org/2392683002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/native_api.js:206: return {
I think I prefer the syntax that inlines the above classes and variables right
in this return dictionary.  Thoughts?

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

https://codereview.chromium.org/2392683002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/scene.js:17: flush() {
This lets us queue a bunch of changes, then apply them in one atomic operation.

https://codereview.chromium.org/2392683002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/scene.js:71: purge() {
This could be used if we can make "reload the scene via button for testing"
work.

[mthiesse, 2016-10-03 20:42:48]

Seems like the right approach, from a high level.

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

https://codereview.chromium.org/2392683002/diff/1/chrome/browser/resources/vr...
chrome/browser/resources/vr_shell/native_api.js:165: constructor(pixelX, pixelY,
pixelWidth, pixelHeight, metersX, metersY) {
metersX/Y should probably be set using setSize to be consistent with others.

[cjgrant, 2016-10-04 19:21:27]

This is relatively clean now.  PTAL.

[bshe, 2016-10-05 00:01:40]

lgtm with nits

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/native_api.js (right):

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/vr_shell/native_api.js:227: this.startInMillis = 0.0;
nit: startInMillis/startInMs

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/vr_shell/native_api.js:230: this.durationMillis =
durationMs;
nit: durationMillis/durationMs

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/scene.js (right):

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/vr_shell/scene.js:38: * // Animate the button size back
to its original size, over 1000 ms.
nit: 250 ms

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.html (right):

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.html:26: 
nit: remove extra empty line

[cjgrant, 2016-10-05 15:21:11]

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/native_api.js (right):

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/vr_shell/native_api.js:227: this.startInMillis = 0.0;
On 2016/10/05 00:01:40, bshe wrote:
> nit: startInMillis/startInMs

Changing these requires native-side changes, which breaks Josh's UI (either for
new builds or existing APKs).  I don't know of a good way to handle this (here
and in the future).  We could have native offer a version API, such that JS can
vary its calls accordingly.  Thoughts?

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/scene.js (right):

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/vr_shell/scene.js:38: * // Animate the button size back
to its original size, over 1000 ms.
On 2016/10/05 00:01:40, bshe wrote:
> nit: 250 ms

Done.

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/vr_shell/vr_shell_ui.html (right):

https://codereview.chromium.org/2392683002/diff/80001/chrome/browser/resource...
chrome/browser/resources/vr_shell/vr_shell_ui.html:26: 
On 2016/10/05 00:01:40, bshe wrote:
> nit: remove extra empty line

Done.

[cjgrant, 2016-10-06 14:34:34]

cjgrant@chromium.org changed reviewers:
+ xiyuan@chromium.org

[cjgrant, 2016-10-06 14:34:35]

xiyuan@chromium.org: Please review changes in:

chrome/browser/browser_resources.grd

Thanks!

[xiyuan, 2016-10-06 16:19:18]

https://codereview.chromium.org/2392683002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/vr_shell/vr_shell_ui_scene.js (right):

https://codereview.chromium.org/2392683002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/vr_shell/vr_shell_ui_scene.js:83: 'data': mesh
What is |mesh|? Do you mean |update|?

[cjgrant, 2016-10-06 16:56:10]

https://codereview.chromium.org/2392683002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/vr_shell/vr_shell_ui_scene.js (right):

https://codereview.chromium.org/2392683002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/vr_shell/vr_shell_ui_scene.js:83: 'data': mesh
On 2016/10/06 16:19:18, xiyuan wrote:
> What is |mesh|? Do you mean |update|?

"mesh" is the name of the base::DictionaryValue key parsed by native code (ie.
changing it requires a native-side change).  We have other such native-side
cleanup we'd like to do, but ideally it would wait until we've committed this
new API interface.

[xiyuan, 2016-10-06 17:04:14]

https://codereview.chromium.org/2392683002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/vr_shell/vr_shell_ui_scene.js (right):

https://codereview.chromium.org/2392683002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/vr_shell/vr_shell_ui_scene.js:83: 'data': mesh
On 2016/10/06 16:56:10, cjgrant wrote:
> On 2016/10/06 16:19:18, xiyuan wrote:
> > What is |mesh|? Do you mean |update|?
> 
> "mesh" is the name of the base::DictionaryValue key parsed by native code (ie.
> changing it requires a native-side change).  We have other such native-side
> cleanup we'd like to do, but ideally it would wait until we've committed this
> new API interface.

I don't see "mesh" is defined anywhere. Wouldn't we get a reference error when
constructing the command? If we want to pass this as string, it should be put
into quotes, i.e. 'mesh'.

[cjgrant, 2016-10-06 17:07:07]

https://codereview.chromium.org/2392683002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/vr_shell/vr_shell_ui_scene.js (right):

https://codereview.chromium.org/2392683002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/vr_shell/vr_shell_ui_scene.js:83: 'data': mesh
On 2016/10/06 17:04:14, xiyuan wrote:
> On 2016/10/06 16:56:10, cjgrant wrote:
> > On 2016/10/06 16:19:18, xiyuan wrote:
> > > What is |mesh|? Do you mean |update|?
> > 
> > "mesh" is the name of the base::DictionaryValue key parsed by native code
(ie.
> > changing it requires a native-side change).  We have other such native-side
> > cleanup we'd like to do, but ideally it would wait until we've committed
this
> > new API interface.
> 
> I don't see "mesh" is defined anywhere. Wouldn't we get a reference error when
> constructing the command? If we want to pass this as string, it should be put
> into quotes, i.e. 'mesh'.

I apologize - you're absolutely correct, and I misread your comment.  Thanks for
catching this.  We don't yet use updateElement, but we will very soon.

[cjgrant, 2016-10-06 17:12:37]

https://codereview.chromium.org/2392683002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/vr_shell/vr_shell_ui_scene.js (right):

https://codereview.chromium.org/2392683002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/vr_shell/vr_shell_ui_scene.js:83: 'data': mesh
On 2016/10/06 17:07:07, cjgrant wrote:
> On 2016/10/06 17:04:14, xiyuan wrote:
> > On 2016/10/06 16:56:10, cjgrant wrote:
> > > On 2016/10/06 16:19:18, xiyuan wrote:
> > > > What is |mesh|? Do you mean |update|?
> > > 
> > > "mesh" is the name of the base::DictionaryValue key parsed by native code
> (ie.
> > > changing it requires a native-side change).  We have other such
native-side
> > > cleanup we'd like to do, but ideally it would wait until we've committed
> this
> > > new API interface.
> > 
> > I don't see "mesh" is defined anywhere. Wouldn't we get a reference error
when
> > constructing the command? If we want to pass this as string, it should be
put
> > into quotes, i.e. 'mesh'.
> 
> I apologize - you're absolutely correct, and I misread your comment.  Thanks
for
> catching this.  We don't yet use updateElement, but we will very soon.

Done.

[xiyuan, 2016-10-06 17:18:01]

lgtm

[cjgrant, 2016-10-06 17:57:51]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2016-10-06 17:57:52]

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/2392683002/#ps160001
(title: "Fix element update mesh typo.")

[commit-bot: I haz the power, 2016-10-06 17:58:20]

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-06 20:20:16]

Description was changed from

==========
VrShell: Split HTML UI Javascript into UI and support portions.

With this change, we have:
- A native API Javascript module, hiding details of the Native API.
- A scene helper class, to manage adding and removing elements and
  animations.
- The remaining UI Javascript.

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

to

==========
VrShell: Split HTML UI Javascript into UI and support portions.

With this change, we have:
- A native API Javascript module, hiding details of the Native API.
- A scene helper class, to manage adding and removing elements and
  animations.
- The remaining UI Javascript.

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

[commit-bot: I haz the power, 2016-10-06 20:20:18]

Committed patchset #9 (id:160001)

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

Description was changed from

==========
VrShell: Split HTML UI Javascript into UI and support portions.

With this change, we have:
- A native API Javascript module, hiding details of the Native API.
- A scene helper class, to manage adding and removing elements and
  animations.
- The remaining UI Javascript.

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

to

==========
VrShell: Split HTML UI Javascript into UI and support portions.

With this change, we have:
- A native API Javascript module, hiding details of the Native API.
- A scene helper class, to manage adding and removing elements and
  animations.
- The remaining UI Javascript.

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

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

[commit-bot: I haz the power, 2016-10-06 20:23:21]

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