796983 - Create a way to generate an example class declaration for a given WebIDL interface

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I was kinda against it, but people seem to want it a lot...

This will be different from what bug 787574 did in that it will create declarations for all the methods.

Comment 1

[(no longer active)]

Wait, why is this a good idea?  We can't really generate a good class without reading the programmer's mind, can we?

Comment 2

[Boris Zbarsky [:bzbarsky]]

To some extent, yes.

And I'm not _entirely_ convinced it's a good idea.  ;)

The proposal is to generate what your skeleton generator generates, plus declarations (not implementations) for the WebIDL methods based on the current annotations in the conf file and whatnot.  So this won't tell people that they can have non-refcounted return values, and it won't tell people that they can make the methods const or virtual or inline or whatnot... but it'll tell them the right arguments, for the most part, and more or less the right return values (modulo the non-refcounting bit).  From there, they can edit things as needed.

Comment 3

[(no longer active)]

(In reply to comment #2)
> To some extent, yes.
> 
> And I'm not _entirely_ convinced it's a good idea.  ;)
> 
> The proposal is to generate what your skeleton generator generates, plus
> declarations (not implementations) for the WebIDL methods based on the current
> annotations in the conf file and whatnot.  So this won't tell people that they
> can have non-refcounted return values, and it won't tell people that they can
> make the methods const or virtual or inline or whatnot... but it'll tell them
> the right arguments, for the most part, and more or less the right return
> values (modulo the non-refcounting bit).  From there, they can edit things as
> needed.

I fear that by generating method signatures, we will in practice discourage people from making them better (such as by using non-refcounted return values).  If people are just interested in knowing the expected return type and the arguments, they can just look at the generated bindings file, and as someone who has used this stuff mostly as a consumer, I think that works fine in practice.

I'm still entirely unconvinced that this will not do more harm than good.

Comment 4

[Peter Van der Beken [:peterv]]

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> I fear that by generating method signatures, we will in practice discourage
> people from making them better (such as by using non-refcounted return
> values).

Why would this make any difference as to whether people use good annotations or not? If they don't know they can annotate for non-refcounted return value (as an example) why would having the exact signature generated based on the WebIDL and annotations change anything?

Comment 5

[Peter Van der Beken [:peterv]]

BTW, you can't exactly tell the argument types from the binding file, see for example NonNull<> (we definitely don't want people using NonNull<T>& instead of T&).

Comment 6

[(no longer active)]

(In reply to comment #4)
> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > I fear that by generating method signatures, we will in practice discourage
> > people from making them better (such as by using non-refcounted return
> > values).
> 
> Why would this make any difference as to whether people use good annotations or
> not? If they don't know they can annotate for non-refcounted return value (as
> an example) why would having the exact signature generated based on the WebIDL
> and annotations change anything?

Well, it depends on what signature we generate, right?  For example, for a getter returning an object of type Class, which one of the following signatures will we generate?

 Class* Foo();
 Class* Foo() const;
 already_AddRefed<Class> Foo();
 // etc.

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
>  Class* Foo();
>  Class* Foo() const;

You didn't say anything about annotations, so I presume it's not marked as returning non-addrefed things. So none of the above, they are misleading since we'd addref the pointer anyway.

>  already_AddRefed<Class> Foo();

This, it points exactly to what happens, we take a strong reference. I'd actually personally make it

  already_AddRefed<Class> Foo() const;

people will have to remove the const if it doesn't work for them.

I'm worried about people using the wrong types here, you have to read the docs to find the right types. The docs are good, but the script can know exactly the right types based on the WebIDL and the annotations. I don't particularly care about giving a useful class (for which there are also multiple solutions, as opposed to just the one that the script from bug 787574 generates), so if you have a better idea of how to give the right types without the downsides of having precanned signatures, please share.

Comment 8

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the skeleton can look like this:

  // Annotate 'foo' as 'resultNotAddRefed' if you don't need to addref before returning
  already_AddRefed<Class> Foo() const;

I guess the second question is whether we want to output a full class a la the existing skeleton generator or just a dump of the WebIDL method/prop declarations...

Comment 9

[(no longer active)]

(In reply to comment #8)
> For what it's worth, the skeleton can look like this:
> 
>   // Annotate 'foo' as 'resultNotAddRefed' if you don't need to addref before
> returning
>   already_AddRefed<Class> Foo() const;

Yeah, other than what Boris suggests here, I can't think of better ideas, Peter.  :/

> I guess the second question is whether we want to output a full class a la the
> existing skeleton generator or just a dump of the WebIDL method/prop
> declarations...

If we're going to generate the signatures, we might as well generate the whole class, right?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Well, sort of.  If I'm just generating the signatures, I can just dump them to stdout.  If I'm doing the whole class, I should probably do separate .cpp and .h files and actually put them somewhere...  ;)

Comment 11

[(no longer active)]

(In reply to comment #10)
> Well, sort of.  If I'm just generating the signatures, I can just dump them to
> stdout.  If I'm doing the whole class, I should probably do separate .cpp and
> .h files and actually put them somewhere...  ;)

Hmm, you can just have a skeleton thing and push stuff inside it using sed or something.  :-)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Well, sure.

