Auto merge of #24544 - saschanaz:stringifier-attr, r=nox,jdm

Support IDL stringifier attributes

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #7590

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

components/script/dom/bindings/codegen/CodegenRust.py

@@ -3667,6 +3667,8 @@ class CGSpecializedMethod(CGAbstractExternMethod):
 
     @staticmethod
     def makeNativeName(descriptor, method):
+        if method.underlyingAttr:
+            return CGSpecializedGetter.makeNativeName(descriptor, method.underlyingAttr)
         name = method.identifier.name
         nativeName = descriptor.binaryNameFor(name)
         if nativeName == name:
@@ -5762,7 +5764,7 @@ class CGInterfaceTrait(CGThing):
             for m in descriptor.interface.members:
                 if (m.isMethod() and not m.isStatic() and
                         not m.isMaplikeOrSetlikeOrIterableMethod() and
-                        (not m.isIdentifierLess() or m.isStringifier()) and
+                        (not m.isIdentifierLess() or (m.isStringifier() and not m.underlyingAttr)) and
                         not m.isDefaultToJSON()):
                     name = CGSpecializedMethod.makeNativeName(descriptor, m)
                     infallible = 'infallible' in descriptor.getExtendedAttributes(m)
@@ -6172,10 +6174,6 @@ class CGDescriptor(CGThing):
                     cgThings.append(CGSpecializedMethod(descriptor, m))
                     cgThings.append(CGMemberJITInfo(descriptor, m))
             elif m.isAttr():
-                if m.stringifier:
-                    raise TypeError("Stringifier attributes not supported yet. "
-                                    "See https://github.com/servo/servo/issues/7590\n"
-                                    "%s" % m.location)
                 if m.getExtendedAttribute("Unscopable"):
                     assert not m.isStatic()
                     unscopableNames.append(m.identifier.name)

components/script/dom/bindings/codegen/parser/WebIDL.py

@@ -760,6 +760,8 @@ class IDLInterfaceOrInterfaceMixinOrNamespace(IDLObjectWithScope, IDLExposureMix
             # specified, it already has a nonempty exposure global names set.
             if len(m._exposureGlobalNames) == 0:
                 m._exposureGlobalNames.update(self._exposureGlobalNames)
+            if m.isAttr() and m.stringifier:
+                m.expand(self.members)
 
         # resolve() will modify self.members, so we need to iterate
         # over a copy of the member list here.
@@ -1855,6 +1857,9 @@ class IDLDictionary(IDLObjectWithScope):
         self._finished = False
         self.members = list(members)
         self._partialDictionaries = []
+        self._extendedAttrDict = {}
+        self.needsConversionToJS = False
+        self.needsConversionFromJS = False
 
         IDLObjectWithScope.__init__(self, location, parentScope, name)
 
@@ -1988,11 +1993,34 @@ class IDLDictionary(IDLObjectWithScope):
                                   self.identifier.name,
                                   [member.location] + locations)
 
+    def getExtendedAttribute(self, name):
+        return self._extendedAttrDict.get(name, None)
+
     def addExtendedAttributes(self, attrs):
-        if len(attrs) != 0:
+        for attr in attrs:
-            raise WebIDLError("There are no extended attributes that are "
+            identifier = attr.identifier()
-                              "allowed on dictionaries",
+
-                              [attrs[0].location, self.location])
+            if (identifier == "GenerateInitFromJSON" or
+                identifier == "GenerateInit"):
+                if not attr.noArguments():
+                    raise WebIDLError("[%s] must not have arguments" % identifier,
+                                      [attr.location])
+                self.needsConversionFromJS = True
+            elif (identifier == "GenerateConversionToJS" or
+                  identifier == "GenerateToJSON"):
+                if not attr.noArguments():
+                    raise WebIDLError("[%s] must not have arguments" % identifier,
+                                      [attr.location])
+                # ToJSON methods require to-JS conversion, because we
+                # implement ToJSON by converting to a JS object and
+                # then using JSON.stringify.
+                self.needsConversionToJS = True
+            else:
+                raise WebIDLError("[%s] extended attribute not allowed on "
+                                  "dictionaries" % identifier,
+                                  [attr.location])
+
+            self._extendedAttrDict[identifier] = True
 
     def _getDependentObjects(self):
         deps = set(self.members)
