Bug 1719747 - Part 3: ListFormat::FormatToParts takes a callback. r=gregtatum,tcampbell

As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1719747#c6,
In Part 1, the ownership of the Span in PartsVector is maintained by
ICU. This method adds a callback so js::intl::ListFormat could copy the
content of the Span to Spidermonkey, and mozilla::intl::ListFormat could
use ScopedICUObject to release the string owned by ICU earlier.

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

intl/components/gtest/TestListFormat.cpp

@@ -119,22 +119,24 @@ TEST(IntlListFormat, FormatToParts)
   MOZ_RELEASE_ASSERT(list.append(MakeStringSpan(u"Alice")));
   MOZ_RELEASE_ASSERT(list.append(MakeStringSpan(u"Bob")));
   MOZ_RELEASE_ASSERT(list.append(MakeStringSpan(u"Charlie")));
-  ListFormat::PartVector parts;
-  ASSERT_TRUE(lf->FormatToParts(list, parts).isOk());
 
-  // 3 elements, and 2 literals.
-  ASSERT_EQ(parts.length(), 5u);
+  ASSERT_TRUE(
+      lf->FormatToParts(list, [](const ListFormat::PartVector& parts) {
+          // 3 elements, and 2 literals.
+          EXPECT_EQ((parts.length()), (5u));
 
-  ASSERT_EQ(parts[0], (ListFormat::Part{ListFormat::PartType::Element,
-                                        MakeStringSpan(u"Alice")}));
-  ASSERT_EQ(parts[1], (ListFormat::Part{ListFormat::PartType::Literal,
-                                        MakeStringSpan(u", ")}));
-  ASSERT_EQ(parts[2], (ListFormat::Part{ListFormat::PartType::Element,
-                                        MakeStringSpan(u"Bob")}));
-  ASSERT_EQ(parts[3], (ListFormat::Part{ListFormat::PartType::Literal,
-                                        MakeStringSpan(u", and ")}));
-  ASSERT_EQ(parts[4], (ListFormat::Part{ListFormat::PartType::Element,
-                                        MakeStringSpan(u"Charlie")}));
+          EXPECT_EQ(parts[0], (ListFormat::Part{ListFormat::PartType::Element,
+                                                MakeStringSpan(u"Alice")}));
+          EXPECT_EQ(parts[1], (ListFormat::Part{ListFormat::PartType::Literal,
+                                                MakeStringSpan(u", ")}));
+          EXPECT_EQ(parts[2], (ListFormat::Part{ListFormat::PartType::Element,
+                                                MakeStringSpan(u"Bob")}));
+          EXPECT_EQ(parts[3], (ListFormat::Part{ListFormat::PartType::Literal,
+                                                MakeStringSpan(u", and ")}));
+          EXPECT_EQ(parts[4], (ListFormat::Part{ListFormat::PartType::Element,
+                                                MakeStringSpan(u"Charlie")}));
+          return true;
+        }).isOk());
 }
 
 }  // namespace mozilla::intl

intl/components/src/ListFormat.cpp

@@ -17,22 +17,13 @@ namespace mozilla::intl {
     return Err(ICUError::InternalError);
   }
 
