753522 - Add support for preffed-off methods/properties to Paris bindings

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I have a plan (and partial code) for most of this; still thinking about how to do the XrayWrapper part.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Add support for controlling the existence of methods/properties with a pref.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 622625 [details] [diff] [review]
Add support for controlling the existence of methods/properties with a pref.

Peter, just asking for feedback on general approach for now, not for review yet.

The idea is to replace our old arrays (of JSPropertySpecs, JSFunctionSpecs, ConstantSpecs, jsids) with a pair of arrays:

1)  An array of the same thing as before (JSPropertySpec, say), but with the relevant array terminator inserted at every point where the pref name for two consecutive things in the array is different.

2)  An array of (prefname, pointer) pairs that points to the right place in the first array for that prefname.  Though the same prefname can occur several times here.

As an example, I hacked up one of my webidl files to have some preffed-out things and the generated code looks sort of like this:

  static JSPropertySpec sAttributes_specs[] = {
    /* specs here */
  };

  static Prefable<JSPropertySpec> sAttributes[] = {
    { "", &sAttributes_specs[0] },
    { "foopy", &sAttributes_specs[4] },
    { "", &sAttributes_specs[6] },
    { "foopy", &sAttributes_specs[15] },
    { "loopy", &sAttributes_specs[18] },
    { "", &sAttributes_specs[20] },
    { "x", &sAttributes_specs[22] },
    { "", &sAttributes_specs[25] },
    { NULL, NULL }
  };

where for Prefable<> the first member is the prefname ("" means unconditional, NULL means end of list).  Then consumers who want to work with the list of attributes walk the sAttributes, check that the pref is "" or is a boolean set to true, and if so walk the relevant things from sAttributes_specs by following the pointer in the second member and then walking until the usual "end of list" terminator.

The main consumers who end up having to do that are the property-defining code and the two callbacks for Xrays.

The resulting code is a tad ugly, but should be faster than storing the value of the pref with each property and then checking the prefs on a per-property basis.  I think.

I considered trying to factor out the walking that happens in the two Xray callbacks somehow; if you want me to I can try harder to figure out how to do it.

The other thing that could be done, if desired, is to actually store a boolean, not a string, in the Prefable<> and set up bool var caches on the booleans as needed (and just set the unconditional ones to true).  That would eliminate the need for dynamic pref checking, at the cost of setting up the preference observers when setting up the interface object.  Might well be a win, so I'll probably poke a bit at doing it that way assuming the rest of this approach seems fine.

Note that with this approach flipping the pref will affect Xrays immeditely but will require a reload for direct access, both in chrome and in content.

Also note that I don't support separate prefs for content and chrome, so there is no way, with a pref, to disable in content only.

Anyway, thoughts?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Add support for controlling the existence of methods, properties, interface constants, etc with a pref.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with bool var caches, ready for review

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 622778 [details] [diff] [review]
Now with bool var caches, ready for review

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

::: dom/bindings/BindingUtils.cpp
@@ +41,5 @@
> +  static inline bool
> +  Define(JSContext* cx, JSObject* obj, ConstantSpec* spec) {
> +    return DefineConstants(cx, obj, spec);
> +  }
> +};

Do we actually need Definer? Couldn't these just be static functions (they have different signatures).

::: dom/bindings/Codegen.py
@@ +711,5 @@
> +        getDataTuple is a callback function that takes an array entry and
> +          returns a tuple suitable for substitution into specTemplate.
> +        """
> +
> +        # We want to generate a single last of specs, but with specTerminator

...single list...

@@ +713,5 @@
> +        """
> +
> +        # We want to generate a single last of specs, but with specTerminator
> +        # inserted at every point where the pref name controlling the member
> +        # changes.  That will make sure the order does not change when pref

...order of the properties as exposed on the interface and interface prototype objects...

@@ +2989,5 @@
>  
>          methods = self.properties.methods
>          if methods.hasNonChromeOnly() or methods.hasChromeOnly():
> +            str += """  for (size_t prefIdx = 0; prefIdx < ArrayLength(%(methods)s_ids); ++prefIdx) {
> +    if (%(methods)s_ids[prefIdx].enabled) {

I'm not sure we need to have both ids and spec arrays be Prefable<>. Given that we already rely on them to have exactly the same structure, couldn't we just loop over the spec Prefable<>'s here

@@ +2998,5 @@
> +          // the offset of %(methods)s_ids[prefIdx].specs from
> +          // %(methods)s_id_data to get our index into the full list, which
> +          // should correspond to the index into %(methods)s_specs.
> +          i += %(methods)s_ids[prefIdx].specs - %(methods)s_id_data;
> +          JSFunction *fun = JS_NewFunctionById(cx, %(methods)s_specs[i].call, %(methods)s_specs[i].nargs, 0, wrapper, id);

and drop the |i += ...| bit and just use |%(method)s[prefIdx].specs[i]| here. If we're worried about perf of the two indexes we could cache the JSFunctionSpec* in a local var. But up to you.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Do we actually need Definer? 

Good catch.  No, we do not.

Fixed the comments.

> couldn't we just loop over the spec Prefable<>'s here

We could.  We'd have to compute the indices into the ID array, since the spec Prefable<>s hold pointers which we'd need to subtract the %(methods)s_specs from to get the index.  Let me write that up and see how it looks.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, I think that does simplify things some, both in the codegen and in the generated code.  I'll do that.

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19623c5b3b69

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/19623c5b3b69