Mojo JS bindings: change module loading solution.

Mojo JS bindings: change module loading solution.

This change takes place on the mojo/public/js/new_bindings copy so it doesn't
affect existing users.

- This change gets rid of AMD module loading. Now the bindings API is defined in
the "mojo" namespace. At build time, all bindings files are combined
into a single file "mojo_bindings.js". Users should use <script> tag to include
this file (as well as generated mojom.js files).

- Generated mojom.js files export their definitions under the same namespace as
the "module" statement in the corresponding mojom files.

- This change also adds a "use_new_js_bindings" option to the generator. It
  duplicates the control message mojom files in order to generate two
  different flavors of JS bindings.

- The new bindings use the Mojo system API defined by Web IDL.

BUG=699569
Review-Url: https://codereview.chromium.org/2759563004
Cr-Commit-Position: refs/heads/master@{#459654}
Committed: https://chromium.googlesource.com/chromium/src/+/e6a5534bb3fe61b5224f1a22e43ba957190ad5d0

[yzshen1, 2017-03-17 23:57:14]

yzshen@chromium.org changed reviewers:
+ rockot@chromium.org

[yzshen1, 2017-03-17 23:57:15]

Hi, Ken.

Would you please take a look? Thanks!

A test of the new bindings is here:
https://codereview.chromium.org/2759803002/

(It depends on another change that damargulis@ is working on. That is why I
didn't include it here.)

Besides, auto-loading mojom deps will be in a separate CL.

Thanks!

[yzshen1, 2017-03-19 04:02:24]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 04:02:37]

Dry run: 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-19 04:04:46]

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

[commit-bot: I haz the power, 2017-03-19 04:04:47]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[yzshen1, 2017-03-19 05:07:07]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 05:07:11]

Dry run: 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-19 05:16:05]

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

[commit-bot: I haz the power, 2017-03-19 05:16:07]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[yzshen1, 2017-03-19 05:42:24]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 05:42:31]

Dry run: 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-19 07:00:40]

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

[commit-bot: I haz the power, 2017-03-19 07:00:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yzshen1, 2017-03-19 23:41:01]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 23:41:14]

Dry run: 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-20 03:22:16]

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

[commit-bot: I haz the power, 2017-03-20 03:22:17]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ken Rockot(use gerrit already), 2017-03-20 15:27:20]

