763643 - Implement static operations for WebIDL

Status: RESOLVED FIXED

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

Description

[William Chen [:wchen]]

http://dev.w3.org/2006/webapi/WebIDL/#idl-static-operations

Comment 1

[Johnny Stenback (:jst)]

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?

Comment 2

[Peter Van der Beken [:peterv]]

Probably need both, I think static operations are hooked up in the parser but need codegen support and static attributes need both.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 4

[Peter Van der Beken [:peterv]]

I ended up passing the global to the static methods and getters and setters, I suspect that we'll need that in most cases.

Comment 5

[Peter Van der Beken [:peterv]]

Still need to enforce that stringifier attributes have DOMString as the type and that there's only one stringifier on an interface.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 7

[William Chen [:wchen]]

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"?

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 9

[Boris Zbarsky [: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.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v2.1

(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).

Comment 11

[Peter Van der Beken [:peterv]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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)?

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

roc, this might help

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The patch doesn't apply cleanly.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I need this to implement URL.createObjectURL properly.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: rebased patch

I rebased the patch to trunk. I'm not 100% confident I rebased it correctly but it seems to be working.

Comment 17

[Boris Zbarsky [:bzbarsky]]

roc points out in bug 792675 that this patch fails if !hasInterfacePrototypeObject(), because then we never even check for static methods...

Comment 18

[Boris Zbarsky [:bzbarsky]]

There's a patch in bug 792675 that fixes that; we can just land it as a followup.

Comment 19

[Josh Aas]

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

It's blocked on bug 778152, as the deps say.

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: v2.1 rebased patch

Bug 778152 was finally landed, so I'll check this in tomorrow.

Comment 22

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6fb95ba0a20

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6fb95ba0a20

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thank you!