Chromium Code Reviews
Help | Chromium Project | Sign in
(9)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 5 days ago by yzshen1
Modified:
1 day, 18 hours ago
Reviewers:
Ken Rockot, dcheng, alokp
CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, tfarina, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
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 "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/+/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 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+543 lines, -302 lines) Patch
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 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 chunk +38 lines, -0 lines 0 comments Download
A mojo/public/js/new_bindings/base.js View 1 2 3 4 1 chunk +33 lines, -0 lines 0 comments Download
M mojo/public/js/new_bindings/bindings.js View 1 2 3 4 8 chunks +22 lines, -31 lines 0 comments Download
M mojo/public/js/new_bindings/buffer.js View 2 chunks +4 lines, -5 lines 0 comments Download
M mojo/public/js/new_bindings/codec.js View 10 chunks +52 lines, -56 lines 0 comments Download
M mojo/public/js/new_bindings/connector.js View 1 2 3 4 4 chunks +20 lines, -30 lines 0 comments Download
M mojo/public/js/new_bindings/interface_types.js View 3 chunks +8 lines, -14 lines 0 comments Download
M mojo/public/js/new_bindings/lib/control_message_handler.js View 1 2 2 chunks +33 lines, -38 lines 0 comments Download
M mojo/public/js/new_bindings/lib/control_message_proxy.js View 1 2 5 chunks +29 lines, -34 lines 0 comments Download
M mojo/public/js/new_bindings/router.js View 1 2 3 4 8 chunks +13 lines, -26 lines 0 comments Download
M mojo/public/js/new_bindings/unicode.js View 2 chunks +7 lines, -7 lines 0 comments Download
M mojo/public/js/new_bindings/validator.js View 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 1 chunk +27 lines, -6 lines 0 comments Download
M mojo/public/tools/bindings/generators/js_templates/module_definition.tmpl View 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 1 chunk +26 lines, -0 lines 0 comments Download
Trybot results:  win_chromium_x64_rel_ng   linux_chromium_chromeos_rel_ng   win_chromium_x64_rel_ng   ios-simulator   chromium_presubmit   win_chromium_x64_rel_ng   linux_chromium_chromeos_rel_ng   ios-device   win_chromium_rel_ng   mac_chromium_rel_ng   mac_chromium_compile_dbg_ng   linux_chromium_tsan_rel_ng   ios-simulator   linux_chromium_chromeos_rel_ng   linux_chromium_compile_dbg_ng   win_clang   linux_chromium_rel_ng   win_chromium_compile_dbg_ng   ios-device-xcode-clang   win_chromium_x64_rel_ng   ios-simulator-xcode-clang   linux_chromium_chromeos_ozone_rel_ng   linux_android_rel_ng   chromeos_daisy_chromium_compile_only_ng   android_n5x_swarming_rel   chromeos_amd64-generic_chromium_compile_only_ng   android_clang_dbg_recipe   android_compile_dbg   chromium_presubmit   cast_shell_linux   android_cronet   cast_shell_android   linux_chromium_asan_rel_ng   android_arm64_dbg_recipe   win_clang   ios-simulator   linux_chromium_rel_ng   chromeos_daisy_chromium_compile_only_ng   android_arm64_dbg_recipe   linux_chromium_compile_dbg_ng   win_chromium_rel_ng   android_clang_dbg_recipe   linux_android_rel_ng   chromeos_amd64-generic_chromium_compile_only_ng   ios-device   cast_shell_linux   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_asan_rel_ng   android_cronet   win_chromium_x64_rel_ng   ios-simulator-xcode-clang   ios-device-xcode-clang   mac_chromium_rel_ng   chromium_presubmit   win_chromium_compile_dbg_ng   android_compile_dbg   linux_chromium_chromeos_rel_ng   linux_chromium_tsan_rel_ng   mac_chromium_compile_dbg_ng   cast_shell_android   android_n5x_swarming_rel 
Commit queue not available (can’t edit this change).

Depends on Patchset:

Messages

Total messages: 53 (35 generated)
yzshen1
Hi, Ken. Would you please take a look? Thanks! A test of the new bindings ...
1 week, 5 days ago (2017-03-17 23:57:15 UTC) #2
Ken Rockot
This is great! I don't see any obvious cause of the redness right now, but ...
1 week, 3 days 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 ...
1 week, 2 days ago (2017-03-20 17:45:02 UTC) #22
Ken Rockot
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, ...
1 week, 1 day 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 ...
1 week, 1 day ago (2017-03-22 16:09:25 UTC) #33
dcheng
lgtm
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
5 days, 8 hours 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)
5 days, 7 hours 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
5 days, 2 hours 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
5 days 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 days, 14 hours 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 days, 11 hours 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 days 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 days 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 ...
2 days, 23 hours 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 ...
2 days, 23 hours ago (2017-03-27 17:24:21 UTC) #51
alokp
> > I meant mojo.createMessagePipe by creating an instance of Mojo and assigning > it ...
2 days, 22 hours ago (2017-03-27 17:50:09 UTC) #52
yzshen1
1 day, 18 hours ago (2017-03-28 22:04:25 UTC) #53
Message was sent while issue was closed.
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.
Sign in to reply to this message.

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