I mean, I can stick the .h and .cpp in the objdir.  I guess I'll just do that....

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Switch nullable primitives to using a const Nullable.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add a way to generate an example class declaration for a given WebIDL interface. mark constructors as static in the parser because they are. This

allows us to just use the isStatic() for the IDLMember to mark our
declarations static.

To generate an example interface implementation, just "make
interfacename-example" in $objdir/dom/bindings.  This will place files
called interfacename-example.h and interfacename-example.cpp in that
directory.  For example, "make XMLHttpRequest-example" will get you
$objdir/dom/bindings/XMLHttpRequest-example.h and
$objdir/dom/bindings/XMLHttpRequest-example.cpp.

Attribute getters currently default to const methods, while setters
and operations default to non-const methods.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: XMLHttpRequest-example.h as generated by the previous patch

In case you want to take a look at what output looks like.

I'm not sure whether I need to #include more stuff (like Nullable.h or BindingUtils.h to pick up Optional<T>).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: TestInterface-example.h as generated by this stuff

I manually compared this to what we have in TestBindingHeader.h, and it seems to match.  Again, in case you want to see how various edge cases are handled.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 with fix to make regression tests pass

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 668678 [details] [diff] [review]
Part 2 with fix to make regression tests pass

Looks good to me. I suspect that this will be something we'll iterate on a bit as people start to use this etc, but that should be easy once this is in the tree. It's unfortunate that there's no easy way to generate code that would actually try to call into these example classes to make sure we don't generate code that doesn't compile... Keeping this up to date will take some amount of work, especially w/o an automated way of detecting when the code generator diverges from the example generator.

Either way, I think this is a huge value add for developers using our new bindings! r=jst

Comment 19

[Boris Zbarsky [:bzbarsky]]

> It's unfortunate that there's no easy way to generate code that would actually try to
> call into these example classes to make sure we don't generate code that doesn't
> compile...

It wouldn't be too bad.  I considered switching out our TestCodeGen stuff for such a setup, then decided it would be better to leave that as-is for now.  But I could add a second testing interface which would in fact exercise the generated example.  Filed bug 802636.

Comment 20

[Boris Zbarsky [:bzbarsky]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/d8798b6331be
  https://hg.mozilla.org/integration/mozilla-inbound/rev/50d6355b80c4

Comment 21

[Boris Zbarsky [:bzbarsky]]

Added some basic documentation.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8798b6331be
https://hg.mozilla.org/mozilla-central/rev/50d6355b80c4

Comment 23

[Peter Van der Beken [:peterv]]

Comment on attachment 668678 [details] [diff] [review]
Part 2 with fix to make regression tests pass

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

Looks great.

::: dom/bindings/Codegen.py
@@ +5757,5 @@
> +
> +    def define(self):
> +        static = "static " if self.member.isStatic() else ""
> +        # Mark our getters, which are attrs that have a non-void return type,
> +        # as const.

Except if they have a type that's passed as an outparam?

@@ +5923,5 @@
> +        if type.isObject():
> +            if type.nullable():
> +                declType = "%s*"
> +            else:
> +                if optional:

Nit: elif

@@ +6017,5 @@
> +        if descriptor.wrapperCache:
> +            wrapFunc = ("  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope,\n"
> +                        "                               bool* aTriedToWrap);\n")
> +        else:
> +            wrapFunc = "  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope);\n"

Should we add a comment that this only needs to be virtual if this might be subclassed?

@@ +6021,5 @@
> +            wrapFunc = "  virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope);\n"
> +
> +        classDecl = CGWrapper(
> +            CGGeneric("public nsISupports,\n"
> +                      "public nsWrapperCache"),

This makes it inherit from nsWrapperCache even if wrapperCache is False, should we add a comment about how it can be non-nsWrapperCache or even non-nsISupports?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Except if they have a type that's passed as an outparam?

No, why would that prevent the method from being const?

> Nit: elif

Will do.

> Should we add a comment that this only needs to be virtual if this might be subclassed?

Actually, going to just make it non-virtual for non-wrappercached things.  That's really easy with the refactored codegen after bug 779048 whereas a comment is hard.

> This makes it inherit from nsWrapperCache even if wrapperCache is False,

Hmm.  Filed bug 808694 on making this actually work right and put in comments for nwow.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5f887e05fc16 to address comment 23.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f887e05fc16