The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla12

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 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

5 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

5 years ago
Assignee: nobody → bugs
(Assignee)

Comment 1

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

Comment 2

5 years ago
Created attachment 581006 [details]
example .h
(Assignee)

Comment 3

5 years ago
Created attachment 581007 [details]
example .cpp
(Assignee)

Comment 4

5 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

5 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

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

Comment 7

5 years ago
But anyway, a bit better patch coming.
(Assignee)

Comment 8

5 years ago
Created attachment 581252 [details] [diff] [review]
export the header
Attachment #581004 - Attachment is obsolete: true
Attachment #581004 - Flags: feedback?(khuey)
Attachment #581252 - Flags: feedback?(khuey)
(Assignee)

Comment 9

5 years ago
Created attachment 581259 [details] [diff] [review]
export the header in libs::
Attachment #581252 - Attachment is obsolete: true
Attachment #581252 - Flags: feedback?(khuey)
Attachment #581259 - Flags: feedback?(khuey)
(Assignee)

Comment 10

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

Comment 11

5 years ago
Created attachment 581359 [details]
example .h
Attachment #581006 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 581360 [details]
example .cpp
Attachment #581007 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Comment 14

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

Updated

5 years ago
Blocks: 710380
(Assignee)

Updated

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

Comment 15

5 years ago
I should obviously use jsids, not chars*s
(Assignee)

Comment 16

5 years ago
Created attachment 581580 [details] [diff] [review]
use jsids
Attachment #581404 - Attachment is obsolete: true
Attachment #581404 - Flags: review?(khuey)
Attachment #581580 - Flags: review?(khuey)
(Assignee)

Comment 17

5 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

5 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

5 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

5 years ago
Created attachment 583227 [details] [diff] [review]
patch

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

Comment 23

5 years ago
Created attachment 583228 [details]
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

5 years ago
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.
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

5 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

5 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

5 years ago
https://hg.mozilla.org/mozilla-central/rev/33473863f42c
Status: NEW → RESOLVED
Last Resolved: 5 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.