Skip to content

Feature/add webworkers api widl #455

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
Closed
wants to merge 11 commits into from
Closed

Feature/add webworkers api widl #455

wants to merge 11 commits into from

Conversation

a-tarasyuk
Copy link
Contributor

mhegazy
mhegazy reviewed Apr 26, 2018
View reviewed changes
baselines/dom.generated.d.ts Outdated
@@ -16632,5 +16657,5 @@ type VRDisplayEventReason = "mounted" | "navigation" | "requested" | "unmounted"
type VREye = "left" | "right";
type VideoFacingModeEnum = "user" | "environment" | "left" | "right";
type VisibilityState = "hidden" | "visible" | "prerender" | "unloaded";
type WorkerType = "classic" | "module";
type WorkerType = "classic" | "module" | "classic" | "module";
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be filtered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

mhegazy
mhegazy reviewed Apr 26, 2018
View reviewed changes
baselines/webworker.generated.d.ts Outdated
declare function clearInterval(handle: number): void;
declare function clearTimeout(handle: number): void;
declare function importScripts(...urls: string[]): void;
declare function setImmediate(handler: any, ...args: any[]): number;
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these removed?

Copy link
Contributor

@saschanaz saschanaz Apr 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Because widlprocess does not support [Global] yet 😅 #456

Copy link
Contributor

Choose a reason for hiding this comment

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

#456 is merged, please rebuild!

@a-tarasyuk a-tarasyuk force-pushed the feature/add-webworkers-api-widl branch from 6bd9cb6 to 2e66a38 Compare April 26, 2018 18:17
@saschanaz saschanaz mentioned this pull request Apr 26, 2018
Merged
@saschanaz
Copy link
Contributor

I was going to add full HTML spec in once, but being incremental is probably better 😁

@saschanaz
Copy link
Contributor

The multipaged worker spec lacks WindowOrWorkerGlobalScope which is the target from many specs to add global methods/properties. We may manually add an empty one.

@saschanaz
Copy link
Contributor

saschanaz commented Apr 27, 2018 •
edited
Loading

Oh, this is an oops situation. I expected the empty WindowOrWorkerGlobalScope would automatically get some types from already added specs, but:

  1. We don't support partial interface mixins yet
  2. Even if we did, specs are incorrectly using it as partial interface. I'll file issues for those specs.

@a-tarasyuk
Copy link
Contributor Author

a-tarasyuk commented Apr 27, 2018 •
edited
Loading

@saschanaz should I remove it for now?

WindowOrWorkerGlobalScope depends on many Canvas related types, like ImageBitmapSource, and many of them are experimental, I don't know should we add them know or not.

@saschanaz
Copy link
Contributor

saschanaz commented Apr 27, 2018 •
edited
Loading

It also has IndexedDB and other stable types. Worker will lose access for them without proper WindowOrWorkerGlobalScope definition. I think we should add those lost types manually for now.

(Don't forget adding implements: ["WindowOrWorkerGlobalScope"] on WorkerGlobalScope.)

@saschanaz saschanaz mentioned this pull request Apr 28, 2018
Closed
saschanaz
saschanaz suggested changes Apr 30, 2018
View reviewed changes
inputfiles/knownWorkerTypes.json Outdated
@@ -1,6 +1,126 @@
[
"ClientQueryOptions",
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to remove these 😀

Copy link
Contributor

@saschanaz saschanaz Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I meant not only that one but all additions here, because now types are autoexposed (#457). Some exceptions are for override-type.

@a-tarasyuk a-tarasyuk force-pushed the feature/add-webworkers-api-widl branch from b8f6438 to f5b57f8 Compare April 30, 2018 08:32
@a-tarasyuk a-tarasyuk force-pushed the feature/add-webworkers-api-widl branch from f5b57f8 to 72e9691 Compare April 30, 2018 08:37
mhegazy
mhegazy reviewed Apr 30, 2018
View reviewed changes
baselines/webworker.generated.d.ts
declare function importScripts(...urls: string[]): void;
declare function setImmediate(handler: any, ...args: any[]): number;
declare function setInterval(handler: any, timeout?: any, ...args: any[]): number;
declare function setTimeout(handler: any, timeout?: any, ...args: any[]): number;
Copy link
Contributor

Choose a reason for hiding this comment

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

is that expected? should setTimeout not be available for workers?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's needed, I think we should wait until we get WindowOrWorkerGlobalScope.

@a-tarasyuk a-tarasyuk closed this May 10, 2018
@a-tarasyuk a-tarasyuk deleted the feature/add-webworkers-api-widl branch May 10, 2018 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschanaz saschanaz saschanaz requested changes

@mhegazy mhegazy mhegazy left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@a-tarasyuk @saschanaz @mhegazy