Closed Bug 1121688 Opened 5 years ago Closed 5 years ago

Remove the need for hacky skipgen worker descriptors

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 3 obsolete files)

We only need these for cases when an overload for a method on a worker binding takes some type that's not exposed in workers.

But we can just filter such overloads out during codegen.  Patch coming up.
Attachment #8549171 - Attachment is obsolete: true
Attachment #8549171 - Flags: review?(peterv)
Attachment #8549172 - Attachment is obsolete: true
Attachment #8549172 - Flags: review?(peterv)
Attachment #8549173 - Attachment is obsolete: true
Attachment #8549173 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8549177 [details] [diff] [review]
Filter out overloads with non-worker-exposed arguments when generating worker bindings

Review of attachment 8549177 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: dom/bindings/Codegen.py
@@ +6874,5 @@
> +            if descriptor.workers:
> +                # Filter out the signatures that should not be exposed in a worker
> +                signatures = filter(
> +                    lambda sig: all(typeExposedInWorkers(arg.type)
> +                                    for arg in sig[1]),

Shouldn't we care about the return value?
Attachment #8549177 - Flags: review?(peterv) → review+
Which return value?  The return value of a lambda in Python is the last expression in the lambda, no?
Flags: needinfo?(peterv)
I meant for members returning a non-worker object (sig[0]).
Flags: needinfo?(peterv)
> I meant for members returning a non-worker object (sig[0]).

Oh, I see.  That actually can't happen: we have checks in the parser that the return type's exposure set is a superset of the member's exposure set.

So for example if I add an overload like this to XMLHttpRequest:

  Document send(XMLHttpRequest data);

I get an IDL parser error:

  WebIDL.WebIDLError: error: Overload returns a type that is not exposed everywhere where the method is exposed, /Users/bzbarsky/mozilla/inbound/mozilla/dom/webidl/XMLHttpRequest.webidl line 106:2
I'll add an assert, though, in case we ever change that.
                # Filter out the signatures that should not be exposed in a
                # worker.  The IDL parser enforces the return value being
                # exposed correctly, but we have to check the argument types.
                assert all(typeExposedInWorkers(sig[0]) for sig in signatures)
                signatures = filter(
                    lambda sig: all(typeExposedInWorkers(arg.type)
                                    for arg in sig[1]),
                    signatures)
That caught an issue: InstallEvent and InstallPhaseEvent are exposed in Window only but have workers:True descriptors for some reason that escapes me, so fail that assert for the constructor.  I see no reason for that workers:True in the descriptors.  Nikhil, any objections to me just removing it?
Flags: needinfo?(nsm.nikhil)
You can remove them, I won't be able to land Bug 1121688 for a couple of days. ServiceWorkers are preffed off, so any changes you make won't matter.
Flags: needinfo?(nsm.nikhil)
https://hg.mozilla.org/mozilla-central/rev/8ecb3619e491
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1127341
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.