[Pref] cannot be applied to constructor operations
Categories
(Core :: DOM: Bindings (WebIDL), defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox73 | --- | fixed |
People
(Reporter: nordzilla, Assigned: nordzilla)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
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.
Assignee | ||
Comment 1•5 years ago
|
||
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.
Comment 2•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
I would love to work on this and get a chance to learn more about WebIDL. Thank you for your quick and detailed response!
Assignee | ||
Updated•5 years ago
|
Comment 4•5 years ago
|
||
As far as adding tests for this, we'd want to do the following:
- 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.
- 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 aWrapperCachedNonISupportsTestInterface
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. - Probably add some tests to
dom/bindings/test/TestCodeGen.webidl
that exercise various combinations ofPref
andChromeOnly
on constructors. Basically create more test interfaces likeTestAttributesOnTypes
but with our variously-annotated constructor operations, add those interfaces toTestBindingHeader.h
andBindings.conf
, and then we can manually examine the resulting codegen to see how it looks.
Assignee | ||
Comment 5•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 7•5 years ago
|
||
bugherder |
Description
•