Closed Bug 763643 Opened 7 years ago Closed 7 years ago

Implement static operations for WebIDL

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: wchen, Assigned: peterv)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 4 obsolete files)

What needs to be done here? Do we have WebIDL parser support for this and just need codegen work, or do we need parser work as well as codegen work?
Probably need both, I think static operations are hooked up in the parser but need codegen support and static attributes need both.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
I ended up passing the global to the static methods and getters and setters, I suspect that we'll need that in most cases.
Still need to enforce that stringifier attributes have DOMString as the type and that there's only one stringifier on an interface.
Comment on attachment 643955 [details] [diff] [review]
v1

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

::: dom/bindings/Codegen.py
@@ +3097,5 @@
> +            unwrap = CGGeneric("""nsISupports* global;
> +xpc_qsSelfRef globalRef;
> +{
> +  nsresult rv;
> +  JS::Value val = OBJECT_TO_JSVAL(obj);

obj is the this object; you need JS_GetGlobalForObject(cx, obj).

@@ +3098,5 @@
> +xpc_qsSelfRef globalRef;
> +{
> +  nsresult rv;
> +  JS::Value val = OBJECT_TO_JSVAL(obj);
> +  rv = xpc_qsUnwrapArg<nsISupports>(cx, val, &global, &globalRef.ptr, &val);

Declare nsresult here?
The patch does not build on the linux try machines. It gives the following error:

/builds/slave/try-lnx64/build/obj-firefox/config/nsinstall -R -m 644 /builds/slave/try-lnx64/build/dom/webidl/XMLHttpRequestEventTarget.webidl .
/builds/slave/try-lnx64/build/obj-firefox/config/nsinstall -R -m 644 /builds/slave/try-lnx64/build/dom/webidl/XMLHttpRequestUpload.webidl .
/builds/slave/try-lnx64/build/obj-firefox/config/nsinstall -R -m 644 /builds/slave/try-lnx64/build/dom/webidl/WebGLRenderingContext.webidl .
/builds/slave/try-lnx64/build/obj-firefox/config/nsinstall -R -m 644 /builds/slave/try-lnx64/build/dom/bindings/test/TestCodeGen.webidl .
/builds/slave/try-lnx64/build/obj-firefox/config/nsinstall -R -m 644 /builds/slave/try-lnx64/build/dom/bindings/test/TestDictionary.webidl .
PYTHONDONTWRITEBYTECODE=1 /builds/slave/try-lnx64/build/obj-firefox/_virtualenv/bin/python /builds/slave/try-lnx64/build/config/pythonpath.py \
    -I/builds/slave/try-lnx64/build/other-licenses/ply -I/builds/slave/try-lnx64/build/dom/bindings/parser \
    /builds/slave/try-lnx64/build/dom/bindings/GlobalGen.py  /builds/slave/try-lnx64/build/dom/bindings/Bindings.conf . \
    --cachedir=_cache \
    CanvasRenderingContext2D.webidl Function.webidl EventListener.webidl EventTarget.webidl Notification.webidl Performance.webidl PerformanceNavigation.webidl PerformanceTiming.webidl XMLHttpRequest.webidl XMLHttpRequestEventTarget.webidl XMLHttpRequestUpload.webidl  WebGLRenderingContext.webidl  TestCodeGen.webidl TestDictionary.webidl 
Traceback (most recent call last):
  File "/builds/slave/try-lnx64/build/config/pythonpath.py", line 56, in <module>
    main(sys.argv[1:])
  File "/builds/slave/try-lnx64/build/config/pythonpath.py", line 48, in main
    execfile(script, frozenglobals)
  File "/builds/slave/try-lnx64/build/dom/bindings/GlobalGen.py", line 10, in <module>
    import WebIDL
  File "/builds/slave/try-lnx64/build/dom/bindings/parser/WebIDL.py", line 2816
    p[0] = IDLAttribute(*p[2], static=static, stringifier=stringifier)
                                    ^
SyntaxError: invalid syntax
make[6]: *** [ParserResults.pkl] Error 1
make[6]: Leaving directory `/builds/slave/try-lnx64/build/obj-firefox/dom/bindings'
make[5]: *** [export] Error 2
make[5]: Leaving directory `/builds/slave/try-lnx64/build/obj-firefox/dom'
make[4]: *** [export_tier_platform] Error 2
make[4]: Leaving directory `/builds/slave/try-lnx64/build/obj-firefox'
make[3]: *** [tier_platform] Error 2
make[3]: Leaving directory `/builds/slave/try-lnx64/build/obj-firefox'
make[2]: *** [default] Error 2
make[2]: Leaving directory `/builds/slave/try-lnx64/build/obj-firefox'
make[1]: *** [realbuild] Error 2
make[1]: Leaving directory `/builds/slave/try-lnx64/build'
make: *** [build] Error 2
program finished with exit code 2
elapsedTime=124.245784

Maybe use a name other than "static"?
Blocks: 594543
Blocks: 781400
Attached patch v2 (obsolete) — Splinter Review
Attachment #643955 - Attachment is obsolete: true
Attachment #650543 - Flags: review?(jst)
Attachment #650543 - Flags: review?(bzbarsky)
Comment on attachment 650543 [details] [diff] [review]
v2

>+++ b/dom/bindings/Codegen.py
> class AttrDefiner(PropertyDefiner):
>+            if self.static:
>+                jitinfo = "nullptr"
>+            else:
>+                jitinfo = "&%s_getterinfo" % attr.identifier.name
>+            return "{ (JSPropertyOp)genericGetter, %s }" % jitinfo

This looks wrong to me, because the genericGetter pulls the function pointer to call for the underlying getter out of the jitinfo, no?  Furthermore, the genericGetter expects the right sort of this object and whatnot...  Seems like you want a different method name here in the static case, just like for methods.

Same thing for setters.

>+class CGAbstractBindingStaticMethod(CGAbstractStaticMethod):

Would CGAbstractStaticBindingMethod make more sense?

>+  val.setObjectOrNull(JS_GetGlobalForObject(cx, obj));

Will this work reasonably with Xrays?  Seems like obj might be an Xray here, or is that not possible?

> class CGGenericMethod(CGAbstractBindingMethod):
>         CGAbstractBindingMethod.__init__(self, descriptor, 'genericMethod', args)
>-
>     def generate_code(self):

Why remove that blank line?

The rest looks fine.  r=me with the above things dealt with.
Attachment #650543 - Flags: review?(bzbarsky) → review+
Attached patch v2.1 (obsolete) — Splinter Review
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #9)
> Seems
> like you want a different method name here in the static case, just like for
> methods.
> 
> Same thing for setters.

Done.

> Would CGAbstractStaticBindingMethod make more sense?

Done. I went back and forth between the two.

> Will this work reasonably with Xrays?  Seems like obj might be an Xray here,
> or is that not possible?

xpc_qsUnwrapArg deals with security wrappers.

> Why remove that blank line?

Done.

Turns out there was an issue with the new enum though, IDLMethod.Special's enum values overlapped with IDLInterfaceMember.Special's, and we pass both in the same array. I ended up adding support for a base enum to enum (and enums use the base enum's length as the start value).
Attachment #650543 - Attachment is obsolete: true
Attachment #650543 - Flags: review?(jst)
Attachment #650890 - Flags: review+
Comment on attachment 650890 [details] [diff] [review]
v2.1

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

Kyle, I want to make sure you're ok with the changes to enum in WebIDL.py. Note that length is a bit odd in that it becomes the sum of the number of enum values of the class and the base classes.
Attachment #650890 - Flags: review?(khuey)
> xpc_qsUnwrapArg deals with security wrappers.

Yes, but you're getting the global first, so won't you end up with the wrong global (not a security wrapper; just the wrong window)?
roc, this might help
The patch doesn't apply cleanly.
I need this to implement URL.createObjectURL properly.
Attached patch rebased patch (obsolete) — Splinter Review
I rebased the patch to trunk. I'm not 100% confident I rebased it correctly but it seems to be working.
roc points out in bug 792675 that this patch fails if !hasInterfacePrototypeObject(), because then we never even check for static methods...
There's a patch in bug 792675 that fixes that; we can just land it as a followup.
Blocks: 782211
Has this work stalled out? We need this for the W3C Web Notifications patches and I'd like for W3C Web Notifications to land soon.
It's blocked on bug 778152, as the deps say.
Bug 778152 was finally landed, so I'll check this in tomorrow.
Attachment #650890 - Attachment is obsolete: true
Attachment #663290 - Attachment is obsolete: true
Attachment #676322 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c6fb95ba0a20
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.