709569 - Create some tool to generate helper code for webidl dictionary value reading

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

This could be done even before we have support for real webidl dictionaries.
Currently one can use normal interfaces, which have just attributes, as dictionaries.
Reading values isn't hard, but there are certain cases, like string attributes, when
the existence of attribute in the JS object need to be checked.

I'd like to have a tool which would create helper classes for dictionary handling so that
getting access to the values could be as simple as, for example
EventInitDict dict;
nsresult rv = dict.Init(cx, obj);
NS_ENSURE_SUCCESS(rv, rv);
// dict.bubbles and dict.cancelable have now valid values.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

This is on top of Bug 675884, Bug 708701 and Bug 709127 (event ctor patches),
so that I can ensure the behavior stays the same.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: example .h

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: example .cpp

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

To clarify, this is not the perfect solution, but works well enough for the cases I need
dictionaries (event ctors and mutationobserver)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 581004 [details] [diff] [review]
WIP

Kyle, feel free to comment the .py and Makefile.in parts.

Be gentle, please. This is my first python hack.
Type handling is absolute terrible, but seems to work in common cases
like various numerical types, booleans, strings, JS::Value and
interface types.

Of course using interface as dictionary is hacky itself, but would be
really nice to have some easy way to read values even before we have
real support for webidl.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Ah, looks like xpidl.py uses some similar type hacks: endswith('*')

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

But anyway, a bit better patch coming.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: export the header

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: export the header in libs::

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: make jsval -> dictionary conversion work per spec

Yet another patch.

This makes us throw if jsval isn't an object. That is required by the webidl spec.

Also, passing jsval* to helper objects makes it easier to use the
API with IDL generated methods which take jsval as parameter.
(I assume there are cases when webidl is like
void callFoo(some_dictionary_type options);
but we would implement that in idl
void callFoo(jsval options);
before getting support for all the webidl stuff)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: example .h

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: example .cpp

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: const jsval*

jsval as an IDL parameter is converted to const jsval& in C++,
so use const also in generated dictionary handling code.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: right strip

Apparently I didn't quite understand python's strip().
"nsIIDB".strip("nsI"); -> "DB" which is ofc wrong.
Noticed the problem while testing the patch for IndexDB (Bug 710380)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I should obviously use jsids, not chars*s

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: use jsids

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 581580 [details] [diff] [review]
use jsids

Or Benjamin, perhaps you could look at the python script.

It creates the kind of code I want and seems to work well for
event ctors and indexeddb (Bug 710380) and geolocation, but the python code
itself can be terrible.

Yet I'd like to get helper tool in quite soon since there are more and more
APIs using dictionaries (which in our case are just simple interfaces for now).
Especially I'd like to use the tool for certain things when cleaning up
my MutationObserver API implementation.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 581580 [details] [diff] [review]
use jsids

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

It's not going to win any beauty pageants, but it looks reasonable enough.

::: js/xpconnect/src/dictionary_helper_gen.py
@@ +373,5 @@
> +        fd.write("  }\n");
> +    fd.write("  return NS_OK;\n");
> +    fd.write("}\n\n");
> +    
> +    fd.write("nsresult\n%s::Init(JSContext* aCx, const jsval* aVal)\n" % dict_name(iface.name))

You can do

fd.write("""
  Big long multiline block of C++
""")

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 581580 [details] [diff] [review]
use jsids

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

::: js/xpconnect/src/Makefile.in
@@ +230,5 @@
> +_EXTRA_EXPORT_FILES = \
> +  DictionaryHelpers.h \
> +  $(NULL)
> +
> +libs:: $(_EXTRA_EXPORT_FILES)

Sorry, total drive-by, but you probably want export::, not libs::, right?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

No. The tool wouldn't find all the necessary files at export phase.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Nope, the tool needs IDL files that aren't installed until later in export.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

remove extra ; and use multiline write
(but not multiline (""" """) strings since that ruins code clarity)

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Example .h

Blake, could you review that the generated code looks ok.
.h file can't be much simpler.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Example .cpp

*::Init methods are extra safe by handling requests and compartments.
NS_ENSURE_STATE(JSVAL_IS_OBJECT(*aVal)) is there since WebIDL requires to throw
an exception in that case. (Unfortunately we can't easily throw the right error type in this case, but we don't throw the right error types anyway in this kinds of cases.)

*_InitInternal functions rely on xpconnect to do the right thing when getting
values (xpconnect makes especially handling of XPCOM object types easy).
HasProperty checks are needed so that default values get updated only if the
property is really there.
NS_ENSURE_SUCCESS(rv, rv); is there since Webidl requires to propagate
exceptions.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 583228 [details]
Example .h

A couple of nitpicks.

>  // If aCx or aVal is null, NS_OK is returned and 
>  // dictionary will use the default values. 

Uber-nit: "the dictionary" here and below.


>class CustomEventInit : public EventInit
>{
>public:
>  CustomEventInit() :
>    EventInit(),
>    detail(nsnull)

Why bother with the empty base constructor initializer or explicitly initializing the nsCOMPtr to NULL?

r=me with my question addressed.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 583231 [details]
Example .cpp

>static nsresult
>EventInit_InitInternal(EventInit& aDict, nsIEventInit* aIfaceObject, JSContext* aCx, JSObject* aObj)

Nit: It is odd to see a method take a JSContext not as the first parameter. Would it be possible to rearrange the parameters so that aCx is first? If not, it isn't a big deal.

>{
>  JSBool found = PR_FALSE;
>  if (JS_HasPropertyById(aCx, aObj, gDictionary_id_bubbles, &found) && found) {

If JS_HasPropertyById returns false, then we have to thrown an excpetion here. Otherwise, we risk leaving an exception sitting in aCx.

r=mrbkap with the first comment addressed and the second comment fixed.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Blake Kaplan (:mrbkap) from comment #25)
> Why bother with the empty base constructor initializer or explicitly
> initializing the nsCOMPtr to NULL?
Because it shouldn't cause harm, and it makes it clear that everything is explicitly 
initialized to some value.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

> >EventInit_InitInternal(EventInit& aDict, nsIEventInit* aIfaceObject, JSContext* aCx, JSObject* aObj)
> 
> Nit: It is odd to see a method take a JSContext not as the first parameter.
> Would it be possible to rearrange the parameters so that aCx is first? If
> not, it isn't a big deal.
I want to keep aDict as the first parameter, since that is what the function is for.

 
> If JS_HasPropertyById returns false, then we have to thrown an excpetion
> here. Otherwise, we risk leaving an exception sitting in aCx.
Will fix.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/33473863f42c