Bug 1388931 - Unship SVGPathSeg APIs behind a preference. r=emilio

Add the preference, dom.svg.pathSeg.enabled, so let
SVGPathElement::getPathSegAtLength(), SVGAnimatedPathData::pathSegList,
and SVGAnimatedPathData::animatedPathSegList behind the preference, and
set the preference to false by default on all channels.

Differential Revision: https://phabricator.services.mozilla.com/D133289

devtools/client/inspector/animation/test/browser.ini

@@ -1,6 +1,7 @@
 [DEFAULT]
 prefs =
   dom.animations.mainthread-synchronization-with-geometric-animations=true
+  dom.svg.pathSeg.enabled=true
 tags = devtools
 subsuite = devtools
 support-files =

dom/svg/SVGAnimatedPathSegList.cpp

@@ -33,18 +33,20 @@ nsresult SVGAnimatedPathSegList::SetBaseValueString(const nsAString& aValue) {
   // DOM items need to copy their internal counterpart's values *before* we
   // change them. See the comments in
   // DOMSVGPathSegList::InternalListWillChangeTo().
-
-  DOMSVGPathSegList* baseValWrapper =
-      DOMSVGPathSegList::GetDOMWrapperIfExists(GetBaseValKey());
-  if (baseValWrapper) {
-    baseValWrapper->InternalListWillChangeTo(newBaseValue);
-  }
-
+  DOMSVGPathSegList* baseValWrapper = nullptr;
   DOMSVGPathSegList* animValWrapper = nullptr;
-  if (!IsAnimating()) {  // DOM anim val wraps our base val too!
-    animValWrapper = DOMSVGPathSegList::GetDOMWrapperIfExists(GetAnimValKey());
-    if (animValWrapper) {
-      animValWrapper->InternalListWillChangeTo(newBaseValue);
+  if (StaticPrefs::dom_svg_pathSeg_enabled()) {
+    baseValWrapper = DOMSVGPathSegList::GetDOMWrapperIfExists(GetBaseValKey());
+    if (baseValWrapper) {
+      baseValWrapper->InternalListWillChangeTo(newBaseValue);
+    }
+
+    if (!IsAnimating()) {  // DOM anim val wraps our base val too!
+      animValWrapper =
+          DOMSVGPathSegList::GetDOMWrapperIfExists(GetAnimValKey());
+      if (animValWrapper) {
+        animValWrapper->InternalListWillChangeTo(newBaseValue);
+      }
     }
   }
 
@@ -56,13 +58,15 @@ nsresult SVGAnimatedPathSegList::SetBaseValueString(const nsAString& aValue) {
 
   nsresult rv2 = mBaseVal.CopyFrom(newBaseValue);
   if (NS_FAILED(rv2)) {
-    // Attempting to increase mBaseVal's length failed (mBaseVal is left
-    // unmodified). We MUST keep any DOM wrappers in sync:
-    if (baseValWrapper) {
-      baseValWrapper->InternalListWillChangeTo(mBaseVal);
-    }
-    if (animValWrapper) {
-      animValWrapper->InternalListWillChangeTo(mBaseVal);
+    if (StaticPrefs::dom_svg_pathSeg_enabled()) {
+      // Attempting to increase mBaseVal's length failed (mBaseVal is left
+      // unmodified). We MUST keep any DOM wrappers in sync:
+      if (baseValWrapper) {
+        baseValWrapper->InternalListWillChangeTo(mBaseVal);
+      }
+      if (animValWrapper) {
+        animValWrapper->InternalListWillChangeTo(mBaseVal);
+      }
     }
     return rv2;
   }
@@ -70,19 +74,21 @@ nsresult SVGAnimatedPathSegList::SetBaseValueString(const nsAString& aValue) {
 }
 
 void SVGAnimatedPathSegList::ClearBaseValue() {
-  // We must send these notifications *before* changing mBaseVal! (See above.)
+  if (StaticPrefs::dom_svg_pathSeg_enabled()) {
+    // We must send these notifications *before* changing mBaseVal! (See above.)
 
-  DOMSVGPathSegList* baseValWrapper =
-      DOMSVGPathSegList::GetDOMWrapperIfExists(GetBaseValKey());
-  if (baseValWrapper) {
-    baseValWrapper->InternalListWillChangeTo(SVGPathData());
-  }
+    DOMSVGPathSegList* baseValWrapper =
+        DOMSVGPathSegList::GetDOMWrapperIfExists(GetBaseValKey());
+    if (baseValWrapper) {
+      baseValWrapper->InternalListWillChangeTo(SVGPathData());
+    }
 
-  if (!IsAnimating()) {  // DOM anim val wraps our base val too!
-    DOMSVGPathSegList* animValWrapper =
-        DOMSVGPathSegList::GetDOMWrapperIfExists(GetAnimValKey());
-    if (animValWrapper) {
-      animValWrapper->InternalListWillChangeTo(SVGPathData());
+    if (!IsAnimating()) {  // DOM anim val wraps our base val too!
+      DOMSVGPathSegList* animValWrapper =
+          DOMSVGPathSegList::GetDOMWrapperIfExists(GetAnimValKey());
+      if (animValWrapper) {
+        animValWrapper->InternalListWillChangeTo(SVGPathData());
+      }
     }
   }
 
@@ -103,12 +109,14 @@ nsresult SVGAnimatedPathSegList::SetAnimValue(const SVGPathData& aNewAnimValue,
   // other DOM list classes, so this is a shame. We'd quite like to be able to
   // skip the call if possible.
 
-  // We must send these notifications *before* changing mAnimVal! (See above.)
+  if (StaticPrefs::dom_svg_pathSeg_enabled()) {
+    // We must send these notifications *before* changing mAnimVal! (See above.)
 
-  DOMSVGPathSegList* domWrapper =
-      DOMSVGPathSegList::GetDOMWrapperIfExists(GetAnimValKey());
-  if (domWrapper) {
-    domWrapper->InternalListWillChangeTo(aNewAnimValue);
+    DOMSVGPathSegList* domWrapper =
+        DOMSVGPathSegList::GetDOMWrapperIfExists(GetAnimValKey());
+    if (domWrapper) {
+      domWrapper->InternalListWillChangeTo(aNewAnimValue);
+    }
   }
   if (!mAnimVal) {
     mAnimVal = MakeUnique<SVGPathData>();
@@ -124,15 +132,17 @@ nsresult SVGAnimatedPathSegList::SetAnimValue(const SVGPathData& aNewAnimValue,
 }
 
 void SVGAnimatedPathSegList::ClearAnimValue(SVGElement* aElement) {
-  // We must send these notifications *before* changing mAnimVal! (See above.)
+  if (StaticPrefs::dom_svg_pathSeg_enabled()) {
+    // We must send these notifications *before* changing mAnimVal! (See above.)
 
-  DOMSVGPathSegList* domWrapper =
-      DOMSVGPathSegList::GetDOMWrapperIfExists(GetAnimValKey());
-  if (domWrapper) {
-    // When all animation ends, animVal simply mirrors baseVal, which may have
-    // a different number of items to the last active animated value.
-    //
-    domWrapper->InternalListWillChangeTo(mBaseVal);
+    DOMSVGPathSegList* domWrapper =
+        DOMSVGPathSegList::GetDOMWrapperIfExists(GetAnimValKey());
+    if (domWrapper) {
+      // When all animation ends, animVal simply mirrors baseVal, which may have
+      // a different number of items to the last active animated value.
+      //
+      domWrapper->InternalListWillChangeTo(mBaseVal);
+    }
   }
   mAnimVal = nullptr;
   aElement->DidAnimatePathSegList();

dom/svg/SVGPathElement.cpp

@@ -219,17 +219,17 @@ SVGPathElement::CreateSVGPathSegCurvetoQuadraticSmoothRel(float x, float y) {
   return pathSeg.forget();
 }
 
+// FIXME: This API is enabled only if dom.svg.pathSeg.enabled is true. This
+// preference is off by default in Bug 1388931, and will be dropped later.
+// So we are not planning to map d property for this API.
 already_AddRefed<DOMSVGPathSegList> SVGPathElement::PathSegList() {
-  // FIXME: This should be removed by Bug 1388931, so we don't add this extra
-  // API from style system. This WebIDL API only supports the SVG d attribute
-  // for now.
   return DOMSVGPathSegList::GetDOMWrapper(mD.GetBaseValKey(), this, false);
 }
 
+// FIXME: This API is enabled only if dom.svg.pathSeg.enabled is true. This
+// preference is off by default in Bug 1388931, and will be dropped later.
+// So we are not planning to map d property for this API.
 already_AddRefed<DOMSVGPathSegList> SVGPathElement::AnimatedPathSegList() {
-  // FIXME: This should be removed by Bug 1388931, so we don't add this extra
-  // API from style system. This WebIDL API only supports the SVG d attribute
-  // for now.
   return DOMSVGPathSegList::GetDOMWrapper(mD.GetAnimValKey(), this, true);
 }
 

dom/svg/test/mochitest.ini

@@ -1,5 +1,6 @@
 [DEFAULT]
 prefs =
+  dom.svg.pathSeg.enabled=true
   layout.css.d-property.enabled=true
 support-files =
   MutationEventChecker.js

dom/tests/mochitest/general/mochitest.ini

@@ -1,4 +1,6 @@
 [DEFAULT]
+prefs =
+  dom.svg.pathSeg.enabled=false
 support-files =
   497633.html
   fail.png

dom/tests/mochitest/general/test_interfaces.js

@@ -1180,8 +1180,6 @@ var interfaceNamesInGlobalScope = [
   // IMPORTANT: Do not change this list without review from a DOM peer!
   { name: "SVGPathElement", insecureContext: true },
   // IMPORTANT: Do not change this list without review from a DOM peer!
-  { name: "SVGPathSegList", insecureContext: true },
-  // IMPORTANT: Do not change this list without review from a DOM peer!
   { name: "SVGPatternElement", insecureContext: true },
   // IMPORTANT: Do not change this list without review from a DOM peer!
   { name: "SVGPoint", insecureContext: true },

dom/webidl/SVGAnimatedPathData.webidl

@@ -11,9 +11,9 @@
  */
 
 interface mixin SVGAnimatedPathData {
+  [Pref="dom.svg.pathSeg.enabled"]
   readonly attribute SVGPathSegList pathSegList;
-  //readonly attribute SVGPathSegList normalizedPathSegList;
+  [Pref="dom.svg.pathSeg.enabled"]
   readonly attribute SVGPathSegList animatedPathSegList;
-  //readonly attribute SVGPathSegList animatedNormalizedPathSegList;
 };
 

dom/webidl/SVGPathElement.webidl

@@ -11,7 +11,7 @@
  */
 [Exposed=Window]
 interface SVGPathElement : SVGGeometryElement {
-
+  [Pref="dom.svg.pathSeg.enabled"]
   unsigned long getPathSegAtLength(float distance);
 };
 

dom/webidl/SVGPathSegList.webidl

@@ -10,7 +10,8 @@
  * liability, trademark and document use rules apply.
  */
 
-[Exposed=Window]
+[Exposed=Window,
+ Pref="dom.svg.pathSeg.enabled"]
 interface SVGPathSegList {
   readonly attribute unsigned long numberOfItems;
   [Throws]

modules/libpref/init/StaticPrefList.yaml

@@ -2931,6 +2931,12 @@
   value: false
   mirror: always
 
+# This enables the SVGPathSeg APIs
+- name: dom.svg.pathSeg.enabled
+  type: bool
+  value: false
+  mirror: always
+
 # Preference that is primarily used for testing of problematic file paths.
 # It can also be used for switching between different storage directories, but
 # such feature is not officially supported.

testing/web-platform/meta/dom/historical.html.ini

@@ -1,4 +1,5 @@
 [historical.html]
+  prefs: [dom.svg.pathSeg.enabled:false]
   [Historical DOM features must be removed: DOMRequest]
     bug: 1499547
     expected: FAIL

testing/web-platform/meta/svg/animations/__dir__.ini

@@ -0,0 +1 @@
+prefs: [dom.svg.pathSeg.enabled:false, layout.css.d-property.enabled:true]

testing/web-platform/meta/svg/animations/animate-path-animation-Cc-Ss.tentative.html.ini

@@ -0,0 +1,5 @@
+[animate-path-animation-Cc-Ss.tentative.html]
+  [Path animation where coordinate modes of start and end differ (C-c and S-s)]
+    expected: FAIL
+    bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1745078
+

testing/web-platform/meta/svg/path/interfaces/SVGAnimatedPathData-removed.svg.ini

@@ -1,6 +1,2 @@
 [SVGAnimatedPathData-removed.svg]
-  expected:
-    if (os == "android") and debug and webrender: ["OK", "CRASH"]
-  [SVGAnimatedPathData-removed]
-    expected: FAIL
-
+  prefs: [dom.svg.pathSeg.enabled:false]

testing/web-platform/tests/dom/historical.html

@@ -29,7 +29,8 @@ var removedInterfaces = [
   "Notation",
   "TypeInfo",
   "UserDataHandler",
-  "RangeException" // DOM Range
+  "RangeException", // DOM Range
+  "SVGPathSegList"
 ]
 removedInterfaces.forEach(isInterfaceRemoved)