Last Comment Bug 709569 - Create some tool to generate helper code for webidl dictionary value reading
: Create some tool to generate helper code for webidl dictionary value reading
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86_64 All
: -- normal (vote)
: mozilla12
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
Depends on: 723894
Blocks: 710882 710380
  Show dependency treegraph
 
Reported: 2011-12-11 04:47 PST by Olli Pettay [:smaug] (vacation Aug 25-28)
Modified: 2012-02-06 01:32 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (45.35 KB, patch)
2011-12-12 12:23 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
example .h (2.33 KB, text/plain)
2011-12-12 12:25 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details
example .cpp (10.65 KB, text/plain)
2011-12-12 12:25 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details
export the header (44.66 KB, patch)
2011-12-13 06:11 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
export the header in libs:: (44.39 KB, patch)
2011-12-13 07:06 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
make jsval -> dictionary conversion work per spec (47.41 KB, patch)
2011-12-13 08:49 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
example .h (3.08 KB, text/plain)
2011-12-13 12:05 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details
example .cpp (11.27 KB, text/plain)
2011-12-13 12:05 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details
const jsval* (47.42 KB, patch)
2011-12-13 12:24 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
right strip (47.67 KB, patch)
2011-12-13 13:39 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
use jsids (50.01 KB, patch)
2011-12-14 03:52 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
khuey: review+
Details | Diff | Splinter Review
patch (49.93 KB, patch)
2011-12-20 11:18 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
Example .h (3.12 KB, text/plain)
2011-12-20 11:21 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
mrbkap: review+
Details
Example .cpp (14.31 KB, text/plain)
2011-12-20 11:29 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
mrbkap: review+
Details

Description Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-11 04:47:32 PST
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] (vacation Aug 25-28) 2011-12-12 12:23:43 PST
Created attachment 581004 [details] [diff] [review]
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] (vacation Aug 25-28) 2011-12-12 12:25:09 PST
Created attachment 581006 [details]
example .h
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-12 12:25:49 PST
Created attachment 581007 [details]
example .cpp
Comment 4 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-12 12:33:20 PST
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] (vacation Aug 25-28) 2011-12-12 13:00:45 PST
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] (vacation Aug 25-28) 2011-12-13 05:59:21 PST
Ah, looks like xpidl.py uses some similar type hacks: endswith('*')
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-13 06:01:42 PST
But anyway, a bit better patch coming.
Comment 8 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-13 06:11:50 PST
Created attachment 581252 [details] [diff] [review]
export the header
Comment 9 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-13 07:06:35 PST
Created attachment 581259 [details] [diff] [review]
export the header in libs::
Comment 10 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-13 08:49:09 PST
Created attachment 581290 [details] [diff] [review]
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] (vacation Aug 25-28) 2011-12-13 12:05:02 PST
Created attachment 581359 [details]
example .h
Comment 12 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-13 12:05:30 PST
Created attachment 581360 [details]
example .cpp
Comment 13 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-13 12:24:32 PST
Created attachment 581372 [details] [diff] [review]
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] (vacation Aug 25-28) 2011-12-13 13:39:18 PST
Created attachment 581404 [details] [diff] [review]
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] (vacation Aug 25-28) 2011-12-13 17:53:20 PST
I should obviously use jsids, not chars*s
Comment 16 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-14 03:52:18 PST
Created attachment 581580 [details] [diff] [review]
use jsids
Comment 17 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-14 10:08:37 PST
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-20 05:50:45 PST
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!) 2011-12-20 06:03:54 PST
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] (vacation Aug 25-28) 2011-12-20 06:07:06 PST
No. The tool wouldn't find all the necessary files at export phase.
Comment 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-20 06:07:50 PST
Nope, the tool needs IDL files that aren't installed until later in export.
Comment 22 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-20 11:18:58 PST
Created attachment 583227 [details] [diff] [review]
patch

remove extra ; and use multiline write
(but not multiline (""" """) strings since that ruins code clarity)
Comment 23 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-20 11:21:23 PST
Created attachment 583228 [details]
Example .h

Blake, could you review that the generated code looks ok.
.h file can't be much simpler.
Comment 24 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-20 11:29:18 PST
Created attachment 583231 [details]
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) 2011-12-22 13:34:36 PST
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) 2011-12-22 13:39:03 PST
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] (vacation Aug 25-28) 2011-12-22 13:40:16 PST
(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] (vacation Aug 25-28) 2011-12-23 13:55:48 PST
> >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] (vacation Aug 25-28) 2011-12-23 15:52:53 PST
https://hg.mozilla.org/mozilla-central/rev/33473863f42c

Note You need to log in before you can comment on or make changes to this bug.