This is great! I don't see any obvious cause of the redness right now, but some
inline comments

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/interfaces/...
File mojo/public/interfaces/bindings/tests/echo_service.mojom (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/echo_service.mojom:9: interface
EchoService {
nit: Even in spite of other "service" names in test interfaces, would you mind
changing this to not be "EchoService"? Could it just be "Echo"? And could we add
an inner mojom namespace?

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
File mojo/public/js/new_bindings/base.js (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
mojo/public/js/new_bindings/base.js:9: var mojoBindings = {};
I wonder about naming here, but it's kind of just a bikeshed. Should it be
MojoBindings, or do we reserve that naming style for IDL generated types? And I
suppose Mojo.Bindings is out of the question since that means monkeypatching a
generated namespace.

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
File mojo/public/js/new_bindings/connector.js (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
mojo/public/js/new_bindings/connector.js:9: if (!handle instanceof MojoHandle)
Unfortunately ! has higher precedence than instanceof, so this condition will
never pass even if handle is not an instance of MojoHandle and we should throw
an error

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
mojo/public/js/new_bindings/connector.js:80: throw new Error("Not supported!");
Just curious - are you planning to not implement wait in the new bindings? Can
we make tests which use it asynchronous? i.e. they don't depend on blocking JS
execution for a time?

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
File mojo/public/js/new_bindings/router.js (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
mojo/public/js/new_bindings/router.js:9: if (!handle instanceof MojoHandle)
same issue here with ! vs instanceof precedence

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/js_templates/module.amd.tmpl (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/module.amd.tmpl:10: var
bindings = mojoBindings;
optional nit: can we use let instead of var for new JS? I guess we don't emit
"use strict" at file scope? Also maybe consistency is better and there are
probably far too many "var" uses in the bindings code to justify updating them
all now?

[yzshen1, 2017-03-20 17:41:18]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 17:41:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2017-03-20 17:45:02]

Thanks Ken!

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/interfaces/...
File mojo/public/interfaces/bindings/tests/echo_service.mojom (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/echo_service.mojom:9: interface
EchoService {
On 2017/03/20 15:27:20, Ken Rockot wrote:
> nit: Even in spite of other "service" names in test interfaces, would you mind
> changing this to not be "EchoService"? Could it just be "Echo"? And could we
add
> an inner mojom namespace?

Done. Thanks!

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
File mojo/public/js/new_bindings/base.js (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
mojo/public/js/new_bindings/base.js:9: var mojoBindings = {};
On 2017/03/20 15:27:20, Ken Rockot wrote:
> I wonder about naming here, but it's kind of just a bikeshed. Should it be
> MojoBindings, or do we reserve that naming style for IDL generated types? And
I
> suppose Mojo.Bindings is out of the question since that means monkeypatching a
> generated namespace.

According to JS style guide, namespace should be namedLikeThis (with lower case
initial).
However, we have things like "Mojo.createMessagePipe" in the WebIDL-generated
API. I think we view "Mojo" as a type instead of a namespace in this case. A
supporting evidence is that we are not using Mojo.Handle, instead we use
MojoHandle.

Do you think we need to start a bikeshedding thread for naming of the
WebIDL-generated API? :) It is usually not used directly so it shouldn't be too
much work even if we would like to change the names later.

Another thing I would like to bring up is whether we want to do our best to
convert mojom names to match JS coding style?
At the moment, method names are converted to match JS style.

However we don't do it for most names (module name, field name, parameter name,
etc.) For example we map

module "some_namespace";

to a namespace "some_namespace" in JS, that is not the format recommended by JS
style guide ("someNamespace").

IMO, it would be nice to follow the style guide as close as possible. And it
probably makes sense to do it at the time while we are switching to a "new
mode", although the work is a little tedious for me. :)

WDYT?

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
File mojo/public/js/new_bindings/connector.js (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
mojo/public/js/new_bindings/connector.js:9: if (!handle instanceof MojoHandle)
On 2017/03/20 15:27:20, Ken Rockot wrote:
> Unfortunately ! has higher precedence than instanceof, so this condition will
> never pass even if handle is not an instance of MojoHandle and we should throw
> an error

Oops. :P Now you know how little I know about JS.

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
mojo/public/js/new_bindings/connector.js:80: throw new Error("Not supported!");
On 2017/03/20 15:27:20, Ken Rockot wrote:
> Just curious - are you planning to not implement wait in the new bindings? Can
> we make tests which use it asynchronous? i.e. they don't depend on blocking JS
> execution for a time?

I am planning to make the tests that rely on this to do it asynchronously. And
if we still need wait in the future, we could add support then.

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
File mojo/public/js/new_bindings/router.js (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
mojo/public/js/new_bindings/router.js:9: if (!handle instanceof MojoHandle)
On 2017/03/20 15:27:20, Ken Rockot wrote:
> same issue here with ! vs instanceof precedence

Thanks! I updated all the places.

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/js_templates/module.amd.tmpl (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/js_templates/module.amd.tmpl:10: var
bindings = mojoBindings;
On 2017/03/20 15:27:20, Ken Rockot wrote:
> optional nit: can we use let instead of var for new JS? I guess we don't emit
> "use strict" at file scope? Also maybe consistency is better and there are
> probably far too many "var" uses in the bindings code to justify updating them
> all now?

Added "use strict". 

IOS9 doesn't support "let" yet. So I don't think we can use that at the moment.
:/

[yzshen1, 2017-03-20 21:47:56]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 21:49:12]

Dry run: 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-20 22:59:01]

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

[commit-bot: I haz the power, 2017-03-20 22:59:02]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ken Rockot(use gerrit already), 2017-03-21 16:42:43]

lgtm

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
File mojo/public/js/new_bindings/base.js (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
mojo/public/js/new_bindings/base.js:9: var mojoBindings = {};
On 2017/03/20 at 17:45:02, yzshen1 wrote:
> On 2017/03/20 15:27:20, Ken Rockot wrote:
> > I wonder about naming here, but it's kind of just a bikeshed. Should it be
> > MojoBindings, or do we reserve that naming style for IDL generated types?
And I
> > suppose Mojo.Bindings is out of the question since that means monkeypatching
a
> > generated namespace.
> 
> According to JS style guide, namespace should be namedLikeThis (with lower
case initial).
> However, we have things like "Mojo.createMessagePipe" in the WebIDL-generated
API. I think we view "Mojo" as a type instead of a namespace in this case. A
supporting evidence is that we are not using Mojo.Handle, instead we use
MojoHandle.
> 
> Do you think we need to start a bikeshedding thread for naming of the
WebIDL-generated API? :) It is usually not used directly so it shouldn't be too
much work even if we would like to change the names later.
> 
> Another thing I would like to bring up is whether we want to do our best to
convert mojom names to match JS coding style?
> At the moment, method names are converted to match JS style.
> 
> However we don't do it for most names (module name, field name, parameter
name, etc.) For example we map
> 
> module "some_namespace";
> 
> to a namespace "some_namespace" in JS, that is not the format recommended by
JS style guide ("someNamespace").
> 
> IMO, it would be nice to follow the style guide as close as possible. And it
probably makes sense to do it at the time while we are switching to a "new
mode", although the work is a little tedious for me. :)
> 
> WDYT?

I agree with this rationale, so SGTM. In fact I was going to make the same
suggestion about module namespaces.

[yzshen1, 2017-03-21 22:34:59]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 22:35:28]

Dry run: 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-22 00:30:29]

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

[commit-bot: I haz the power, 2017-03-22 00:30:30]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-03-22 16:09:24]

yzshen@chromium.org changed reviewers:
+ dcheng@chromium.org

[yzshen1, 2017-03-22 16:09:25]

+Daniel for security review.

Hi, Daniel.

Please take a look at those *.mojom files:
The following two are exact duplicates (except the namespace) of the two mojoms
in the parent folder. I duplicate them so that I can generate two different
modes of the JS bindings. This copy will be removed once the old mode goes away.
mojo/public/interfaces/bindings/new_bindings/interface_control_messages.mojom
mojo/public/interfaces/bindings/new_bindings/pipe_control_messages.mojom

The following two are testing files.
mojo/public/interfaces/bindings/tests/echo.mojom
mojo/public/interfaces/bindings/tests/echo_import.mojom

[dcheng, 2017-03-23 00:13:04]

lgtm

[yzshen1, 2017-03-25 07:39:28]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2017-03-25 07:39:29]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2759563004/#ps120001
(title: ".")

[commit-bot: I haz the power, 2017-03-25 07:39:42]

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-25 09:03:52]

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

