Closed Bug 1073124 Opened 10 years ago Closed 10 years ago

Improve JSM WebIDL ergonomics and Implement [Exposed=System]

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bholley, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files)

Our current mechanism for using WebIDL classes in JSMs and JS XPCOM components kinda sucks for two reasons:

1) Callers need to do Cu.importGlobalProperties['ClassName'] before using it.
2) Implementors need to go manually add a call to FooBinding::GetConstructorObject and associated machinery in xpc::GlobalProperties.

(1) is a policy decision which I'm happy to reverse for BackstagePass globals at this point.

(2) Can be fixed by implementing [Exposed=System].

I'm wary in general about making it too easy to expose classes to non-Window scopes, because DOM implementations often make assumptions about the scope being a DOM scope. But I'm ok with it now that we have a strict review policy for dom/webidl, so long as we make it crystal clear to all DOM peers that [Exposed=System] requires (a) an audit of the code to make sure it doesn't depend on Windows and Documents and (b) simple xpcshell test coverage.

We'd basically need to generated something like RegisterWorkerBindings.cpp called RegisterSystemBindings.cpp or somesuch. Unlike workers, I think we should do lazy resolution here, otherwise each additional constructor's memory footprint will be multiplied by ~200.

Once we do that, we just need to invoke it from BackstagePass::NewResolve and we should be good to go.
Kyle is going to find someone to own this.
Flags: needinfo?(khuey)
- "System" is a bit vague. What does that mean? TabChildGlobal? JS components?
- what code does (a) talk about?


Other than that, sound good.
(In reply to Olli Pettay [:smaug] from comment #2)
> - "System" is a bit vague. What does that mean? TabChildGlobal? JS
> components?

I was thinking JSMs, JS XPCOM components and (now that you mention it) TabChildGlobal. I'm open to better naming suggestions.

> - what code does (a) talk about?

Not sure I understand your question. I'm saying that we want to make sure that the binding implementation never assumes that the global is an nsGlobalWindow before we expose it to non-DOM globals.
(In reply to Bobby Holley (:bholley) from comment #3)
> Not sure I understand your question. I'm saying that we want to make sure
> that the binding implementation never assumes that the global is an
> nsGlobalWindow before we expose it to non-DOM globals.
And you answered here. You're talking about the implementation of some webidl interface.
Note, so far .webidl reviews haven't required DOM peer to review all of the implementation, but
it has been there to make sure we expose only the right stuff to the web etc.
(In reply to Olli Pettay [:smaug] from comment #4)
> Note, so far .webidl reviews haven't required DOM peer to review all of the
> implementation, but
> it has been there to make sure we expose only the right stuff to the web etc.

And they still won't for the most part, unless somebody is exposing an interface to non-Window scopes. It should be a pretty simple 20-second grep, and tbh I care more about the test coverage anyway.
Bug 1048698 was suggesting calling this Exposed="XPConnect" or Exposed="NonDOM", right?  Is that bug just a duplicate of this one?
(In reply to Boris Zbarsky [:bz] from comment #6)
> Bug 1048698 was suggesting calling this Exposed="XPConnect" or
> Exposed="NonDOM", right?  Is that bug just a duplicate of this one?

Yes. Kyle asked me to file a bug and I couldn't find the other one.
Depends on: 1082843
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8505028 - Flags: review?(bobbyholley)
Flags: needinfo?(khuey)
Attachment #8505028 - Flags: review?(bobbyholley) → review+
Blocks: 939636
Whiteboard: [need review]
Comment on attachment 8505027 [details] [diff] [review]
part 2.  Define Exposed=System things in BackstagePass::NewResolve as needed

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

I keep glancing at this patch and seeing the Codegen.py changes and thinking "ugh". If they need me to really look at them, please upload the generated code, or let me know that I should look at the Codegen more carefully. Otherwise, I trust you. Sorry for holding this up so long before deciding that. r=me

::: dom/bindings/Codegen.py
@@ +14454,5 @@
>  
>      @staticmethod
> +    def ResolveSystemBinding(config):
> +
> +        # TODO - Generate the methods we want

TODO?

::: js/xpconnect/src/XPCRuntimeService.cpp
@@ +71,1 @@
>      *_retval = ResolveWorkerClasses(cx, obj, id, &objp);

Can you file a followup to remove this part in favor of Exposed=System?
Attachment #8505027 - Flags: review?(bobbyholley) → review+
> If they need me to really look at them, please upload the generated code

Here you go.  I hacked a few interfaces (ImageData, WebGLBuffer) to be Exposed=System so you can see what the overall code flow looks like.
Attachment #8506944 - Flags: review?(bobbyholley)
Attachment #8506944 - Attachment mime type: text/x-c++src → text/plain
Comment on attachment 8506944 [details]
ResolveSystemBinding.cpp

I'll trust you on making sure that the ConstructorEnabled calls are happening in the right places. Looks good in general. Thanks!
Attachment #8506944 - Flags: review?(bobbyholley) → review+
> Can you file a followup to remove this part in favor of Exposed=System?

Ah, this caught an actual bug.  I wasn't doing anything for this stuff in BackstagePass::Enumerate.

I added the same setup as ResolveWorkerClasses: passing JSID_VOID as the handle will make it match everything, and added a call like that to BackstagePass::Enumerate.
(In reply to Boris Zbarsky [:bz] from comment #16)
> > Can you file a followup to remove this part in favor of Exposed=System?
> 
> Ah, this caught an actual bug.  I wasn't doing anything for this stuff in
> BackstagePass::Enumerate.

Doh, sorry for missing that in review.
In any case, the TODO is removed and bug 1084439 is filed.
Blocks: 1084439
Blocks: 781615
Anyone want to document this in MDN?
You need to log in before you can comment on or make changes to this bug.