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

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks 1 bug)

unspecified
mozilla12
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 10 obsolete attachments)

Assignee

Description

8 years ago
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.
Assignee

Updated

8 years ago
Assignee: nobody → bugs
Assignee

Comment 1

8 years ago
Posted patch WIP (obsolete) — Splinter Review
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.
Assignee

Comment 2

8 years ago
Posted file example .h (obsolete) —
Assignee

Comment 3

8 years ago
Posted file example .cpp (obsolete) —
Assignee

Comment 4

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

Comment 5

8 years ago
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.
Attachment #581004 - Flags: feedback?(khuey)
Assignee

Comment 6

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

Comment 7

8 years ago
But anyway, a bit better patch coming.
Assignee

Comment 8

8 years ago
Posted patch export the header (obsolete) — Splinter Review
Attachment #581004 - Attachment is obsolete: true
Attachment #581004 - Flags: feedback?(khuey)
Attachment #581252 - Flags: feedback?(khuey)
Assignee

Comment 9

8 years ago
Posted patch export the header in libs:: (obsolete) — Splinter Review
Attachment #581252 - Attachment is obsolete: true
Attachment #581252 - Flags: feedback?(khuey)
Attachment #581259 - Flags: feedback?(khuey)
Assignee

Comment 10

8 years ago
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)
Attachment #581259 - Attachment is obsolete: true
Attachment #581259 - Flags: feedback?(khuey)
Attachment #581290 - Flags: feedback?(khuey)
Assignee

Comment 11

8 years ago
Posted file example .h (obsolete) —
Attachment #581006 - Attachment is obsolete: true
Assignee

Comment 12

8 years ago
Posted file example .cpp (obsolete) —
Attachment #581007 - Attachment is obsolete: true
Assignee

Comment 13

8 years ago
Posted patch const jsval* (obsolete) — Splinter Review
jsval as an IDL parameter is converted to const jsval& in C++,
so use const also in generated dictionary handling code.
Attachment #581290 - Attachment is obsolete: true
Attachment #581290 - Flags: feedback?(khuey)
Attachment #581372 - Flags: feedback?(khuey)
Assignee

Comment 14

8 years ago
Posted patch right strip (obsolete) — Splinter Review
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)
Attachment #581372 - Attachment is obsolete: true
Attachment #581372 - Flags: feedback?(khuey)
Assignee

Updated

8 years ago
Blocks: 710380
Assignee

Updated

8 years ago
Attachment #581404 - Flags: review?(khuey)
Assignee

Comment 15

8 years ago
I should obviously use jsids, not chars*s
Assignee

Comment 16

8 years ago
Posted patch use jsidsSplinter Review
Attachment #581404 - Attachment is obsolete: true
Attachment #581404 - Flags: review?(khuey)
Attachment #581580 - Flags: review?(khuey)
Assignee

Comment 17

8 years ago
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.
Attachment #581580 - Flags: review?(khuey) → review?(benjamin)
Assignee

Updated

8 years ago
Blocks: 710882
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++
""")
Attachment #581580 - Flags: review?(benjamin) → review+
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?
Assignee

Comment 20

8 years ago
No. The tool wouldn't find all the necessary files at export phase.
Nope, the tool needs IDL files that aren't installed until later in export.
Assignee

Comment 22

8 years ago
Posted patch patchSplinter Review
remove extra ; and use multiline write
(but not multiline (""" """) strings since that ruins code clarity)
Assignee

Comment 23

8 years ago
Posted file Example .h
Blake, could you review that the generated code looks ok.
.h file can't be much simpler.
Attachment #583228 - Flags: review?(mrbkap)
Assignee

Comment 24

8 years ago
Posted file 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.
Attachment #581359 - Attachment is obsolete: true
Attachment #581360 - Attachment is obsolete: true
Attachment #583231 - Flags: review?(mrbkap)
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.
Attachment #583228 - Flags: review?(mrbkap) → review+
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.
Attachment #583231 - Flags: review?(mrbkap) → review+
Assignee

Comment 27

8 years ago
(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.
Assignee

Comment 28

8 years ago
> >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.
Target Milestone: --- → mozilla11
Assignee

Comment 29

8 years ago
https://hg.mozilla.org/mozilla-central/rev/33473863f42c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: mozilla11 → mozilla12
Depends on: 723894
You need to log in before you can comment on or make changes to this bug.