[commit-bot: I haz the power, 2017-03-25 09:03:58]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yzshen1, 2017-03-25 13:48:52]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2017-03-25 13:49:07]

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-25 15:59:55]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490449732469580,
"parent_rev": "2d2a3b2d3247797fd2d2c8f471b18f40b18cfeab", "commit_rev":
"e6a5534bb3fe61b5224f1a22e43ba957190ad5d0"}

[commit-bot: I haz the power, 2017-03-25 16:00:41]

Description was changed from

==========
Mojo JS bindings: change module loading solution.

This change takes place on the mojo/public/js/new_bindings copy so it doesn't
affect existing users.

- This change gets rid of AMD module loading. Now the bindings API is defined in
the "mojoBindings" namespace. At build time, all bindings files are combined
into a single file "mojo_bindings.js". Users should use <script> tag to include
this file (as well as generated mojom.js files).

- Generated mojom.js files export their definitions under the same namespace as
the "module" statement in the corresponding mojom files.

- This change also adds a "use_new_js_bindings" option to the generator. It
  duplicates the control message mojom files in order to generate two
  different flavors of JS bindings.

- The new bindings use the Mojo system API defined by Web IDL.

BUG=699569
==========

to

==========
Mojo JS bindings: change module loading solution.

This change takes place on the mojo/public/js/new_bindings copy so it doesn't
affect existing users.

- This change gets rid of AMD module loading. Now the bindings API is defined in
the "mojoBindings" namespace. At build time, all bindings files are combined
into a single file "mojo_bindings.js". Users should use <script> tag to include
this file (as well as generated mojom.js files).

- Generated mojom.js files export their definitions under the same namespace as
the "module" statement in the corresponding mojom files.

- This change also adds a "use_new_js_bindings" option to the generator. It
  duplicates the control message mojom files in order to generate two
  different flavors of JS bindings.

- The new bindings use the Mojo system API defined by Web IDL.

BUG=699569

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

[commit-bot: I haz the power, 2017-03-25 16:00:42]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e6a5534bb3fe61b5224f1a22e43b...

[alokp, 2017-03-27 01:56:25]

alokp@chromium.org changed reviewers:
+ alokp@chromium.org

[alokp, 2017-03-27 01:56:26]

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
File mojo/public/js/new_bindings/base.js (right):

