Boris Zbarsky [:bzbarsky] Reporter
	Description • 10 years ago

Right now we only put things that are Exposed=Workers into RegisterWorkerBindings.

We should instead consider putting in everything that's defined in any worker (e.g. Exposed=SharedWorker) but auto-generating IsEnabled functions based on the worker type.

	Ben Kelly [:bkelly, not reviewing]
	Comment 1 • 10 years ago

Maybe not a true blocker, but link this up to the meta bug so we don't lose it.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 2 • 10 years ago

Generates (IsFooWorker()) for every binding before the call to its ConstructorEnabled().

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 3 • 10 years ago

So I was thinking through this and I may have led you astray.

With this setup, the descriptor also needs to be flagged as worker-only in Bindings.conf to avoid it getting set up on Window globals, right?

If we were adding conditions to ConstructorEnabled instead, then we wouldn't need to touch Bindings.conf: the ConstructorEnabled check would handle things.

Thoughts?  

If we decide to go this route, we could change isExposedConditionally() to check for cases like "non-worker descriptor with interface not exposed on Window" or "descriptor exposed on some, but not all, workers" and return true if so, and change ConstructorEnabled to consider exposure sets.

Apart from that, I think we should add an API on IDLInterface to ask it whether it's exposed in any workers instead of assuming that _exposureGlobalNames that contain "Worker" mean it is.  Specifically, the interface could look at self.parentScope.globalNameMapping["Worker"] and intersect that with self.exposureSet.  If the intersection is non-empty, the interface is exposed in _some_ worker and we should put it in RegisterWorkerBindings.

In any case, isExposedInAllWorkers should be renamed to isExposedInAnyWorker, right?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 4 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #3)
> So I was thinking through this and I may have led you astray.
> 
> With this setup, the descriptor also needs to be flagged as worker-only in
> Bindings.conf to avoid it getting set up on Window globals, right?

Correct.
> 
> If we were adding conditions to ConstructorEnabled instead, then we wouldn't
> need to touch Bindings.conf: the ConstructorEnabled check would handle
> things.
> 
> Thoughts?

Yea I'll try this approach.
> 
> If we decide to go this route, we could change isExposedConditionally() to
> check for cases like "non-worker descriptor with interface not exposed on
> Window" or "descriptor exposed on some, but not all, workers" and return
> true if so, and change ConstructorEnabled to consider exposure sets.

Is this just a note or something that affects this bug?
> 
> Apart from that, I think we should add an API on IDLInterface to ask it
> whether it's exposed in any workers instead of assuming that
> _exposureGlobalNames that contain "Worker" mean it is.  Specifically, the
> interface could look at self.parentScope.globalNameMapping["Worker"] and
> intersect that with self.exposureSet.  If the intersection is non-empty, the
> interface is exposed in _some_ worker and we should put it in
> RegisterWorkerBindings.
> 
> In any case, isExposedInAllWorkers should be renamed to
> isExposedInAnyWorker, right?

Right.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 5 • 10 years ago

I'm confused about how to check for the various globals inside the ConstructorEnabled() body. Do I just unwrap the global passed to the function and check if it is the type required?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 6 • 10 years ago

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)
> Created attachment 8479513 [details] [diff] [review]
> Make Exposed=SpecificWorkerType work automatically
> 
> I'm confused about how to check for the various globals inside the
> ConstructorEnabled() body. Do I just unwrap the global passed to the
> function and check if it is the type required?

Especially, how do I check for window? afaik on the main thread the global can also be a sandbox.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 7 • 10 years ago

> Is this just a note or something that affects this bug?

It was an implementation suggestion for this bug.

> afaik on the main thread the global can also be a sandbox.

Good catch.  So what I think we should do is the following, for now:

1)  Check for "Window" by doing NS_IsMainThread().
2)  Check for workers as needed by comparing the js::GetObjectClass()->name of the global
    to the things in the interface.exposureSet.

and I'll think a bit more about how to handle various non-Window mainthread globals in bug 1048698.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 8 • 10 years ago

