Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reduce the number of DOM APIs we generate code for #158

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Reduce the number of DOM APIs we generate code for #158

merged 4 commits into from
Feb 6, 2024

Conversation

devoncarew
Copy link
Member

  • reduce the number of DOM APIs we generate code for

The criteria to include an API is:

  • it must be standards-track (it could also be deprecated or experimental)
  • it must be supported by all three of Chrome, Safari, and Firefox

This takes the number of generated libraries from ~277 to ~117.

Done as 2 commits for easier review.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

srujzs
srujzs approved these changes Feb 5, 2024
View reviewed changes
tool/generator/bcd.dart Outdated Show resolved Hide resolved
tool/generator/translator.dart Show resolved Hide resolved
tool/generator/translator.dart
@@ -526,10 +526,12 @@ class Translator {
for (final include in _includes) {
final target = include.target;
final includes = include.includes;
assert(_interfacelikes.containsKey(target));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these still generated even though we don't emit them? Did you come across assertion failures?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yup, both assertions fired. This is something I didn't track down - why they're firing and what that means for the generated API.

I added a bit more diagnostics to see when they're triggering:

not in partial interface likes: PaintRenderingContext2D
not in partial interface likes: CSSPseudoElement
not in partial interface likes: NetworkInformation
not in partial interface likes: Bluetooth
not in partial interface likes: BluetoothDevice
not in partial interface likes: BluetoothRemoteGATTService
not in partial interface likes: BluetoothRemoteGATTCharacteristic
not in partial interface likes: GPUDevice
not in partial interface likes: GPUBuffer
not in partial interface likes: GPUTexture
not in partial interface likes: GPUTextureView
not in partial interface likes: GPUExternalTexture
not in partial interface likes: GPUSampler
not in partial interface likes: GPUBindGroupLayout
not in partial interface likes: GPUBindGroup
not in partial interface likes: GPUPipelineLayout
not in partial interface likes: GPUShaderModule
not in partial interface likes: GPUComputePipeline
not in partial interface likes: GPURenderPipeline
not in partial interface likes: GPUCommandBuffer
not in partial interface likes: GPUCommandEncoder
not in partial interface likes: GPUComputePassEncoder
not in partial interface likes: GPURenderPassEncoder
not in partial interface likes: GPURenderBundle
not in partial interface likes: GPURenderBundleEncoder
not in partial interface likes: GPUQueue
not in partial interface likes: GPUQuerySet


not in calculated mixins:HTMLAttributionSrcElementUtils [target=HTMLAnchorElement]
not in calculated mixins:HTMLAttributionSrcElementUtils [target=HTMLImageElement]
not in calculated mixins:HTMLAttributionSrcElementUtils [target=HTMLScriptElement]
not in calculated mixins:NavigatorBadge [target=Navigator]
not in calculated mixins:NavigatorBadge [target=WorkerNavigator]
not in calculated mixins:Region [target=Element]
not in calculated mixins:NavigatorDeviceMemory [target=Navigator]
not in calculated mixins:NavigatorDeviceMemory [target=WorkerNavigator]
not in calculated mixins:ARIAMixin [target=ElementInternals]
not in calculated mixins:NavigatorNetworkInformation [target=Navigator]
not in calculated mixins:NavigatorNetworkInformation [target=WorkerNavigator]
not in calculated mixins:HTMLSharedStorageWritableElementUtils [target=HTMLIFrameElement]
not in calculated mixins:HTMLSharedStorageWritableElementUtils [target=HTMLImageElement]
not in calculated mixins:NavigatorStorageBuckets [target=Navigator]
not in calculated mixins:NavigatorStorageBuckets [target=WorkerNavigator]
not in calculated mixins:NavigatorUA [target=Navigator]
not in calculated mixins:NavigatorUA [target=WorkerNavigator]
not in calculated mixins:ARIAMixin [target=Element]
not in calculated mixins:NavigatorAutomationInformation [target=Navigator]
not in calculated mixins:NavigatorGPU [target=Navigator]
not in calculated mixins:NavigatorGPU [target=WorkerNavigator]
not in calculated mixins:NavigatorML [target=Navigator]
not in calculated mixins:NavigatorML [target=WorkerNavigator]

devoncarew
devoncarew commented Feb 5, 2024
View reviewed changes
Copy link
Member Author

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you want to do wrt the two assertions that are firing. Currently I'm ignoring them w/ the hope that it's a non-event for the generated API surface.

tool/generator/translator.dart Show resolved Hide resolved
tool/generator/bcd.dart Outdated Show resolved Hide resolved
tool/generator/translator.dart
@@ -526,10 +526,12 @@ class Translator {
for (final include in _includes) {
final target = include.target;
final includes = include.includes;
assert(_interfacelikes.containsKey(target));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yup, both assertions fired. This is something I didn't track down - why they're firing and what that means for the generated API.

I added a bit more diagnostics to see when they're triggering:

not in partial interface likes: PaintRenderingContext2D
not in partial interface likes: CSSPseudoElement
not in partial interface likes: NetworkInformation
not in partial interface likes: Bluetooth
not in partial interface likes: BluetoothDevice
not in partial interface likes: BluetoothRemoteGATTService
not in partial interface likes: BluetoothRemoteGATTCharacteristic
not in partial interface likes: GPUDevice
not in partial interface likes: GPUBuffer
not in partial interface likes: GPUTexture
not in partial interface likes: GPUTextureView
not in partial interface likes: GPUExternalTexture
not in partial interface likes: GPUSampler
not in partial interface likes: GPUBindGroupLayout
not in partial interface likes: GPUBindGroup
not in partial interface likes: GPUPipelineLayout
not in partial interface likes: GPUShaderModule
not in partial interface likes: GPUComputePipeline
not in partial interface likes: GPURenderPipeline
not in partial interface likes: GPUCommandBuffer
not in partial interface likes: GPUCommandEncoder
not in partial interface likes: GPUComputePassEncoder
not in partial interface likes: GPURenderPassEncoder
not in partial interface likes: GPURenderBundle
not in partial interface likes: GPURenderBundleEncoder
not in partial interface likes: GPUQueue
not in partial interface likes: GPUQuerySet


not in calculated mixins:HTMLAttributionSrcElementUtils [target=HTMLAnchorElement]
not in calculated mixins:HTMLAttributionSrcElementUtils [target=HTMLImageElement]
not in calculated mixins:HTMLAttributionSrcElementUtils [target=HTMLScriptElement]
not in calculated mixins:NavigatorBadge [target=Navigator]
not in calculated mixins:NavigatorBadge [target=WorkerNavigator]
not in calculated mixins:Region [target=Element]
not in calculated mixins:NavigatorDeviceMemory [target=Navigator]
not in calculated mixins:NavigatorDeviceMemory [target=WorkerNavigator]
not in calculated mixins:ARIAMixin [target=ElementInternals]
not in calculated mixins:NavigatorNetworkInformation [target=Navigator]
not in calculated mixins:NavigatorNetworkInformation [target=WorkerNavigator]
not in calculated mixins:HTMLSharedStorageWritableElementUtils [target=HTMLIFrameElement]
not in calculated mixins:HTMLSharedStorageWritableElementUtils [target=HTMLImageElement]
not in calculated mixins:NavigatorStorageBuckets [target=Navigator]
not in calculated mixins:NavigatorStorageBuckets [target=WorkerNavigator]
not in calculated mixins:NavigatorUA [target=Navigator]
not in calculated mixins:NavigatorUA [target=WorkerNavigator]
not in calculated mixins:ARIAMixin [target=Element]
not in calculated mixins:NavigatorAutomationInformation [target=Navigator]
not in calculated mixins:NavigatorGPU [target=Navigator]
not in calculated mixins:NavigatorGPU [target=WorkerNavigator]
not in calculated mixins:NavigatorML [target=Navigator]
not in calculated mixins:NavigatorML [target=WorkerNavigator]

@srujzs
Copy link
Contributor

srujzs commented Feb 5, 2024

Let me know what you want to do wrt the two assertions that are firing. Currently I'm ignoring them w/ the hope that it's a non-event for the generated API surface.

I think it's fine for now and judging from the generated output, it isn't changing the output of libraries that are still generated.

@srujzs
Copy link
Contributor

srujzs commented Feb 5, 2024

Nevermind, I lied! It does look like some stuff has changed. Let me patch this PR and play around with it.

@srujzs
Copy link
Contributor

srujzs commented Feb 5, 2024

Okay, sorry for the back and forth. I think this is okay. The interfaces and mixins belong to specs that we don't want to emit, so their removal from the libraries that we still generate is okay. So not only are we not generating non-standard libraries, we're also not generating members that correspond to non-standard partial interfaces and mixins that were mixed in. This is fine, and even desired long-term.

@devoncarew
Copy link
Member Author

Okay, sorry for the back and forth. I think this is okay. The interfaces and mixins belong to specs that we don't want to emit, so their removal from the libraries that we still generate is okay. So not only are we not generating non-standard libraries, we're also not generating members that correspond to non-standard partial interfaces and mixins that were mixed in. This is fine, and even desired long-term.

Ok, great, thanks for the investigation. I'll update the comments around this bit of code to reflect the above.

@devoncarew devoncarew merged commit 3f1285e into main Feb 6, 2024
11 checks passed
@devoncarew devoncarew deleted the reduce_libraries branch February 6, 2024 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srujzs srujzs srujzs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@devoncarew @srujzs