Closed Bug 1604340 Opened 5 years ago Closed 5 years ago

[Pref] cannot be applied to constructor operations

Categories

(Core :: DOM: Bindings (WebIDL), defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox73 --- fixed

People

(Reporter: nordzilla, Assigned: nordzilla)

References

Details

Attachments

(1 file)

Attempted to add identifier == "Pref" to this list of checks. Adding this line allowed the code to compile; however, toggling the flag had no effect. It behaved as though the flag was always on.

I noticed that IDLConstructor invokes IDLMethod's handleExtendedAttribute() function [searchfox]
And that IDLMethod just passes if identifier == "Pref" [searchfox]

So it makes sense that I'm seeing this behavior.

Flags: needinfo?(bzbarsky)

I was able to look at the generated code and manually add the lines that make it work:

// CSSStyleSheetBinding.cpp
...
+ #include "mozilla/StaticPrefs_layout.h"
...
static bool
_constructor(JSContext* cx, unsigned argc, JS::Value* vp)
{
  + if (!StaticPrefs::layout_css_constructable_stylesheets_enabled()) {
  +   return ThrowingConstructor(cx, argc, vp);
  + }
...
}
...

I'm not very familiar with WebIDL yet, so I don't know exactly what it would take to get that code generated, nor do I know why constructor() does not currently support the [Pref] attribute.

Right now the [Pref] attribute controls exposure. That is, if the pref is false the annotated construct is not visible at all. In IDLMethod this is implemented via the IDLInterfaceMember.handleExtendedAttribute call, which stores the value, which is later read in codegen to conditionally define the method on the object it should live on.

In the case of constructors the situation is a bit weird because the thing that is exposed is the interface object, which corresponds to both the interface itself and to the constructor operation... Up until we had constructor operations there wasn't even a way to annotate a constructor with a [Pref], which is why it's not currently supported: the implementation of constructor operations only implemented the extended attributes that made sense or were needed at the time.

I think it's probably OK to allow [Pref] on constructor operations with the slightly different semantic "throw as if it were not a constructor if the pref is not set"; that's how [ChromeOnly] works on them already. In terms of implementing that, you'd add it to the whitelist in IDLConstructor.handleExtendedAttribute and then replace the chromeOnlyCheck bits in CGClassConstructor.generate_code in Codegen.py with something using getConditionList instead. That should handle both [ChromeOnly] and [Pref] cases, and probably allow [Func] and [SecureContext] as well. And while we're here we should change getConditionList to use isChromeOnly() instead of manually poking the extended attribute. And add tests, of course.

Do you want to try to do all that? If not, I can probably just write the patch... just let me know, please.

Flags: needinfo?(bzbarsky) → needinfo?(enordin)
Priority: -- → P1
Summary: [Pref] on a constructor fails to build → [Pref] cannot be applied to constructor operations

I would love to work on this and get a chance to learn more about WebIDL. Thank you for your quick and detailed response!

Flags: needinfo?(enordin)
Assignee: nobody → enordin

As far as adding tests for this, we'd want to do the following:

  1. Add tests to dom/bindings/parser/tests/test_constructor.py that the new extended attributes we want to expose are parsed correctly and are stashed on the constructor operation.
  2. Probably add a Pref-controlled constructor in TestFunctions.webidl (e.g. add such a thing to WrapperCachedNonISupportsTestInterface). Then test that in a test setting "dom.expose_test_interfaces" and then creating an iframe does not let one construct a WrapperCachedNonISupportsTestInterface in that iframe, but then setting the pref we annotate the constructor with allows the constructor to be called. The pref can be "dom.webidl.test1", say.
  3. Probably add some tests to dom/bindings/test/TestCodeGen.webidl that exercise various combinations of Pref and ChromeOnly on constructors. Basically create more test interfaces like TestAttributesOnTypes but with our variously-annotated constructor operations, add those interfaces to TestBindingHeader.h and Bindings.conf, and then we can manually examine the resulting codegen to see how it looks.
Attachment #9116876 - Attachment description: Bug 1604340 - Allow [Pref] for WebIDL constructor → Bug 1604340 - Allow [Pref] for WebIDL constructor r=bzbarsky
Status: NEW → ASSIGNED
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2e89a1d89e56 Allow [Pref] for WebIDL constructor r=bzbarsky
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: