Closed
Bug 1081241
Opened 10 years ago
Closed 10 years ago
Exposed=Worker should prevent interface from showing up in RegisterBindings
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: smaug, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
9.44 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•10 years ago
|
||
Looks like Nikhil actually fixed this in bug 1048699. Specifically, this part in CGConstructorEnabled: if not iface.isExposedInWindow(): exposedInWindowCheck = dedent( """ if (NS_IsMainThread()) { return false; } """) body.append(CGGeneric(exposedInWindowCheck)) and the fact that isExposedConditionally is true if self.hasThreadChecks() and that's true if (not self.workers and not self.interface.isExposedInWindow()). So a non-worker descriptor that's not exposed in window will end up with that NS_IsMainThread check. Seems better to not register it at all, though, so I'm going to morph this bug to do that.
Summary: Exposed=Worker should prevent interface to show up in the Window scope → Exposed=Worker should prevent interface from showing up in RegisterBindings
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8523477 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8523477 [details] [diff] [review] Things that aren't exposed in Window shouldn't show up in RegisterBindings at all >+ return (self.isExposedConditionally() and >+ (not self.interface.isExposedInWindow() or > self.interface.isExposedOnlyInSomeWorkers())) is a bit hard to read, especially since self.isExposedConditionally() has magical meaning (self.interface.isExposedConditionally() + self.interface.isExposedOnlyInSomeWorkers() meaning). So why not return (self.interface.isExposedConditionally() and not self.interface.isExposedInWindow()) or self.interface.isExposedOnlyInSomeWorkers() > def isExposedOnlyInSomeWorkers(self): >- assert self.isExposedInAnyWorker() >+ if not self.isExposedInAnyWorker(): >+ return False > workerScopes = self.parentScope.globalNameMapping["Worker"] Not really about this bug but I don't understand why self.parentScope.globalNameMapping["Worker"] does what it seems to do, but apparently "Worker" term is used in rather broad way here. Also, is isExposedOnlyInSomeWorkers supposed to return true only if isExposedInWindow returns false? Or perhaps 'Only' refers to a set of possible Worker-type of globals, not all Globals The naming really could be a bit better.
Attachment #8523477 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 4•10 years ago
|
||
>So why not OK. I was basically modeling it as "we generate the IsConstructorEnabled method, and whatever conditions cause that method to use NS_IsMainThread". But your formulation is equivalent. > I don't understand why self.parentScope.globalNameMapping["Worker"] does what it seems Because the IDL for workers looks like this: [Global=(Worker,DedicatedWorker), Exposed=DedicatedWorker] interface DedicatedWorkerGlobalScope : WorkerGlobalScope { and [Global=(Worker,SharedWorker), Exposed=SharedWorker] interface SharedWorkerGlobalScope : WorkerGlobalScope { and [Global=(Worker,ServiceWorker), Exposed=ServiceWorker] interface ServiceWorkerGlobalScope : WorkerGlobalScope { In other words, all the worker global interfaces explicitly say they are Worker in addition to whatever else they are, so you can use [Exposed=Worker] to expose things in all workers. > Also, is isExposedOnlyInSomeWorkers supposed to return true only if isExposedInWindow > returns false? No. And it doesn't. What this is doing is taking the set of all worker scopes, subtracting out the scopes we're exposed in, and returning whether the result is the empty set. Since Window is not in the set of all worker scopes, it doesn't matter whether we're exposed in Window or not. The reason we need the isExposedInAnyWorker check is to not claim isExposedOnlyInSomeWorkers when we're not exposed in workers at all. > Or perhaps 'Only' refers to a set of possible Worker-type of globals Yes. > The naming really could be a bit better. I could call this isNotExposedInAllWorkers, but that would probably require going back to requiring all callers to check isExposedInAnyWorker... If you have good naming ideas here, I'm all ears.
Reporter | ||
Comment 6•10 years ago
|
||
That is quite clear (+ a comment somewhere that it doesn't say anything about exposing the interface in Window context)
Flags: needinfo?(bugs)
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8646f1c6339d with the better name and a comment.
Target Milestone: --- → mozilla36
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8646f1c6339d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•