@@ -2004,6 +2032,7 @@ class IDLDictionary(IDLObjectWithScope):
         assert self.identifier.name == partial.identifier.name
         self._partialDictionaries.append(partial)
 
+
 class IDLEnum(IDLObjectWithIdentifier):
     def __init__(self, location, parentScope, name, values):
         assert isinstance(parentScope, IDLScope)
@@ -4622,6 +4651,40 @@ class IDLAttribute(IDLInterfaceMember):
     def _getDependentObjects(self):
         return set([self.type])
 
+    def expand(self, members):
+        assert self.stringifier
+        if not self.type.isDOMString() and not self.type.isUSVString():
+            raise WebIDLError("The type of a stringifer attribute must be "
+                              "either DOMString or USVString",
+                              [self.location])
+        identifier = IDLUnresolvedIdentifier(self.location, "__stringifier",
+                                             allowDoubleUnderscore=True)
+        method = IDLMethod(self.location,
+                           identifier,
+                           returnType=self.type, arguments=[],
+                           stringifier=True, underlyingAttr=self)
+        allowedExtAttrs = ["Throws", "NeedsSubjectPrincipal", "Pure"]
+        # Safe to ignore these as they are only meaningful for attributes
+        attributeOnlyExtAttrs = [
+            "CEReactions",
+            "CrossOriginWritable",
+            "SetterThrows",
+        ]
+        for (key, value) in self._extendedAttrDict.items():
+            if key in allowedExtAttrs:
+                if value is not True:
+                    raise WebIDLError("[%s] with a value is currently "
+                                      "unsupported in stringifier attributes, "
+                                      "please file a bug to add support" % key,
+                                      [self.location])
+                method.addExtendedAttributes([IDLExtendedAttribute(self.location, (key,))])
+            elif not key in attributeOnlyExtAttrs:
+                raise WebIDLError("[%s] is currently unsupported in "
+                                  "stringifier attributes, please file a bug "
+                                  "to add support" % key,
+                                  [self.location])
+        members.append(method)
+
 
 class IDLArgument(IDLObjectWithIdentifier):
     def __init__(self, location, identifier, type, optional=False, defaultValue=None, variadic=False, dictionaryMember=False, allowTypeAttributes=False):
@@ -4867,7 +4930,8 @@ class IDLMethod(IDLInterfaceMember, IDLScope):
                  static=False, getter=False, setter=False,
                  deleter=False, specialType=NamedOrIndexed.Neither,
                  legacycaller=False, stringifier=False,
-                 maplikeOrSetlikeOrIterable=None):
+                 maplikeOrSetlikeOrIterable=None,
+                 underlyingAttr=None):
         # REVIEW: specialType is NamedOrIndexed -- wow, this is messed up.
         IDLInterfaceMember.__init__(self, location, identifier,
                                     IDLInterfaceMember.Tags.Method)
@@ -4894,6 +4958,7 @@ class IDLMethod(IDLInterfaceMember, IDLScope):
         assert maplikeOrSetlikeOrIterable is None or isinstance(maplikeOrSetlikeOrIterable, IDLMaplikeOrSetlikeOrIterableBase)
         self.maplikeOrSetlikeOrIterable = maplikeOrSetlikeOrIterable
         self._htmlConstructor = False
+        self.underlyingAttr = underlyingAttr
         self._specialType = specialType
         self._unforgeable = False
         self.dependsOn = "Everything"
@@ -4933,7 +4998,8 @@ class IDLMethod(IDLInterfaceMember, IDLScope):
             assert len(self._overloads) == 1
             overload = self._overloads[0]
             assert len(overload.arguments) == 0
