Promise types may be represented incorrectly in IDL. |
|||||
Issue descriptionThe Problem: There are 24 IDL files in Chromium that are formatted incorrectly according to the IDL spec. The Types section (https://heycam.github.io/webidl/#idl-types) of the IDL spec shows that Promise are supposed to be represented like this: Promise < ReturnType > Here's an example from the clipboard API (https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/clipboard/clipboard.idl): Promise<DOMString> readText(); Some IDL files have thins like this: Promise createImageBitmap(ImageBitmapSource imageBitmap, optional ImageBitmapOptions options) (For the curious, the line that blows up in webidl2 is this: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources/webidl2.js?q=Webidl2&g=0&l=320) I don't know exactly how webidl2 is used by Chromium, but I would have thought someone else would have bumped into this. I'll be happy to submit a PR to correct the IDL files if that's what's needed. (I'm opening a ticket on webidl2 to ask for more graceful handling of the error.) The 24 files are these: blink/renderer/core/svg/svg_image_element.idl blink/renderer/core/imagebitmap/image_bitmap_factories.idl blink/renderer/core/aom/computed_accessible_node.idl blink/renderer/core/html/html_image_element.idl blink/renderer/core/testing/internals.idl blink/renderer/core/dom/element.idl blink/renderer/core/workers/worker_global_scope.idl blink/renderer/bindings/tests/idls/core/test_attributes.idl blink/renderer/bindings/tests/idls/core/test_object.idl blink/renderer/bindings/tests/idls/core/test_interface.idl blink/renderer/bindings/tests/idls/modules/test_interface_partial_3.idl blink/renderer/modules/wake_lock/navigator_wake_lock.idl blink/renderer/modules/vr/navigator_vr.idl blink/renderer/modules/vr/vr_display.idl blink/renderer/modules/crypto/subtle_crypto.idl blink/renderer/modules/credentialmanager/credentials_container.idl blink/renderer/modules/credentialmanager/public_key_credential.idl blink/renderer/modules/screen_orientation/screen_orientation.idl blink/renderer/modules/push_messaging/push_manager.idl blink/renderer/modules/encryptedmedia/html_media_element_encrypted_media.idl blink/renderer/modules/filesystem/file_system_directory_iterator.idl blink/renderer/modules/battery/navigator_battery.idl blink/renderer/modules/webmidi/midi_port.idl blink/renderer/modules/notifications/service_worker_registration_notifications.idl
,
Nov 26
,
Nov 26
webidl2.js is used in web-platform-tests to parse the files in LayoutTests/external/wpt/interfaces/, but not to parse any of the IDL files used to generate Chromium C++ code. Those are processed by a bindings generator written in Python. The fix for this would be to require a type for Promise<T> in that Python code, and fixing the cases where there is none currently. Probably a very easy fix.
,
Nov 26
Our idl parser can parse Promise<ReturnType> but the code generator doesn't use them. peria@ is working on new code generator (called v2). I think this will be fixed by code generator v2.
,
Nov 30
,
Dec 3
Comment #4 is true, but this bug is based on another issue in our IDL parser. It takes "Promise" as "Promise<any>". https://cs.chromium.org/chromium/src/tools/idl_parser/test_parser/interface_web.idl?l=345-351,356 We can work for it with updating IDL files and making the parser reject "Promise" without return value.
,
Dec 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b0806b882e7cc4efe8bdc10ee431c922972f9a4c commit b0806b882e7cc4efe8bdc10ee431c922972f9a4c Author: Hitoshi Yoshida <peria@chromium.org> Date: Fri Dec 07 09:03:17 2018 bindings: Update IDL files with Promise In WebIDL spec, Promise requires a type parameter like Promise<T>, but Blink's IDL compiler allows not to have it, and some IDL files use Promise without parameters. This CL update such Promise types in .idl files to follow each spec as far as spec defines. Otherwise, update to Promise<any> as our IDL perser converts. Because our IDL compiler ignores the parameter, this CL has no actual behavior changes. Bug: 900628 Change-Id: Ieb6aeb68c52608c7629e11c96653da729b050624 Reviewed-on: https://chromium-review.googlesource.com/c/1367068 Reviewed-by: Yuki Shiino <yukishiino@chromium.org> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Hitoshi Yoshida <peria@chromium.org> Cr-Commit-Position: refs/heads/master@{#614645} [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/bindings/tests/idls/core/test_interface.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/bindings/tests/idls/core/test_object.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/bindings/tests/idls/modules/test_interface_partial_3.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/core/aom/computed_accessible_node.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/core/dom/element.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/core/frame/window_or_worker_global_scope.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/core/html/html_image_element.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/core/svg/svg_image_element.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/core/testing/internals.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/battery/navigator_battery.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/credentialmanager/credentials_container.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/credentialmanager/public_key_credential.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/crypto/subtle_crypto.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/encryptedmedia/html_media_element_encrypted_media.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/filesystem/file_system_directory_iterator.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/notifications/service_worker_registration_notifications.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/push_messaging/push_manager.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/screen_orientation/screen_orientation.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/vr/navigator_vr.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/vr/vr_display.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/wake_lock/navigator_wake_lock.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/webmidi/midi_port.idl [modify] https://crrev.com/b0806b882e7cc4efe8bdc10ee431c922972f9a4c/third_party/blink/renderer/modules/webmidi/navigator_web_midi.idl
,
Dec 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/86c013f510fe87ba4f8efed31a5fe1e8efb2a408 commit 86c013f510fe87ba4f8efed31a5fe1e8efb2a408 Author: Hitoshi Yoshida <peria@chromium.org> Date: Fri Dec 07 10:08:24 2018 IDL parser: Disallow to use Promise without return type We allowed to use Promise without return types. But now no specs use Promise without return types. This CL drops the support of unspec'ed Promise use cases. Bug: 900628 Change-Id: Ife6f6fe62894ea48f0a3d6a6778f95d7b44bd0bf Reviewed-on: https://chromium-review.googlesource.com/c/1367069 Commit-Queue: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Cr-Commit-Position: refs/heads/master@{#614652} [modify] https://crrev.com/86c013f510fe87ba4f8efed31a5fe1e8efb2a408/tools/idl_parser/idl_parser.py [modify] https://crrev.com/86c013f510fe87ba4f8efed31a5fe1e8efb2a408/tools/idl_parser/test_parser/interface_web.idl
,
Dec 7
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by dtapu...@chromium.org
, Nov 26Components: Blink