852350 - Support [Pref=foo] decorator on interfaces

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

Doing the webidl bindings for the TextTrack* object hierarchy in bug 833385, it struck me as better to use the [Pref="media.webvtt.enabled"] decorator to hide the new api behind a preference, rather than [PrefControlled] which requires every wrapped class to export a PrefEnabled() method.

Currently [Pref] only works on individual properties, not interfaces. This bug proposes to add support for that, generating code to check the preference directly in the binding.

Comment 1

[Ralph Giles (:rillian)]

Attachment: work in progress

Checkpointing work for the day. New code generator isn't called.

Comment 2

[Boris Zbarsky [:bzbarsky]]

For what it's worth, in for web audio stuff Ehsan just inherited all the classes from one base class that had a static PrefControlled() method on it, and that worked pretty well.

Not that I mind having a way to do this from codegen directly.

Comment 3

[Ralph Giles (:rillian)]

Thanks for the feedback. I did see ehsan's solution.

Comment 4

[Ralph Giles (:rillian)]

Attachment: enable Pref decorator on interfaces

Updated patch. Removed bug prints and advertise the new binding method in the dom object descriptor. Verified to work with the webvtt integration branch.

Are there tests I should be adding to?

Comment 5

[Ralph Giles (:rillian)]

https://tbpl.mozilla.org/?tree=Try&rev=c79ec6d1fdb5

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 726975 [details] [diff] [review]
enable Pref decorator on interfaces

>+    def declare(self):
>+        return CGAbstractMethod.declare(self)
>+
>+    def define(self):
>+        return CGAbstractMethod.define(self)

Just remove all that: if you're not modifying the base class behavior there's no need to override it either.

>+        return "  return Preferences::GetBool(\"%s\");" % pref[0]

Is it worth using a bool var cache?  I guess this is OK for now until we hit a case where we need the performance...

Also, this should probably assert that isinstance(pref, list) and len(pref) == 1.

>+                and desc.interface.getExtendedAttribute("Pref") is None):

I'd prefer the "and" at the end of the previous line.

r=me.  Are you going to update the docs after I land, or should I?

Comment 7

[Ralph Giles (:rillian)]

Attachment: address review comments

Updated patch addressing review comments.

> Just remove all that: if you're not modifying the base class behavior there's > no need to override it either.

Done.

>>+        return "  return Preferences::GetBool(\"%s\");" % pref[0]
>
> Is it worth using a bool var cache?  I guess this is OK for now until we hit > a case where we need the performance...

I mostly seems to be called on document load, so I think this is fine for now.

> Also, this should probably assert that isinstance(pref, list) and len(pref) == 1.

Good point. Assumptions are now explicit.

> I'd prefer the "and" at the end of the previous line.

Fixed.

> Are you going to update the docs after I land, or should I?

I'm happy to update them. Is it just https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings ?

Comment 8

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6873f9a7c4d6

Comment 9

[Ralph Giles (:rillian)]

I've updated https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BPref.3Dprefname.5D

Comment 10

[Boris Zbarsky [:bzbarsky]]

Thank you for the documentation update!

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6873f9a7c4d6