Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2759563004: Mojo JS bindings: change module loading solution. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by yzshen1
Modified:
2 months, 3 weeks ago
CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, tfarina, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, dpapad
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : sync & merge #

Patch Set 3 : . #

Patch Set 4 : add test module-loading.html #

Total comments: 14

Patch Set 5 : . #

Patch Set 6 : . #

Patch Set 7 : . #

Patch Set 8 : Make sure mojo_bindings.js is packaged in build result for running layout tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+556 lines, -301 lines) Patch
M BUILD.gn View 1 2 3 4 5 6 7 2 chunks +17 lines, -0 lines 0 comments Download
M mojo/public/interfaces/bindings/BUILD.gn View 1 chunk +12 lines, -0 lines 0 comments Download
A mojo/public/interfaces/bindings/new_bindings/OWNERS View 1 chunk +2 lines, -0 lines 0 comments Download
A mojo/public/interfaces/bindings/new_bindings/interface_control_messages.mojom View 1 2 1 chunk +67 lines, -0 lines 0 comments Download
A mojo/public/interfaces/bindings/new_bindings/pipe_control_messages.mojom View 1 2 1 chunk +46 lines, -0 lines 0 comments Download
M mojo/public/interfaces/bindings/tests/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +10 lines, -0 lines 0 comments Download
A mojo/public/interfaces/bindings/tests/echo.mojom View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download
A mojo/public/interfaces/bindings/tests/echo_import.mojom View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
M mojo/public/js/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +38 lines, -0 lines 0 comments Download
A mojo/public/js/new_bindings/base.js View 1 2 3 4 5 6 7 1 chunk +33 lines, -0 lines 0 comments Download
M mojo/public/js/new_bindings/bindings.js View 1 2 3 4 5 6 7 8 chunks +21 lines, -31 lines 0 comments Download
M mojo/public/js/new_bindings/buffer.js View 1 2 3 4 5 6 7 2 chunks +4 lines, -5 lines 0 comments Download
M mojo/public/js/new_bindings/codec.js View 1 2 3 4 5 6 7 10 chunks +51 lines, -56 lines 0 comments Download
M mojo/public/js/new_bindings/connector.js View 1 2 3 4 5 6 7 4 chunks +20 lines, -30 lines 0 comments Download
M mojo/public/js/new_bindings/interface_types.js View 1 2 3 4 5 6 7 3 chunks +8 lines, -14 lines 0 comments Download
M mojo/public/js/new_bindings/lib/control_message_handler.js View 1 2 3 4 5 6 7 2 chunks +33 lines, -38 lines 0 comments Download
M mojo/public/js/new_bindings/lib/control_message_proxy.js View 1 2 3 4 5 6 7 5 chunks +29 lines, -34 lines 0 comments Download
M mojo/public/js/new_bindings/router.js View 1 2 3 4 5 6 7 8 chunks +13 lines, -26 lines 0 comments Download
M mojo/public/js/new_bindings/unicode.js View 1 2 3 4 5 6 7 2 chunks +7 lines, -7 lines 0 comments Download
M mojo/public/js/new_bindings/validator.js View 1 2 3 4 5 6 7 21 chunks +44 lines, -45 lines 0 comments Download
M mojo/public/tools/bindings/generators/js_templates/module.amd.tmpl View 1 2 3 4 5 6 7 2 chunks +26 lines, -5 lines 0 comments Download
M mojo/public/tools/bindings/generators/js_templates/module_definition.tmpl View 1 2 3 4 5 6 7 3 chunks +7 lines, -8 lines 0 comments Download
M mojo/public/tools/bindings/generators/mojom_js_generator.py View 1 chunk +1 line, -0 lines 0 comments Download
M mojo/public/tools/bindings/mojom.gni View 2 chunks +12 lines, -0 lines 0 comments Download
M mojo/public/tools/bindings/mojom_bindings_generator.py View 2 chunks +5 lines, -0 lines 0 comments Download
M mojo/public/tools/bindings/pylib/mojom/generate/generator.py View 2 chunks +3 lines, -2 lines 0 comments Download
A third_party/WebKit/LayoutTests/mojo/module-loading.html View 1 2 3 4 5 6 7 1 chunk +25 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 68 (46 generated)
yzshen1
Hi, Ken. Would you please take a look? Thanks! A test of the new bindings ...
3 months, 1 week ago (2017-03-17 23:57:15 UTC) #2
Ken Rockot(use gerrit already)
This is great! I don't see any obvious cause of the redness right now, but ...
3 months, 1 week ago (2017-03-20 15:27:20 UTC) #19
yzshen1
Thanks Ken! https://codereview.chromium.org/2759563004/diff/60001/mojo/public/interfaces/bindings/tests/echo_service.mojom File mojo/public/interfaces/bindings/tests/echo_service.mojom (right): https://codereview.chromium.org/2759563004/diff/60001/mojo/public/interfaces/bindings/tests/echo_service.mojom#newcode9 mojo/public/interfaces/bindings/tests/echo_service.mojom:9: interface EchoService { On 2017/03/20 15:27:20, Ken ...
3 months, 1 week ago (2017-03-20 17:45:02 UTC) #22
Ken Rockot(use gerrit already)
lgtm https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bindings/base.js File mojo/public/js/new_bindings/base.js (right): https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bindings/base.js#newcode9 mojo/public/js/new_bindings/base.js:9: var mojoBindings = {}; On 2017/03/20 at 17:45:02, ...
3 months, 1 week ago (2017-03-21 16:42:43 UTC) #27
yzshen1
+Daniel for security review. Hi, Daniel. Please take a look at those *.mojom files: The ...
3 months, 1 week ago (2017-03-22 16:09:25 UTC) #33
dcheng (OOO Jun 25 - Jul 1)
lgtm
3 months, 1 week ago (2017-03-23 00:13:04 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2759563004/120001
3 months ago (2017-03-25 07:39:42 UTC) #37
commit-bot: I haz the power
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_chromeos_rel_ng/builds/391389)
3 months ago (2017-03-25 09:03:58 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2759563004/120001
3 months ago (2017-03-25 13:49:07 UTC) #41
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e6a5534bb3fe61b5224f1a22e43ba957190ad5d0
3 months ago (2017-03-25 16:00:42 UTC) #44
alokp
https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bindings/base.js File mojo/public/js/new_bindings/base.js (right): https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bindings/base.js#newcode9 mojo/public/js/new_bindings/base.js:9: var mojoBindings = {}; On 2017/03/20 17:45:02, yzshen1 wrote: ...
3 months ago (2017-03-27 01:56:26 UTC) #46
hayato
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2779533002/ by hayato@chromium.org. ...
3 months ago (2017-03-27 04:38:22 UTC) #47
yzshen1
On 2017/03/27 01:56:26, alokp wrote: > https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bindings/base.js > File mojo/public/js/new_bindings/base.js (right): > > https://codereview.chromium.org/2759563004/diff/60001/mojo/public/js/new_bindings/base.js#newcode9 > ...
3 months ago (2017-03-27 16:27:04 UTC) #48
yzshen1
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_bindings/base.js ...
3 months ago (2017-03-27 16:29:06 UTC) #49
alokp
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_bindings/base.js ...
3 months ago (2017-03-27 17:12:10 UTC) #50
yzshen1
On 2017/03/27 17:12:10, alokp wrote: > On 2017/03/27 16:27:04, yzshen1 wrote: > > On 2017/03/27 ...
3 months ago (2017-03-27 17:24:21 UTC) #51
alokp
> > I meant mojo.createMessagePipe by creating an instance of Mojo and assigning > it ...
3 months ago (2017-03-27 17:50:09 UTC) #52
yzshen1
On 2017/03/27 04:38:22, hayato wrote: > A revert of this CL (patchset #7 id:120001) has ...
3 months ago (2017-03-28 22:04:25 UTC) #53
yzshen1
Hi, Dirk. Would you please take a look at the change in //BUILD.gn? As discussed, ...
2 months, 4 weeks ago (2017-04-02 06:28:32 UTC) #58
Dirk Pranke
lgtm
2 months, 3 weeks ago (2017-04-02 21:36:18 UTC) #62
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2759563004/140001
2 months, 3 weeks ago (2017-04-02 21:56:41 UTC) #65
commit-bot: I haz the power
2 months, 3 weeks ago (2017-04-02 22:06:16 UTC) #68
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 23e94e589