-  UFormattedList* fl = ulistfmt_openResult(&status);
-  if (U_FAILURE(status)) {
-    return Err(ICUError::InternalError);
-  }
-
-  return UniquePtr<ListFormat>(new ListFormat(fmt, fl));
+  return UniquePtr<ListFormat>(new ListFormat(fmt));
 }
 
 ListFormat::~ListFormat() {
   if (mListFormatter) {
     ulistfmt_close(mListFormatter.GetMut());
   }
-
-  if (mFormattedList) {
-    ulistfmt_closeResult(mFormattedList.GetMut());
-  }
 }
 
 /* static */ UListFormatterType ListFormat::ToUListFormatterType(Type type) {
@@ -62,22 +53,28 @@ ListFormat::~ListFormat() {
   return ULISTFMT_WIDTH_WIDE;
 }
 
-ICUResult ListFormat::FormatToParts(const StringList& list, PartVector& parts) {
+ICUResult ListFormat::FormatToParts(const StringList& list,
+                                    PartsCallback&& callback) {
   UErrorCode status = U_ZERO_ERROR;
 
   mozilla::Vector<const char16_t*, DEFAULT_LIST_LENGTH> u16strings;
   mozilla::Vector<int32_t, DEFAULT_LIST_LENGTH> u16stringLens;
   MOZ_TRY(ConvertStringListToVectors(list, u16strings, u16stringLens));
 
+  UFormattedList* formatted = ulistfmt_openResult(&status);
+  if (U_FAILURE(status)) {
+    return Err(ICUError::InternalError);
+  }
+  ScopedICUObject<UFormattedList, ulistfmt_closeResult> toClose(formatted);
   ulistfmt_formatStringsToResult(mListFormatter.GetConst(), u16strings.begin(),
                                  u16stringLens.begin(), int32_t(list.length()),
-                                 mFormattedList.GetMut(), &status);
+                                 formatted, &status);
   if (U_FAILURE(status)) {
     return Err(ICUError::InternalError);
   }
 
   const UFormattedValue* formattedValue =
-      ulistfmt_resultAsValue(mFormattedList.GetConst(), &status);
+      ulistfmt_resultAsValue(formatted, &status);
   if (U_FAILURE(status)) {
     return Err(ICUError::InternalError);
   }
@@ -93,6 +90,7 @@ ICUResult ListFormat::FormatToParts(const StringList& list, PartVector& parts) {
   mozilla::Span<const char16_t> formattedSpan{formattedChars, formattedSize};
   size_t lastEndIndex = 0;
 
+  PartVector parts;
   auto AppendPart = [&](PartType type, size_t beginIndex, size_t endIndex) {
     if (!parts.emplaceBack(type, formattedSpan.FromTo(beginIndex, endIndex))) {
       return false;
@@ -159,6 +157,9 @@ ICUResult ListFormat::FormatToParts(const StringList& list, PartVector& parts) {
     }
   }
 
+  if (!callback(parts)) {
+    return Err(ICUError::InternalError);
+  }
   return Ok();
 }
 

intl/components/src/ListFormat.h

@@ -4,6 +4,8 @@
 #ifndef intl_components_ListFormat_h_
 #define intl_components_ListFormat_h_
 
+#include <functional>
+
 #include "mozilla/CheckedInt.h"
 #include "mozilla/intl/ICU4CGlue.h"
 #include "mozilla/Result.h"
@@ -12,7 +14,6 @@
 #include "unicode/ulistformatter.h"
 
 struct UListFormatter;
-struct UFormattedList;
 
 namespace mozilla::intl {
 
@@ -109,27 +110,33 @@ class ListFormat final {
   using Part = std::pair<PartType, mozilla::Span<const char16_t>>;
   using PartVector = mozilla::Vector<Part, DEFAULT_LIST_LENGTH>;
 
+  using PartsCallback = std::function<bool(const PartVector& parts)>;
+
   /**
-   * Format the list to a list of parts, and write the result into parts.
+   * Format the list to a list of parts, and the callback will be called with
+   * the formatted parts.
+   *
+   * In the callback, it has a argument of type PartVector&, which is the vector
+   * of the formatted parts. The life-cycle of the PartVector is valid only
+   * during the callback call, so the caller of FormatToParts needs to copy the
+   * data in PartVector to its own storage during the callback.
+   *
    * The PartVector contains mozilla::Span which point to memory which may be
    * overridden when the next format method is called.
    *
    * https://tc39.es/ecma402/#sec-Intl.ListFormat.prototype.formatToParts
    * https://tc39.es/ecma402/#sec-formatlisttoparts
    */
-  ICUResult FormatToParts(const StringList& list, PartVector& parts);
+  ICUResult FormatToParts(const StringList& list, PartsCallback&& callback);
 
  private:
   ListFormat() = delete;
-  ListFormat(UListFormatter* fmt, UFormattedList* fl)
-      : mListFormatter(fmt), mFormattedList(fl) {}
+  explicit ListFormat(UListFormatter* fmt) : mListFormatter(fmt) {}
   ListFormat(const ListFormat&) = delete;
   ListFormat& operator=(const ListFormat&) = delete;
 
   ICUPointer<UListFormatter> mListFormatter =
       ICUPointer<UListFormatter>(nullptr);
-  ICUPointer<UFormattedList> mFormattedList =
-      ICUPointer<UFormattedList>(nullptr);
 
   // Convert StringList to an array of type 'const char16_t*' and an array of
   // int32 for ICU-API.

js/src/builtin/intl/ListFormat.cpp

@@ -243,55 +243,58 @@ static bool FormatList(JSContext* cx, mozilla::intl::ListFormat* lf,
 static bool FormatListToParts(JSContext* cx, mozilla::intl::ListFormat* lf,
                               const mozilla::intl::ListFormat::StringList& list,
                               MutableHandleValue result) {
-  mozilla::intl::ListFormat::PartVector parts;
-  auto formatResult = lf->FormatToParts(list, parts);
+  auto formatResult = lf->FormatToParts(
+      list,
+      [cx,
+       &result](const mozilla::intl::ListFormat::PartVector& parts) -> bool {
+        RootedArrayObject partsArray(
+            cx, NewDenseFullyAllocatedArray(cx, parts.length()));
+        if (!partsArray) {
+          return false;
+        }
+        partsArray->ensureDenseInitializedLength(0, parts.length());
+
+        RootedObject singlePart(cx);
+        RootedValue val(cx);
+
+        size_t index = 0;
+        for (const mozilla::intl::ListFormat::Part& part : parts) {
+          singlePart = NewPlainObject(cx);
+          if (!singlePart) {
+            return false;
+          }
+
+          if (part.first == mozilla::intl::ListFormat::PartType::Element) {
+            val = StringValue(cx->names().element);
+          } else {
+            val = StringValue(cx->names().literal);
+          }
+
+          if (!DefineDataProperty(cx, singlePart, cx->names().type, val)) {
+            return false;
+          }
+
+          JSString* partStr =
+              NewStringCopyN<CanGC>(cx, part.second.data(), part.second.size());
+          if (!partStr) {
+            return false;
+          }
+          val = StringValue(partStr);
+          if (!DefineDataProperty(cx, singlePart, cx->names().value, val)) {
+            return false;
+          }
+
+          partsArray->initDenseElement(index++, ObjectValue(*singlePart));
+        }
+        MOZ_ASSERT(index == parts.length());
+        result.setObject(*partsArray);
+        return true;
+      });
   if (formatResult.isErr()) {
     js::intl::ReportInternalError(cx, formatResult.unwrapErr());
     return false;
   }
 
-  RootedArrayObject partsArray(cx,
-                               NewDenseFullyAllocatedArray(cx, parts.length()));
-  if (!partsArray) {
-    return false;
-  }
-  partsArray->ensureDenseInitializedLength(0, parts.length());
-
-  RootedObject singlePart(cx);
-  RootedValue val(cx);
-
-  size_t index = 0;
-  for (const mozilla::intl::ListFormat::Part& part : parts) {
-    singlePart = NewPlainObject(cx);
-    if (!singlePart) {
-      return false;
-    }
-
-    if (part.first == mozilla::intl::ListFormat::PartType::Element) {
-      val = StringValue(cx->names().element);
-    } else {
-      val = StringValue(cx->names().literal);
-    }
-
-    if (!DefineDataProperty(cx, singlePart, cx->names().type, val)) {
-      return false;
-    }
-
-    JSString* partStr =
-        NewStringCopyN<CanGC>(cx, part.second.data(), part.second.size());
-    if (!partStr) {
-      return false;
-    }
-    val = StringValue(partStr);
-    if (!DefineDataProperty(cx, singlePart, cx->names().value, val)) {
-      return false;
-    }
-
-    partsArray->initDenseElement(index++, ObjectValue(*singlePart));
-  }
-  MOZ_ASSERT(index == parts.length());
-
-  result.setObject(*partsArray);
   return true;
 }