https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
mojo/public/js/new_bindings/base.js:9: var mojoBindings = {};
On 2017/03/20 17:45:02, yzshen1 wrote:
> On 2017/03/20 15:27:20, Ken Rockot wrote:
> > I wonder about naming here, but it's kind of just a bikeshed. Should it be
> > MojoBindings, or do we reserve that naming style for IDL generated types?
And
> I
> > suppose Mojo.Bindings is out of the question since that means monkeypatching
a
> > generated namespace.
> 
> According to JS style guide, namespace should be namedLikeThis (with lower
case
> initial).
> However, we have things like "Mojo.createMessagePipe" in the WebIDL-generated
> API. I think we view "Mojo" as a type instead of a namespace in this case. A
> supporting evidence is that we are not using Mojo.Handle, instead we use
> MojoHandle.
> 
> Do you think we need to start a bikeshedding thread for naming of the
> WebIDL-generated API? :) It is usually not used directly so it shouldn't be
too
> much work even if we would like to change the names later.
> 
> Another thing I would like to bring up is whether we want to do our best to
> convert mojom names to match JS coding style?
> At the moment, method names are converted to match JS style.
> 
> However we don't do it for most names (module name, field name, parameter
name,
> etc.) For example we map
> 
> module "some_namespace";
> 
> to a namespace "some_namespace" in JS, that is not the format recommended by
JS
> style guide ("someNamespace").
> 
> IMO, it would be nice to follow the style guide as close as possible. And it
> probably makes sense to do it at the time while we are switching to a "new
> mode", although the work is a little tedious for me. :)
> 
> WDYT?
> 

Bikeshed: I find mojoBindings ugly :)

I am open to installing the Mojo object into window.mojo and then we can extend
the mojo object with everything in the Bindings layer? Then consumers of the API
do not need to care about the distinction between core and bindings? This would
also be consistent with C++ API where everything is in the mojo namespace.

[hayato, 2017-03-27 04:38:22]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2779533002/ by hayato@chromium.org.

The reason for reverting is: Consistent failure: webkit_tests failing on 6
builders

33 since the first detection
e.g.
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac1...

This CL is suspicious because mojo/module-loading.html has been failing.


https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit...

Regressions: Unexpected text-only failures (1)
  mojo/module-loading.html [ Failure ]

09:20:53.708 5958 worker/0
virtual/mojo-loading/http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html
output stderr lines:
09:20:53.709 5958   [09:20:53.354] vtDecompressionDuctCreate signalled err=-8973
(err) (Could not select and open decoder instance) at
/SourceCache/CoreMedia_frameworks/CoreMedia-1562.235/Sources/VideoToolbox/VTDecompressionSession.c
line 1181
09:20:53.709 5958   <<<< VTVideoEncoderSelection >>>>
VTSelectAndCreateVideoEncoderInstanceInternal: no video encoder found for 'avc1'
.

[yzshen1, 2017-03-27 16:27:04]

On 2017/03/27 01:56:26, alokp wrote:
>
https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
> File mojo/public/js/new_bindings/base.js (right):
> 
>
https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
> mojo/public/js/new_bindings/base.js:9: var mojoBindings = {};
> On 2017/03/20 17:45:02, yzshen1 wrote:
> > On 2017/03/20 15:27:20, Ken Rockot wrote:
> > > I wonder about naming here, but it's kind of just a bikeshed. Should it be
> > > MojoBindings, or do we reserve that naming style for IDL generated types?
> And
> > I
> > > suppose Mojo.Bindings is out of the question since that means
monkeypatching
> a
> > > generated namespace.
> > 
> > According to JS style guide, namespace should be namedLikeThis (with lower
> case
> > initial).
> > However, we have things like "Mojo.createMessagePipe" in the
WebIDL-generated
> > API. I think we view "Mojo" as a type instead of a namespace in this case. A
> > supporting evidence is that we are not using Mojo.Handle, instead we use
> > MojoHandle.
> > 
> > Do you think we need to start a bikeshedding thread for naming of the
> > WebIDL-generated API? :) It is usually not used directly so it shouldn't be
> too
> > much work even if we would like to change the names later.
> > 
> > Another thing I would like to bring up is whether we want to do our best to
> > convert mojom names to match JS coding style?
> > At the moment, method names are converted to match JS style.
> > 
> > However we don't do it for most names (module name, field name, parameter
> name,
> > etc.) For example we map
> > 
> > module "some_namespace";
> > 
> > to a namespace "some_namespace" in JS, that is not the format recommended by
> JS
> > style guide ("someNamespace").
> > 
> > IMO, it would be nice to follow the style guide as close as possible. And it
> > probably makes sense to do it at the time while we are switching to a "new
> > mode", although the work is a little tedious for me. :)
> > 
> > WDYT?
> > 
> 
> Bikeshed: I find mojoBindings ugly :)
> 
> I am open to installing the Mojo object into window.mojo and then we can
extend
> the mojo object with everything in the Bindings layer? Then consumers of the
API
> do not need to care about the distinction between core and bindings? This
would
> also be consistent with C++ API where everything is in the mojo namespace.

