Closed Bug 947487 Opened 7 years ago Closed 7 years ago

Generate js::Class structs for DOM proxies.

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: efaust, Assigned: efaust)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 2 obsolete files)

At present, we have a way more complicated structure than we need to for WebIDL stuff implemented as proxies. In particular, the DOMJSClass and DOMClass structs hanging from different places. This is because prior to bug 924720, there was no way to create custom js::Class structs corresponding to proxies.

Now that this is no longer the case, we can do away with DOMClass and unify proxy and non-proxy objects under DOMJSClass.
Depends on: 924720
In order to generate classes for proxies, we need to generate js::Classes, not JSClasses. Make the appropriate changes.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8344934 - Flags: review?(bzbarsky)
With name changes discussed on IRC
Attachment #8344934 - Attachment is obsolete: true
Attachment #8344934 - Flags: review?(bzbarsky)
Attachment #8344937 - Flags: review?(bzbarsky)
Comment on attachment 8344937 [details] [diff] [review]
Part 1: Convert DOMJSClass::mBase from JSClass to js::Class

r=me, but maybe the FromJSClass functions should be fixed too?  That is, do the cast in the one that takes js::Class and have the other call it?
Attachment #8344937 - Flags: review?(bzbarsky) → review+
Attachment #8368332 - Flags: review?(bzbarsky)
Now with more audited IsDOMClass callers.
Attachment #8368332 - Attachment is obsolete: true
Attachment #8368332 - Flags: review?(bzbarsky)
Attachment #8368443 - Flags: review?(bzbarsky)
Comment on attachment 8368443 [details] [diff] [review]
Part 2: Generate and use js::Class structs for DOM proxies

>+++ b/dom/bindings/BindingUtils.cpp

In NativeToString, can we now drop the IsDOMProxy branch?  Seems like we can as long as clasp->name is correct for our DOM proxy classes, since the IsDOMClass() branch will do the right thing there.

>+++ b/dom/bindings/BindingUtils.h
>@@ -524,7 +528,7 @@
>-  if (dom::IsDOMClass(clasp)) {
>+  if (dom::IsNonProxyDOMClass(clasp)) {

Actully, if you leave that as IsDOMClass(clasp) but check for clasp->isProxy() (just like we check for global) before entering that inner branch, you could get rid of the end-of-method IsDOMProxy() call in this method and just return false there, right?

If we get rid of these two IsDOMProxy() callsites, the only remaining IsDOMProxy() uses will be in DOMJSProxyHandler.h/cpp, I believe.  And the two-argument version of IsDOMProxy will be dead code.

>diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py
>--- a/dom/bindings/Codegen.py
>+++ b/dom/bindings/Codegen.py
>+class CGDOMProxyJSClass(CGThing):
>+static const DOMJSClass Class = {
>+    PROXY_CLASS_DEF("DOMProxy",

How about self.descriptor.interface.identifier.name for the name so we can make the NativeToString stuff above work?

Also, outdent this by two spaces, please.

>+    %s
>+};
>+""" % CGIndenter(CGGeneric(DOMClass(self.descriptor))).define()

Outdent the %s all the way, please; the CGIndenter will indent the text as needed.  The string replacing that %s is multiline, so the current setup just gives the first line a weird indent.

The rest looks great.  r=me with the above nits.
Attachment #8368443 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/bedbbb50c106
https://hg.mozilla.org/mozilla-central/rev/ed63a08515a4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.