-            assert overload.returnType == BuiltinTypes[IDLBuiltinType.Types.domstring]
+            if not self.underlyingAttr:
+                assert overload.returnType == BuiltinTypes[IDLBuiltinType.Types.domstring]
 
     def isStatic(self):
         return self._static

components/script/dom/bindings/codegen/parser/tests/test_stringifier.py

@@ -44,3 +44,103 @@ def WebIDLTest(parser, harness):
 
     harness.ok(threw, "Should not allow a 'stringifier;' and a 'stringifier()'")
 
+    parser = parser.reset()
+    parser.parse("""
+        interface TestStringifier {
+            stringifier attribute DOMString foo;
+        };
+    """)
+    results = parser.finish()
+    harness.ok(isinstance(results[0].members[0], WebIDL.IDLAttribute),
+               "Stringifier attribute should be an attribute")
+    stringifier = results[0].members[1]
+    harness.ok(isinstance(stringifier, WebIDL.IDLMethod),
+               "Stringifier attribute should insert a method")
+    harness.ok(stringifier.isStringifier(),
+               "Inserted method should be a stringifier")
+
+    parser = parser.reset()
+    parser.parse("""
+        interface TestStringifier {};
+        interface mixin TestStringifierMixin {
+            stringifier attribute DOMString foo;
+        };
+        TestStringifier includes TestStringifierMixin;
+    """)
+    results = parser.finish()
+    harness.ok(isinstance(results[0].members[0], WebIDL.IDLAttribute),
+               "Stringifier attribute should be an attribute")
+    stringifier = results[0].members[1]
+    harness.ok(isinstance(stringifier, WebIDL.IDLMethod),
+               "Stringifier attribute should insert a method")
+    harness.ok(stringifier.isStringifier(),
+               "Inserted method should be a stringifier")
+
+    parser = parser.reset()
+    parser.parse("""
+        interface TestStringifier {
+            stringifier attribute USVString foo;
+        };
+    """)
+    results = parser.finish()
+    stringifier = results[0].members[1]
+    harness.ok(stringifier.signatures()[0][0].isUSVString(),
+               "Stringifier attributes should allow USVString")
+
+    parser = parser.reset()
+    parser.parse("""
+        interface TestStringifier {
+            [Throws, NeedsSubjectPrincipal]
+            stringifier attribute USVString foo;
+        };
+    """)
+    results = parser.finish()
+    stringifier = results[0].members[1]
+    harness.ok(stringifier.getExtendedAttribute("Throws"),
+               "Stringifier attributes should support [Throws]")
+    harness.ok(stringifier.getExtendedAttribute("NeedsSubjectPrincipal"),
+               "Stringifier attributes should support [NeedsSubjectPrincipal]")
+
+    parser = parser.reset()
+    threw = False
+    try:
+        parser.parse("""
+            interface TestStringifier {
+              stringifier attribute ByteString foo;
+            };
+        """)
+        results = parser.finish()
+    except:
+        threw = True
+
+    harness.ok(threw, "Should not allow ByteString")
+
+    parser = parser.reset()
+    threw = False
+    try:
+        parser.parse("""
+            interface TestStringifier {
+              stringifier;
+              stringifier attribute DOMString foo;
+            };
+        """)
+        results = parser.finish()
+    except:
+        threw = True
+
+    harness.ok(threw, "Should not allow a 'stringifier;' and a stringifier attribute")
+
+    parser = parser.reset()
+    threw = False
+    try:
+        parser.parse("""
+            interface TestStringifier {
+              stringifier attribute DOMString foo;
+              stringifier attribute DOMString bar;
+            };
+        """)
+        results = parser.finish()
+    except:
+        threw = True
+
+    harness.ok(threw, "Should not allow multiple stringifier attributes")

components/script/dom/domtokenlist.rs

@@ -177,11 +177,6 @@ impl DOMTokenListMethods for DOMTokenList {
         Ok(())
     }
 