When you said installing the Mojo object into window.mojo. Do you mean we will
have mojo.Mojo.createMessagePipe / mojo.MojoHandle / etc.? Or do you mean
mojo.createMessagePipe / mojo.Handle? If we are not directly patching anther
object, but instead installing both the core API and the bindings into the same
namespace. I am happy with that.

WDYT?

[yzshen1, 2017-03-27 16:29:06]

On 2017/03/27 16:27:04, yzshen1 wrote:
> On 2017/03/27 01:56:26, alokp wrote:
> >
>
https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
> > File mojo/public/js/new_bindings/base.js (right):
> > 
> >
>
https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
> > mojo/public/js/new_bindings/base.js:9: var mojoBindings = {};
> > On 2017/03/20 17:45:02, yzshen1 wrote:
> > > On 2017/03/20 15:27:20, Ken Rockot wrote:
> > > > I wonder about naming here, but it's kind of just a bikeshed. Should it
be
> > > > MojoBindings, or do we reserve that naming style for IDL generated
types?
> > And
> > > I
> > > > suppose Mojo.Bindings is out of the question since that means
> monkeypatching
> > a
> > > > generated namespace.
> > > 
> > > According to JS style guide, namespace should be namedLikeThis (with lower
> > case
> > > initial).
> > > However, we have things like "Mojo.createMessagePipe" in the
> WebIDL-generated
> > > API. I think we view "Mojo" as a type instead of a namespace in this case.
A
> > > supporting evidence is that we are not using Mojo.Handle, instead we use
> > > MojoHandle.
> > > 
> > > Do you think we need to start a bikeshedding thread for naming of the
> > > WebIDL-generated API? :) It is usually not used directly so it shouldn't
be
> > too
> > > much work even if we would like to change the names later.
> > > 
> > > Another thing I would like to bring up is whether we want to do our best
to
> > > convert mojom names to match JS coding style?
> > > At the moment, method names are converted to match JS style.
> > > 
> > > However we don't do it for most names (module name, field name, parameter
> > name,
> > > etc.) For example we map
> > > 
> > > module "some_namespace";
> > > 
> > > to a namespace "some_namespace" in JS, that is not the format recommended
by
> > JS
> > > style guide ("someNamespace").
> > > 
> > > IMO, it would be nice to follow the style guide as close as possible. And
it
> > > probably makes sense to do it at the time while we are switching to a "new
> > > mode", although the work is a little tedious for me. :)
> > > 
> > > WDYT?
> > > 
> > 
> > Bikeshed: I find mojoBindings ugly :)
> > 
> > I am open to installing the Mojo object into window.mojo and then we can
> extend
> > the mojo object with everything in the Bindings layer? Then consumers of the
> API
> > do not need to care about the distinction between core and bindings? This
> would
> > also be consistent with C++ API where everything is in the mojo namespace.
> 
> When you said installing the Mojo object into window.mojo. Do you mean we will
> have mojo.Mojo.createMessagePipe / mojo.MojoHandle / etc.? Or do you mean
> mojo.createMessagePipe / mojo.Handle? If we are not directly patching anther
> object, but instead installing both the core API and the bindings into the
same
> namespace.
When I said, "if we are not directly patching anther object", I meant "if we are
not directly patching a type auto-generated from WebIDL". :)

> I am happy with that.
> 
> WDYT?

[alokp, 2017-03-27 17:12:10]

