Bug 1719747 - Part 3: ListFormat::FormatToParts takes a callback. r=gregtatum,tcampbell
authorYoshi Cheng-Hao Huang <allstars.chh@gmail.com>
Thu, 09 Sep 2021 12:02:22 +0000
changeset 591489 0433a2711b0a02f9b1b282b52324b84476b3cb4f
parent 591488 1aa6a3fcd6327639c86bc1bacfcbb90a95d10845
child 591490 0e6db6c31531004febb037f974d186f671fc6d3b
push id38774
push usernbeleuzu@mozilla.com
push dateThu, 09 Sep 2021 15:22:41 +0000
treeherdermozilla-central@0e6db6c31531 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgregtatum, tcampbell
bugs1719747
milestone94.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
intl/components/src/ListFormat.cpp
intl/components/src/ListFormat.h
js/src/builtin/intl/ListFormat.cpp
--- a/intl/components/gtest/TestListFormat.cpp
+++ b/intl/components/gtest/TestListFormat.cpp
@@ -114,27 +114,29 @@ TEST(IntlListFormat, FormatBufferLength)
 TEST(IntlListFormat, FormatToParts)
 {
   ListFormat::Options options;
   UniquePtr<ListFormat> lf = ListFormat::TryCreate("en-US", options).unwrap();
   ListFormat::StringList list;
   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
--- a/intl/components/src/ListFormat.cpp
+++ b/intl/components/src/ListFormat.cpp
@@ -12,32 +12,23 @@ namespace mozilla::intl {
 
   UErrorCode status = U_ZERO_ERROR;
   UListFormatter* fmt =
       ulistfmt_openForType(aLocale.data(), utype, uwidth, &status);
   if (U_FAILURE(status)) {
     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) {
   switch (type) {
     case Type::Conjunction:
       return ULISTFMT_TYPE_AND;
     case Type::Disjunction:
       return ULISTFMT_TYPE_OR;
@@ -57,47 +48,54 @@ ListFormat::~ListFormat() {
       return ULISTFMT_WIDTH_SHORT;
     case Style::Narrow:
       return ULISTFMT_WIDTH_NARROW;
   }
   MOZ_ASSERT_UNREACHABLE();
   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);
   }
 
   int32_t formattedCharsLen;
   const char16_t* formattedChars =
       ufmtval_getString(formattedValue, &formattedCharsLen, &status);
   if (U_FAILURE(status)) {
     return Err(ICUError::InternalError);
   }
 
   size_t formattedSize = AssertedCast<size_t>(formattedCharsLen);
   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;
     }
 
     lastEndIndex = endIndex;
     return true;
   };
@@ -154,12 +152,15 @@ ICUResult ListFormat::FormatToParts(cons
 
   // Append any final literal.
   if (lastEndIndex < formattedSize) {
     if (!AppendPart(PartType::Literal, lastEndIndex, formattedSize)) {
       return Err(ICUError::InternalError);
     }
   }
 
+  if (!callback(parts)) {
+    return Err(ICUError::InternalError);
+  }
   return Ok();
 }
 
 }  // namespace mozilla::intl
--- a/intl/components/src/ListFormat.h
+++ b/intl/components/src/ListFormat.h
@@ -1,23 +1,24 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 #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"
 #include "mozilla/ResultVariant.h"
 #include "mozilla/Vector.h"
 #include "unicode/ulistformatter.h"
 
 struct UListFormatter;
-struct UFormattedList;
 
 namespace mozilla::intl {
 
 static constexpr size_t DEFAULT_LIST_LENGTH = 8;
 
 /**
  * This component is a Mozilla-focused API for the list formatting provided by
  * ICU. It implements the API provided by the ECMA-402 Intl.ListFormat object.
@@ -104,37 +105,43 @@ class ListFormat final {
    */
   enum class PartType {
     Element,
     Literal,
   };
   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.
   ICUResult ConvertStringListToVectors(
       const StringList& list,
       mozilla::Vector<const char16_t*, DEFAULT_LIST_LENGTH>& u16strings,
       mozilla::Vector<int32_t, DEFAULT_LIST_LENGTH>& u16stringLens) const {
     // Keep a conservative running count of overall length.
--- a/js/src/builtin/intl/ListFormat.cpp
+++ b/js/src/builtin/intl/ListFormat.cpp
@@ -238,65 +238,68 @@ static bool FormatList(JSContext* cx, mo
 }
 
 /**
  * FormatListToParts ( listFormat, list )
  */
 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;
 }
 
 bool js::intl_FormatList(JSContext* cx, unsigned argc, Value* vp) {
   CallArgs args = CallArgsFromVp(argc, vp);
   MOZ_ASSERT(args.length() == 3);
 
   Rooted<ListFormatObject*> listFormat(