Closed Bug 1121688 Opened 5 years ago Closed 5 years ago
Remove the need for hacky skipgen worker descriptors
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.
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), 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?
I meant for members returning a non-worker object (sig).
> I meant for members returning a non-worker object (sig). 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) for sig in signatures) signatures = filter( lambda sig: all(typeExposedInWorkers(arg.type) for arg in sig), 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?
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.
Target Milestone: --- → mozilla38
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.