On 2017/03/27 16:27:04, yzshen1 wrote:
> On 2017/03/27 01:56:26, alokp wrote:
> >
>
https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
> > File mojo/public/js/new_bindings/base.js (right):
> > 
> >
>
https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
> > mojo/public/js/new_bindings/base.js:9: var mojoBindings = {};
> > On 2017/03/20 17:45:02, yzshen1 wrote:
> > > On 2017/03/20 15:27:20, Ken Rockot wrote:
> > > > I wonder about naming here, but it's kind of just a bikeshed. Should it
be
> > > > MojoBindings, or do we reserve that naming style for IDL generated
types?
> > And
> > > I
> > > > suppose Mojo.Bindings is out of the question since that means
> monkeypatching
> > a
> > > > generated namespace.
> > > 
> > > According to JS style guide, namespace should be namedLikeThis (with lower
> > case
> > > initial).
> > > However, we have things like "Mojo.createMessagePipe" in the
> WebIDL-generated
> > > API. I think we view "Mojo" as a type instead of a namespace in this case.
A
> > > supporting evidence is that we are not using Mojo.Handle, instead we use
> > > MojoHandle.
> > > 
> > > Do you think we need to start a bikeshedding thread for naming of the
> > > WebIDL-generated API? :) It is usually not used directly so it shouldn't
be
> > too
> > > much work even if we would like to change the names later.
> > > 
> > > Another thing I would like to bring up is whether we want to do our best
to
> > > convert mojom names to match JS coding style?
> > > At the moment, method names are converted to match JS style.
> > > 
> > > However we don't do it for most names (module name, field name, parameter
> > name,
> > > etc.) For example we map
> > > 
> > > module "some_namespace";
> > > 
> > > to a namespace "some_namespace" in JS, that is not the format recommended
by
> > JS
> > > style guide ("someNamespace").
> > > 
> > > IMO, it would be nice to follow the style guide as close as possible. And
it
> > > probably makes sense to do it at the time while we are switching to a "new
> > > mode", although the work is a little tedious for me. :)
> > > 
> > > WDYT?
> > > 
> > 
> > Bikeshed: I find mojoBindings ugly :)
> > 
> > I am open to installing the Mojo object into window.mojo and then we can
> extend
> > the mojo object with everything in the Bindings layer? Then consumers of the
> API
> > do not need to care about the distinction between core and bindings? This
> would
> > also be consistent with C++ API where everything is in the mojo namespace.
> 
> When you said installing the Mojo object into window.mojo. Do you mean we will
> have mojo.Mojo.createMessagePipe / mojo.MojoHandle / etc.? Or do you mean
> mojo.createMessagePipe / mojo.Handle? If we are not directly patching anther
> object, but instead installing both the core API and the bindings into the
same
> namespace. I am happy with that.
> 
> WDYT?

I meant mojo.createMessagePipe by creating an instance of Mojo and assigning it
to window.mojo. This is how most other Web APIs work AFAICT. The bindings layer
would then be able to extend this window.mojo object. This also means that we
would still have MojoHandle (not mojo.Handle). And I am OK with that since
MojoHandle is a type name.

If you would rather not patch window.mojo, we could create it in the bindings
layer and add simple wrappers for Mojo functions like:
window.mojo.createMessagePipe = () { return Mojo.createMessagePipe(); }

This would present a single mojo object by hiding the IDL layer from the
clients.

It might be worth asking blink folks for their recommendation.

[yzshen1, 2017-03-27 17:24:21]

On 2017/03/27 17:12:10, alokp wrote:
> On 2017/03/27 16:27:04, yzshen1 wrote:
> > On 2017/03/27 01:56:26, alokp wrote:
> > >
> >
>
https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
> > > File mojo/public/js/new_bindings/base.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bind...
> > > mojo/public/js/new_bindings/base.js:9: var mojoBindings = {};
> > > On 2017/03/20 17:45:02, yzshen1 wrote:
> > > > On 2017/03/20 15:27:20, Ken Rockot wrote:
> > > > > I wonder about naming here, but it's kind of just a bikeshed. Should
it
> be
> > > > > MojoBindings, or do we reserve that naming style for IDL generated
> types?
> > > And
> > > > I
> > > > > suppose Mojo.Bindings is out of the question since that means
> > monkeypatching
> > > a
> > > > > generated namespace.
> > > > 
> > > > According to JS style guide, namespace should be namedLikeThis (with
lower
> > > case
> > > > initial).
> > > > However, we have things like "Mojo.createMessagePipe" in the
> > WebIDL-generated
> > > > API. I think we view "Mojo" as a type instead of a namespace in this
case.
> A
> > > > supporting evidence is that we are not using Mojo.Handle, instead we use
> > > > MojoHandle.
> > > > 
> > > > Do you think we need to start a bikeshedding thread for naming of the
> > > > WebIDL-generated API? :) It is usually not used directly so it shouldn't
> be
> > > too
> > > > much work even if we would like to change the names later.
> > > > 
> > > > Another thing I would like to bring up is whether we want to do our best
> to
> > > > convert mojom names to match JS coding style?
> > > > At the moment, method names are converted to match JS style.
> > > > 
> > > > However we don't do it for most names (module name, field name,
parameter
> > > name,
> > > > etc.) For example we map
> > > > 
> > > > module "some_namespace";
> > > > 
> > > > to a namespace "some_namespace" in JS, that is not the format
recommended
> by
> > > JS
> > > > style guide ("someNamespace").
> > > > 
> > > > IMO, it would be nice to follow the style guide as close as possible.
And
> it
> > > > probably makes sense to do it at the time while we are switching to a
"new
> > > > mode", although the work is a little tedious for me. :)
> > > > 
> > > > WDYT?
> > > > 
> > > 
> > > Bikeshed: I find mojoBindings ugly :)
> > > 
> > > I am open to installing the Mojo object into window.mojo and then we can
> > extend
> > > the mojo object with everything in the Bindings layer? Then consumers of
the
> > API
> > > do not need to care about the distinction between core and bindings? This
> > would
> > > also be consistent with C++ API where everything is in the mojo namespace.
> > 
> > When you said installing the Mojo object into window.mojo. Do you mean we
will
> > have mojo.Mojo.createMessagePipe / mojo.MojoHandle / etc.? Or do you mean
> > mojo.createMessagePipe / mojo.Handle? If we are not directly patching anther
> > object, but instead installing both the core API and the bindings into the
> same
> > namespace. I am happy with that.
> > 
> > WDYT?
> 
> I meant mojo.createMessagePipe by creating an instance of Mojo and assigning
it
> to window.mojo. This is how most other Web APIs work AFAICT. The bindings
layer
> would then be able to extend this window.mojo object. This also means that we
> would still have MojoHandle (not mojo.Handle). And I am OK with that since
> MojoHandle is a type name.

