Closed Bug 852350 Opened 9 years ago Closed 9 years ago

Support [Pref=foo] decorator on interfaces

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: rillian, Assigned: rillian)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

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.
Attached patch work in progress (obsolete) — Splinter Review
Checkpointing work for the day. New code generator isn't called.
Assignee: nobody → giles
Blocks: 833385
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.
Thanks for the feedback. I did see ehsan's solution.
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?
Attachment #726423 - Attachment is obsolete: true
Attachment #726975 - Flags: review?(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?
Attachment #726975 - Flags: review?(bzbarsky) → review+
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 ?
Attachment #726975 - Attachment is obsolete: true
Thank you for the documentation update!
https://hg.mozilla.org/mozilla-central/rev/6873f9a7c4d6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.