Comment on attachment 8479513 [details] [diff] [review]
Make Exposed=SpecificWorkerType work automatically

This is missing the codegen bits, but the part that's here is good.  ;)

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 10 • 10 years ago

Relevant try run
https://tbpl.mozilla.org/?tree=Try&rev=7ac9f4ab4b22

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 11 • 10 years ago

Comment on attachment 8480164 [details] [diff] [review]
Make Exposed=SpecificWorkerType work automatically

>+          exposedInWindowCheck = fill("""

You probably want dedent(), not fill(), and put the """ on the next line (which will also auto-indent better to make it clearer that the "if" is not Python code).  It should end up looking something like this:

          exposedInWindowCheck = dedent(
              """
              if (NS_IsMainThread()) {
                return false;
              }
              """)

>+            workerGlobals = filter(lambda x: x.find('WorkerGlobalScope') > 0, iface.exposureSet)

I would really prefer it if this were something you asked the IDLInterface, because the IDLInterface can directly tell you this information.  Specifically, this is just the set whose length isExposedInAnyWorker is checking in your implementation, no?

>+            workerCondition = CGList((CGGeneric('strcmp(name, "%s")'% workerGlobal) for workerGlobal in workerGlobals), " && ")

You want sorted(workerGlobals) so codegen is deterministic.  And maybe a linebreak before "for" to not end up with an overlong line.

>+            exposedInWorkerCheck = fill("""

Again, """ on next line.

>+        conditionsWrapper = CGGeneric("return true;\n")
>+        if len(conditions):

How about putting that set to CGGeneric("return true;\n") in the else clause of the if?  I think that would make things clearer.


>+        return CGList([CGGeneric(exposedInWindowCheck or ''),
>+                       CGGeneric(exposedInWorkerCheck or ''),
>+                       conditionsWrapper], "\n").define()

This will produce a bunch of leading newlines when there are no exposed checks, right?  Seems like what we really want here is to create a CGList then add those CGGenerics to it conditionally.

>                     + condition)
>+
>             conditions.append(condition)

That newline addition looks odd to me.  Why do it?

>+        hasWorkerStuff = hasWorkerStuff or any(d.interface.isExposedInAnyWorker() for d in descriptors)
>         bindingHeaders["WorkerPrivate.h"] = hasWorkerStuff
>         bindingHeaders["nsThreadUtils.h"] = hasWorkerStuff

I think this will overinclude.  In particular, it will include for cases like ImageData that are exposed everywhere, right?

This should use a condition similar to the one isExposedConditionally() uses for worker bits.  Possibly factor that out into a separate method on Descriptor called from both places?

The rest looks lovely.  r=me with the above bits fixed

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 12 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e5931282ddd

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 13 • 10 years ago

Nikhil, your editor is mis-indenting Python code.  Specifically:

1)  The indentation inside dedent() for the string should have been 4-space, not 2-space. 
    Same for fill().
2)  The "for workerGlobal" bit is mis-indented: it should line up with the CGGeneric on the
    previous line.
3)  The " &&n\n" string in the definition of conditionsWrapper is totally mis-indented.
    It should line up with the '(' before CGGeneric.

Also on that previous line there should be a space between ')' and '%'.

You never answered the "That newline addition looks odd to me.  Why do it?" part above.  I think that newline makes the code pretty confusing.

The landed patch I think _underincludes_ in that it will fail to include nsThreadUtils.h in some cases when it should.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 15 • 10 years ago

Comment on attachment 8480829 [details] [diff] [review]
Patch 2 - Indentation and proper include

>+        def descriptorHasThreadChecks(desc):

This is duplicating an identical check that's part of isConditionallyExposed.  Could we make this a method on Descriptor and call it from both places?

r=me with that.  Thanks!

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 16 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/58c44d7982af

	Carsten Book [:Tomcat]
	Comment 17 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/8e5931282ddd
https://hg.mozilla.org/mozilla-central/rev/58c44d7982af