It seems a little inconsistent to me if we have MojoHandle,
MojoCreateMessagePipeResult... and they don't belong to the mojo namespace.
Is it easy to install createMessagePipe (function), Handle,
CreateMessagePipeResult, etc. all into the mojo namespace?


> 
> If you would rather not patch window.mojo, we could create it in the bindings
> layer and add simple wrappers for Mojo functions like:
> window.mojo.createMessagePipe = () { return Mojo.createMessagePipe(); }
> 
> This would present a single mojo object by hiding the IDL layer from the
> clients.
> 
> It might be worth asking blink folks for their recommendation.

Yeah. That makes sense. Do you know who are the best to ask? I am a little
afraid a bikeshedding mail sent to a large mailing list will trigger endless
discussion. :P

[alokp, 2017-03-27 17:50:09]

> > I meant mojo.createMessagePipe by creating an instance of Mojo and assigning
> it
> > to window.mojo. This is how most other Web APIs work AFAICT. The bindings
> layer
> > would then be able to extend this window.mojo object. This also means that
we
> > would still have MojoHandle (not mojo.Handle). And I am OK with that since
> > MojoHandle is a type name.
> 
> It seems a little inconsistent to me if we have MojoHandle,
> MojoCreateMessagePipeResult... and they don't belong to the mojo namespace.
> Is it easy to install createMessagePipe (function), Handle,
> CreateMessagePipeResult, etc. all into the mojo namespace?

All dictionary types are hidden from the clients. They never need to know about
CreateMessagePipeResult, for example. So the concern here is only for interface
types, i.e., Mojo, MojoHandle, and MojoWatcher. I am not too concerned about
MojoHandle and MojoWatcher, because the only direct usage is when doing
instanceof, where seems consistent with other Web APIs. As I mentioned in my
previous comments, installing Mojo functions into window.mojo would be as simple
as:
window.mojo.createMessagePipe = () { return Mojo.createMessagePipe(); }

> > 
> > If you would rather not patch window.mojo, we could create it in the
bindings
> > layer and add simple wrappers for Mojo functions like:
> > window.mojo.createMessagePipe = () { return Mojo.createMessagePipe(); }
> > 
> > This would present a single mojo object by hiding the IDL layer from the
> > clients.
> > 
> > It might be worth asking blink folks for their recommendation.
> 
> Yeah. That makes sense. Do you know who are the best to ask? I am a little
> afraid a bikeshedding mail sent to a large mailing list will trigger endless
> discussion. :P

I would start with jbroman@ and esprehn@ who reviewed all of my WebIDL patches.

[yzshen1, 2017-03-28 22:04:25]

On 2017/03/27 04:38:22, hayato wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/2779533002/ by mailto:hayato@chromium.org.
> 
> The reason for reverting is: Consistent failure: webkit_tests failing on 6
> builders
> 
> 33 since the first detection
> e.g.
>
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac1...
> 
> This CL is suspicious because mojo/module-loading.html has been failing.
> 
> 
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit...
> 
> Regressions: Unexpected text-only failures (1)
>   mojo/module-loading.html [ Failure ]
> 
> 09:20:53.708 5958 worker/0
>
virtual/mojo-loading/http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html
> output stderr lines:
> 09:20:53.709 5958   [09:20:53.354] vtDecompressionDuctCreate signalled
err=-8973
> (err) (Could not select and open decoder instance) at
>
/SourceCache/CoreMedia_frameworks/CoreMedia-1562.235/Sources/VideoToolbox/VTDecompressionSession.c
> line 1181
> 09:20:53.709 5958   <<<< VTVideoEncoderSelection >>>>
> VTSelectAndCreateVideoEncoderInstanceInternal: no video encoder found for
'avc1'
> .

