Support calling a function to determine whether a property is defined in a WebIDL binding, not just checking a pref

RESOLVED FIXED in mozilla22

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({dev-doc-complete})

unspecified
mozilla22
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

10.88 KB, patch
peterv
: review+
Details | Diff | Splinter Review
14.50 KB, patch
peterv
: review+
Details | Diff | Splinter Review
7.00 KB, patch
peterv
: review+
Details | Diff | Splinter Review
2.67 KB, patch
peterv
: review+
Details | Diff | Splinter Review
13.36 KB, patch
peterv
: review+
Details | Diff | Splinter Review
Need this for bug 838614.
Blocks: 838614
Created attachment 711092 [details] [diff] [review]
part 1.  Add support in Prefable for calling a function to determine whether a property should be exposed in a WebIDL binding.
Attachment #711092 - Flags: review?(peterv)
Created attachment 711093 [details] [diff] [review]
part 2.  Add codegen support for calling a function to determine whether a property should be exposed in a WebIDL binding.
Attachment #711093 - Flags: review?(peterv)
Created attachment 711095 [details] [diff] [review]
part 3.  Switch touch event handler attributes to using a function to check whether they're enabled.
Attachment #711095 - Flags: review?(peterv)
Created attachment 711097 [details] [diff] [review]
part 4.  Add tests for pref-controlled and function-controlled properties.   manually verified that the generated code for this looks like it should.
Attachment #711097 - Flags: review?(peterv)
Created attachment 711098 [details] [diff] [review]
part 5.  Uncomment touch stuff on Document, since now it's controlled on a per-member basis, which is supported even for quickstubs.
Attachment #711098 - Flags: review?(peterv)
Try run: https://tbpl.mozilla.org/?tree=Try&rev=a2d7d23ee48d
Whiteboard: [need review]
Created attachment 711129 [details] [diff] [review]
Part 3 actually compiling

Try run for real: https://tbpl.mozilla.org/?tree=Try&rev=a2d7d23ee48d
Attachment #711129 - Flags: review?(peterv)
Attachment #711095 - Attachment is obsolete: true
Attachment #711095 - Flags: review?(peterv)
Created attachment 711136 [details] [diff] [review]
Part 3 actually compiling for real

Try run for real real: https://tbpl.mozilla.org/?tree=Try&rev=1ba85fe2a5dd
Attachment #711136 - Flags: review?(peterv)
Attachment #711129 - Attachment is obsolete: true
Attachment #711129 - Flags: review?(peterv)
Comment on attachment 711092 [details] [diff] [review]
part 1.  Add support in Prefable for calling a function to determine whether a property should be exposed in a WebIDL binding.

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

::: dom/bindings/BindingUtils.cpp
@@ +649,4 @@
>  }
>  
>  static bool
>  XrayResolveAttribute(JSContext* cx, JSObject* wrapper, jsid id,

Would it make sense to actually pass in the unwrapped object as |JSObject* obj| here too (we already do that for all the Xray* hooks)?

@@ +901,5 @@
>                                     id, desc);
>  }
>  
>  bool
> +XrayEnumerateAttributes(JSContext* cx, JSObject* wrapper,

Add |JSObject* obj| here too.

@@ +924,5 @@
>    return true;
>  }
>  
>  bool
> +XrayEnumerateProperties(JSContext* cx, JSObject* wrapper,

Same here.

@@ +1029,5 @@
>    const NativePropertiesHolder& nativeProperties =
>      nativePropertyHooks->mNativeProperties;
>  
>    if (nativeProperties.regular &&
> +      !XrayEnumerateProperties(cx, wrapper, flags, props, type,

Pass in obj here.

@@ +1036,5 @@
>    }
>  
>    if (nativeProperties.chromeOnly &&
>        xpc::AccessCheck::isChrome(js::GetObjectCompartment(wrapper)) &&
> +      !XrayEnumerateProperties(cx, wrapper, flags, props, type,

And here.

::: dom/bindings/DOMJSClass.h
@@ +69,5 @@
> +  inline bool isEnabled(JSContext* cx, JSObject* obj) {
> +    return enabled &&
> +      (!enabledFunc ||
> +       enabledFunc(cx, js::GetGlobalForObjectCrossCompartment(obj)));
> +  

Trailing whitespace.
Attachment #711092 - Flags: review?(peterv) → review+
> Would it make sense to actually pass in the unwrapped object as |JSObject* obj| here too 

Makes sense.  I'll do that.
Comment on attachment 711093 [details] [diff] [review]
part 2.  Add codegen support for calling a function to determine whether a property should be exposed in a WebIDL binding.

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

::: dom/bindings/BindingUtils.cpp
@@ +716,5 @@
>    }
>    if (methods) {
>      Prefable<JSFunctionSpec>* method;
>      for (method = methods; method->specs; ++method) {
> +      if (method->isEnabled(cx, js::UnwrapObject(wrapper))) {

If you plan to land these separately you should merge this into part 1.

::: dom/bindings/Codegen.py
@@ +1049,2 @@
>              return None
> +            # It's a list of strings

Wrong indentation.

::: dom/bindings/DOMJSClass.h
@@ +69,5 @@
>    inline bool isEnabled(JSContext* cx, JSObject* obj) {
>      return enabled &&
>        (!enabledFunc ||
>         enabledFunc(cx, js::GetGlobalForObjectCrossCompartment(obj)));
> +  }

Again, should probably merge this into part 1.

@@ +76,5 @@
>    bool enabled;
>    // A function pointer to a function that can say the property is disabled
>    // even if "enabled" is set to true.  If the pointer is null the value of
>    // "enabled" is used as-is.
> +  PropertyEnabled enabledFunc;

And this.
Attachment #711093 - Flags: review?(peterv) → review+
Attachment #711136 - Flags: review?(peterv) → review+
Attachment #711097 - Flags: review?(peterv) → review+
Attachment #711098 - Flags: review?(peterv) → review+
> If you plan to land these separately you should merge this into part 1.
> Again, should probably merge this into part 1.
> And this.

Done locally when addressing the comments for part 1.  Sorry I didn't post an updated part 2.  :(

> Wrong indentation.

Fixed.
   https://hg.mozilla.org/integration/mozilla-inbound/rev/83952a3e9b74
   https://hg.mozilla.org/integration/mozilla-inbound/rev/35231869058a
   https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fa1cb51dc8
   https://hg.mozilla.org/integration/mozilla-inbound/rev/20931d1439b2
   https://hg.mozilla.org/integration/mozilla-inbound/rev/136477aeba1e
Whiteboard: [need review]
Blocks: 842372
https://hg.mozilla.org/mozilla-central/rev/83952a3e9b74
https://hg.mozilla.org/mozilla-central/rev/35231869058a
https://hg.mozilla.org/mozilla-central/rev/b5fa1cb51dc8
https://hg.mozilla.org/mozilla-central/rev/20931d1439b2
https://hg.mozilla.org/mozilla-central/rev/136477aeba1e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Documented at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BFunc.3D.22funcname.22.5D
Keywords: dev-doc-complete
You need to log in before you can comment on or make changes to this bug.