-    // https://dom.spec.whatwg.org/#concept-dtl-serialize
-    fn Stringifier(&self) -> DOMString {
-        self.element.get_string_attribute(&self.local_name)
-    }
-
     // check-tidy: no specs after this line
     fn IndexedGetter(&self, index: u32) -> Option<DOMString> {
         self.Item(index)

components/script/dom/htmlanchorelement.rs

@@ -526,11 +526,6 @@ impl HTMLAnchorElementMethods for HTMLAnchorElement {
         // Step 5.
         self.update_href(url);
     }
-
-    // https://html.spec.whatwg.org/multipage/#dom-hyperlink-href
-    fn Stringifier(&self) -> DOMString {
-        DOMString::from(self.Href().0)
-    }
 }
 
 impl Activatable for HTMLAnchorElement {

components/script/dom/location.rs

@@ -9,7 +9,7 @@ use crate::dom::bindings::error::{Error, ErrorResult, Fallible};
 use crate::dom::bindings::inheritance::Castable;
 use crate::dom::bindings::reflector::{reflect_dom_object, Reflector};
 use crate::dom::bindings::root::{Dom, DomRoot};
-use crate::dom::bindings::str::{DOMString, USVString};
+use crate::dom::bindings::str::USVString;
 use crate::dom::globalscope::GlobalScope;
 use crate::dom::urlhelper::UrlHelper;
 use crate::dom::window::Window;
@@ -246,11 +246,6 @@ impl LocationMethods for Location {
         Ok(())
     }
 
-    // https://html.spec.whatwg.org/multipage/#dom-location-href
-    fn Stringifier(&self) -> Fallible<DOMString> {
-        Ok(DOMString::from(self.GetHref()?.0))
-    }
-
     // https://html.spec.whatwg.org/multipage/#dom-location-search
     fn GetSearch(&self) -> Fallible<USVString> {
         self.check_same_origin_domain()?;

components/script/dom/url.rs

@@ -270,11 +270,6 @@ impl URLMethods for URL {
             .or_init(|| URLSearchParams::new(&self.global(), Some(self)))
     }
 
-    // https://url.spec.whatwg.org/#dom-url-href
-    fn Stringifier(&self) -> DOMString {
-        DOMString::from(self.Href().0)
-    }
-
     // https://url.spec.whatwg.org/#dom-url-username
     fn Username(&self) -> USVString {
         UrlHelper::Username(&self.url.borrow())

components/script/dom/webidls/DOMTokenList.webidl

@@ -22,8 +22,7 @@ interface DOMTokenList {
   void replace(DOMString token, DOMString newToken);
 
   [CEReactions, Pure]
-           attribute DOMString value;
+  stringifier attribute DOMString value;
 
-  stringifier;
   iterable<DOMString?>;
 };

components/script/dom/webidls/HTMLHyperlinkElementUtils.webidl

@@ -4,10 +4,8 @@
 
 // https://html.spec.whatwg.org/multipage/#htmlhyperlinkelementutils
 interface mixin HTMLHyperlinkElementUtils {
-  // [CEReactions]
-  // stringifier attribute USVString href;
   [CEReactions]
-           attribute USVString href;
+  stringifier attribute USVString href;
   readonly attribute USVString origin;
   [CEReactions]
            attribute USVString protocol;
@@ -27,9 +25,4 @@ interface mixin HTMLHyperlinkElementUtils {
            attribute USVString search;
   [CEReactions]
            attribute USVString hash;
-
-  // Adding a separate stringifier method until
-  // https://github.com/servo/servo/issues/7590 adds attribute stringifier
-  // support.
-  stringifier;
 };

components/script/dom/webidls/Location.webidl

@@ -4,7 +4,7 @@
 
 // https://html.spec.whatwg.org/multipage/#location
 [Exposed=Window, Unforgeable] interface Location {
-  /*stringifier*/ [Throws] attribute USVString href;
+  [Throws] stringifier attribute USVString href;
   [Throws] readonly attribute USVString origin;
   [Throws]          attribute USVString protocol;
   [Throws]          attribute USVString host;
@@ -19,9 +19,4 @@
   [Throws] void reload();
 
   //[SameObject] readonly attribute USVString[] ancestorOrigins;
-
-  // This is only doing as well as gecko right now.
-  // https://github.com/servo/servo/issues/7590 is on file for
-  // adding attribute stringifier support.
-  [Throws] stringifier;
 };

components/script/dom/webidls/MediaList.webidl

@@ -3,10 +3,9 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 // https://drafts.csswg.org/cssom/#the-medialist-interface
-// [LegacyArrayClass]
 [Exposed=Window]
 interface MediaList {
-  /* stringifier */ attribute [TreatNullAs=EmptyString] DOMString mediaText;
+  stringifier attribute [TreatNullAs=EmptyString] DOMString mediaText;
   readonly attribute unsigned long length;
   getter DOMString? item(unsigned long index);
   void appendMedium(DOMString medium);

components/script/dom/webidls/URL.webidl

@@ -8,7 +8,7 @@
 interface URL {
   [Throws] constructor(USVString url, optional USVString base);
   [SetterThrows]
-  /*stringifier*/ attribute USVString href;
+  stringifier attribute USVString href;
   readonly attribute USVString origin;
            attribute USVString protocol;
            attribute USVString username;
@@ -27,9 +27,4 @@ interface URL {
   static void revokeObjectURL(DOMString url);
 
   USVString toJSON();
-
-  // This is only doing as well as gecko right now.
-  // https://github.com/servo/servo/issues/7590 is on file for
-  // adding attribute stringifier support.
-  stringifier;
 };

components/script/dom/webidls/WorkerLocation.webidl

@@ -5,7 +5,7 @@
 // https://html.spec.whatwg.org/multipage/#worker-locations
 [Exposed=Worker]
 interface WorkerLocation {
-  /*stringifier*/ readonly attribute USVString href;
+  stringifier readonly attribute USVString href;
   readonly attribute USVString origin;
   readonly attribute USVString protocol;
   readonly attribute USVString host;
@@ -14,9 +14,4 @@ interface WorkerLocation {
   readonly attribute USVString pathname;
   readonly attribute USVString search;
   readonly attribute USVString hash;
-
-  // This is only doing as well as gecko right now.
-  // https://github.com/servo/servo/issues/7590 is on file for
-  // adding attribute stringifier support.
-  stringifier;
 };

components/script/dom/workerlocation.rs

@@ -6,7 +6,7 @@ use crate::dom::bindings::codegen::Bindings::WorkerLocationBinding;
 use crate::dom::bindings::codegen::Bindings::WorkerLocationBinding::WorkerLocationMethods;
 use crate::dom::bindings::reflector::{reflect_dom_object, Reflector};
 use crate::dom::bindings::root::DomRoot;
-use crate::dom::bindings::str::{DOMString, USVString};
+use crate::dom::bindings::str::USVString;
 use crate::dom::urlhelper::UrlHelper;
 use crate::dom::workerglobalscope::WorkerGlobalScope;
 use dom_struct::dom_struct;
@@ -87,9 +87,4 @@ impl WorkerLocationMethods for WorkerLocation {
     fn Search(&self) -> USVString {
         UrlHelper::Search(&self.url)
     }
-
-    // https://html.spec.whatwg.org/multipage/#dom-workerlocation-href
-    fn Stringifier(&self) -> DOMString {
-        DOMString::from(self.Href().0)
-    }
 }

tests/wpt/metadata/css/cssom/MediaList.html.ini

@@ -1,8 +0,0 @@
-[MediaList.html]
-  type: testharness
-  [MediaList]
-    expected: FAIL
-
-  [CSSOM - MediaList interface]
-    expected: FAIL
-

tests/wpt/metadata/css/cssom/idlharness.html.ini

@@ -95,9 +95,6 @@
   [Stringification of sheet.cssRules[2\].cssRules[0\]]
     expected: FAIL
 
-  [MediaList interface: stringifier]
-    expected: FAIL
-
   [CSSImportRule interface: attribute media]
     expected: FAIL