I figured out the reason: I didn't change build/scripts/slave/zip_build.py to
include the mojo_bindings.js generated file.
It is sad that I have to land a CL in a separate repo in order to fix this.

Will ask for dpranke's opinion when he is back.

[yzshen1, 2017-04-02 06:20:54]

Description was changed from

==========
Mojo JS bindings: change module loading solution.

This change takes place on the mojo/public/js/new_bindings copy so it doesn't
affect existing users.

- This change gets rid of AMD module loading. Now the bindings API is defined in
the "mojoBindings" namespace. At build time, all bindings files are combined
into a single file "mojo_bindings.js". Users should use <script> tag to include
this file (as well as generated mojom.js files).

- Generated mojom.js files export their definitions under the same namespace as
the "module" statement in the corresponding mojom files.

- This change also adds a "use_new_js_bindings" option to the generator. It
  duplicates the control message mojom files in order to generate two
  different flavors of JS bindings.

- The new bindings use the Mojo system API defined by Web IDL.

BUG=699569

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

to

==========
Mojo JS bindings: change module loading solution.

This change takes place on the mojo/public/js/new_bindings copy so it doesn't
affect existing users.

- This change gets rid of AMD module loading. Now the bindings API is defined in
the "mojoBindings" namespace. At build time, all bindings files are combined
into a single file "mojo_bindings.js". Users should use <script> tag to include
this file (as well as generated mojom.js files).

- Generated mojom.js files export their definitions under the same namespace as
the "module" statement in the corresponding mojom files.

- This change also adds a "use_new_js_bindings" option to the generator. It
  duplicates the control message mojom files in order to generate two
  different flavors of JS bindings.

- The new bindings use the Mojo system API defined by Web IDL.

BUG=699569

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

[yzshen1, 2017-04-02 06:24:50]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-02 06:24:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2017-04-02 06:28:32]

yzshen@chromium.org changed reviewers:
+ dpranke@chromium.org

[yzshen1, 2017-04-02 06:28:32]

Hi, Dirk.

Would you please take a look at the change in //BUILD.gn?

As discussed, I added a target in this file to copy mojo_bindings.js to
gen/layout_test_data/.
(Mojom.js files are not yet copied into that folder. That will be in a follow-up
CL.)

Thanks!

[yzshen1, 2017-04-02 06:29:11]

Description was changed from

==========
Mojo JS bindings: change module loading solution.

This change takes place on the mojo/public/js/new_bindings copy so it doesn't
affect existing users.

- This change gets rid of AMD module loading. Now the bindings API is defined in
the "mojoBindings" namespace. At build time, all bindings files are combined
into a single file "mojo_bindings.js". Users should use <script> tag to include
this file (as well as generated mojom.js files).

- Generated mojom.js files export their definitions under the same namespace as
the "module" statement in the corresponding mojom files.

- This change also adds a "use_new_js_bindings" option to the generator. It
  duplicates the control message mojom files in order to generate two
  different flavors of JS bindings.

- The new bindings use the Mojo system API defined by Web IDL.

BUG=699569

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

to

==========
Mojo JS bindings: change module loading solution.

This change takes place on the mojo/public/js/new_bindings copy so it doesn't
affect existing users.

- This change gets rid of AMD module loading. Now the bindings API is defined in
the "mojo" namespace. At build time, all bindings files are combined
into a single file "mojo_bindings.js". Users should use <script> tag to include
this file (as well as generated mojom.js files).

- Generated mojom.js files export their definitions under the same namespace as
the "module" statement in the corresponding mojom files.

- This change also adds a "use_new_js_bindings" option to the generator. It
  duplicates the control message mojom files in order to generate two
  different flavors of JS bindings.

- The new bindings use the Mojo system API defined by Web IDL.

BUG=699569

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

[commit-bot: I haz the power, 2017-04-02 07:51:17]

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

[commit-bot: I haz the power, 2017-04-02 07:51:18]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2017-04-02 21:36:18]

lgtm

[yzshen1, 2017-04-02 21:56:37]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2017-04-02 21:56:38]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2759563004/#ps140001
(title: "Make sure mojo_bindings.js is packaged in build result for running
layout tests.")

[commit-bot: I haz the power, 2017-04-02 21:56:41]

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-04-02 22:01:32]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491170197832590,
"parent_rev": "81f28ed19f771fe93406496d1d23da137abcd602", "commit_rev":
"a06469911359843f3d823fac7a7c06fec3c46edb"}

[commit-bot: I haz the power, 2017-04-02 22:06:14]

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

[commit-bot: I haz the power, 2017-04-02 22:06:16]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.