Bug 1613705 - [localization] part3: Introduce passing Registry to Localization to replace BundleGenerator argument. r=emilio,nika

Depends on D104789

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

browser/components/newtab/lib/RemoteL10n.jsm

@@ -184,34 +184,27 @@ class _RemoteL10n {
    */
   _createDOML10n() {
     /* istanbul ignore next */
-    async function* generateBundles(resourceIds) {
+    let useRemoteL10n = Services.prefs.getBoolPref(USE_REMOTE_L10N_PREF, true);
+    if (useRemoteL10n && !L10nRegistry.getInstance().hasSource("cfr")) {
       const appLocale = Services.locale.appLocaleAsBCP47;
-      const appLocales = Services.locale.appLocalesAsBCP47;
       const l10nFluentDir = OS.Path.join(
         OS.Constants.Path.localProfileDir,
         RS_DOWNLOADED_FILE_SUBDIR
       );
-      const fs = new L10nFileSource(
+      let cfrIndexedFileSource = new L10nFileSource(
         "cfr",
         [appLocale],
-        `file://${l10nFluentDir}/`
+        `file://${l10nFluentDir}/`,
+        {
+          addResourceOptions: {
+            allowOverrides: true,
+          },
+        },
+        [`file://${l10nFluentDir}/browser/newtab/asrouter.ftl`]
       );
-      // In the case that the Fluent file has not been downloaded from Remote Settings,
-      // `fetchFile` will return `false` and fall back to the packaged Fluent file.
-      const resource = await fs.fetchFile(appLocale, "asrouter.ftl");
-      for await (let bundle of L10nRegistry.getInstance().generateBundles(
-        appLocales.slice(0, 1),
-        resourceIds
-      )) {
-        // Override built-in messages with the resource loaded from remote settings for
-        // the app locale, i.e. the first item of `appLocales`.
-        if (resource) {
-          bundle.addResource(resource, { allowOverrides: true });
-        }
-        yield bundle;
-      }
-      // Now generating bundles for the rest of locales of `appLocales`.
-      yield* L10nRegistry.generateBundles(appLocales.slice(1), resourceIds);
+      L10nRegistry.getInstance().registerSources([cfrIndexedFileSource]);
+    } else if (!useRemoteL10n && L10nRegistry.getInstance().hasSource("cfr")) {
+      L10nRegistry.getInstance().removeSources(["cfr"]);
     }
 
     return new DOMLocalization(
@@ -222,10 +215,7 @@ class _RemoteL10n {
         "branding/brand.ftl",
         "browser/defaultBrowserNotification.ftl",
       ],
-      false,
-      Services.prefs.getBoolPref(USE_REMOTE_L10N_PREF, true)
-        ? { generateBundles }
-        : {}
+      false
     );
   }
 

dom/chrome-webidl/L10nRegistry.webidl

@@ -14,6 +14,10 @@ enum L10nFileSourceHasFileStatus {
     "unknown"
 };
 
+dictionary FileSourceOptions {
+  FluentBundleAddResourceOptions addResourceOptions = {};
+};
+
 /**
  * The interface represents a single source location for
  * the registry.
@@ -37,7 +41,7 @@ interface L10nFileSource {
    * files not available in the source.
    */
   [Throws]
-  constructor(UTF8String name, sequence<UTF8String> locales, UTF8String prePath, optional sequence<UTF8String> index);
+  constructor(UTF8String name, sequence<UTF8String> locales, UTF8String prePath, optional FileSourceOptions options = {}, optional sequence<UTF8String> index);
 
   /**
    * Tests may want to introduce custom file sources and
@@ -105,9 +109,13 @@ interface FluentBundleAsyncIterator {
   [Alias="@@asyncIterator"] FluentBundleAsyncIterator values();
 };
 
+dictionary L10nRegistryOptions {
+  FluentBundleOptions bundleOptions = {};
+};
+
 [ChromeOnly, Exposed=Window]
 interface L10nRegistry {
-  constructor();
+  constructor(optional L10nRegistryOptions aOptions = {});
 
   static L10nRegistry getInstance();
 

dom/webidl/Localization.webidl

@@ -93,7 +93,8 @@ interface Localization {
    */
   [Throws]
   constructor(sequence<UTF8String> aResourceIds,
-              optional boolean aSync = false);
+              optional boolean aSync = false,
+	      optional L10nRegistry aRegistry);
 
   /**
    * A method for adding resources to the localization context.

intl/l10n/FileSource.cpp

@@ -23,11 +23,14 @@ L10nFileSource::L10nFileSource(RefPtr<const ffi::FileSource> aRaw,
 /* static */
 already_AddRefed<L10nFileSource> L10nFileSource::Create(
     const nsACString& aName, const nsTArray<nsCString>& aLocales,
-    const nsACString& aPrePath, ErrorResult& aRv) {
+    const nsACString& aPrePath, const FileSourceOptions& aOptions,
+    ErrorResult& aRv) {
   ffi::L10nFileSourceStatus status;
 
-  RefPtr<const ffi::FileSource> raw(dont_AddRef(
-      ffi::l10nfilesource_new(&aName, &aLocales, &aPrePath, &status)));
+  bool allowOverrides = aOptions.mAddResourceOptions.mAllowOverrides;
+
+  RefPtr<const ffi::FileSource> raw(dont_AddRef(ffi::l10nfilesource_new(
+      &aName, &aLocales, &aPrePath, allowOverrides, &status)));
 
   if (PopulateError(aRv, status)) {
     return nullptr;
@@ -40,19 +43,22 @@ already_AddRefed<L10nFileSource> L10nFileSource::Create(
 already_AddRefed<L10nFileSource> L10nFileSource::Constructor(
     const GlobalObject& aGlobal, const nsACString& aName,
     const nsTArray<nsCString>& aLocales, const nsACString& aPrePath,
+    const dom::FileSourceOptions& aOptions,
     const Optional<Sequence<nsCString>>& aIndex, ErrorResult& aRv) {
   nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
 
   ffi::L10nFileSourceStatus status;
 
+  bool allowOverrides = aOptions.mAddResourceOptions.mAllowOverrides;
+
   RefPtr<const ffi::FileSource> raw;
   if (aIndex.WasPassed()) {
     raw = dont_AddRef(ffi::l10nfilesource_new_with_index(
         &aName, &aLocales, &aPrePath, aIndex.Value().Elements(),
-        aIndex.Value().Length(), &status));
+        aIndex.Value().Length(), allowOverrides, &status));
   } else {
-    raw = dont_AddRef(
-        ffi::l10nfilesource_new(&aName, &aLocales, &aPrePath, &status));
+    raw = dont_AddRef(ffi::l10nfilesource_new(&aName, &aLocales, &aPrePath,
+                                              allowOverrides, &status));
   }
 
   if (PopulateError(aRv, status)) {

intl/l10n/FileSource.h

@@ -27,11 +27,13 @@ class L10nFileSource : public nsWrapperCache {
 
   static already_AddRefed<L10nFileSource> Create(
       const nsACString& aName, const nsTArray<nsCString>& aLocales,
-      const nsACString& aPrePath, ErrorResult& aRv);
+      const nsACString& aPrePath, const dom::FileSourceOptions& aOptions,
+      ErrorResult& aRv);
 
   static already_AddRefed<L10nFileSource> Constructor(
       const dom::GlobalObject& aGlobal, const nsACString& aName,
       const nsTArray<nsCString>& aLocales, const nsACString& aPrePath,
+      const dom::FileSourceOptions& aOptions,
       const dom::Optional<dom::Sequence<nsCString>>& aIndex, ErrorResult& aRv);
 
   static already_AddRefed<L10nFileSource> CreateMock(

intl/l10n/L10nRegistry.cpp

@@ -114,8 +114,9 @@ NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(L10nRegistry, mGlobal)
 NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(L10nRegistry, AddRef)
 NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(L10nRegistry, Release)
 
-L10nRegistry::L10nRegistry(nsIGlobalObject* aGlobal)
-    : mGlobal(aGlobal), mRaw(dont_AddRef(ffi::l10nregistry_new())) {}
+L10nRegistry::L10nRegistry(nsIGlobalObject* aGlobal, bool aUseIsolating)
+    : mGlobal(aGlobal),
+      mRaw(dont_AddRef(ffi::l10nregistry_new(aUseIsolating))) {}
 
 L10nRegistry::L10nRegistry(nsIGlobalObject* aGlobal,
                            RefPtr<const ffi::GeckoL10nRegistry> aRaw)
@@ -123,9 +124,10 @@ L10nRegistry::L10nRegistry(nsIGlobalObject* aGlobal,
 
 /* static */
 already_AddRefed<L10nRegistry> L10nRegistry::Constructor(
-    const GlobalObject& aGlobal) {
+    const GlobalObject& aGlobal, const L10nRegistryOptions& aOptions) {
   nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
-  return MakeAndAddRef<L10nRegistry>(global);
+  return MakeAndAddRef<L10nRegistry>(global,
+                                     aOptions.mBundleOptions.mUseIsolating);
 }
 
 /* static */

intl/l10n/L10nRegistry.h

@@ -73,12 +73,14 @@ class L10nRegistry final : public nsWrapperCache {
   NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(L10nRegistry)
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(L10nRegistry)
 
-  explicit L10nRegistry(nsIGlobalObject* aGlobal);
+  L10nRegistry(nsIGlobalObject* aGlobal, bool aUseIsolating);
+
   L10nRegistry(nsIGlobalObject* aGlobal,
                RefPtr<const ffi::GeckoL10nRegistry> aRaw);
 
   static already_AddRefed<L10nRegistry> Constructor(
-      const dom::GlobalObject& aGlobal);
+      const dom::GlobalObject& aGlobal,
+      const dom::L10nRegistryOptions& aOptions);
 
   static already_AddRefed<L10nRegistry> GetInstance(
       const dom::GlobalObject& aGlobal);
@@ -119,6 +121,8 @@ class L10nRegistry final : public nsWrapperCache {
   JSObject* WrapObject(JSContext* aCx,
                        JS::Handle<JSObject*> aGivenProto) override;
 
+  const ffi::GeckoL10nRegistry* Raw() const { return mRaw; }
+
  protected:
   virtual ~L10nRegistry() = default;
   nsCOMPtr<nsIGlobalObject> mGlobal;

intl/l10n/Localization.cpp

@@ -108,6 +108,14 @@ Localization::Localization(nsIGlobalObject* aGlobal,
   ffi::localization_new(&aResIds, mIsSync, getter_AddRefs(mRaw));
 }
 
+Localization::Localization(nsIGlobalObject* aGlobal,
+                           const nsTArray<nsCString>& aResIds, bool aIsSync,
+                           const L10nRegistry& aRegistry)
+    : mGlobal(aGlobal), mIsSync(aIsSync) {
+  ffi::localization_new_with_reg(&aResIds, mIsSync, aRegistry.Raw(),
+                                 getter_AddRefs(mRaw));
+}
+
 Localization::Localization(nsIGlobalObject* aGlobal, bool aIsSync)
     : mGlobal(aGlobal), mIsSync(aIsSync) {
   nsTArray<nsCString> resIds;
@@ -122,12 +130,18 @@ Localization::Localization(nsIGlobalObject* aGlobal)
 
 already_AddRefed<Localization> Localization::Constructor(
     const GlobalObject& aGlobal, const Sequence<nsCString>& aResourceIds,
-    bool aIsSync, ErrorResult& aRv) {
+    bool aIsSync, const Optional<NonNull<L10nRegistry>>& aRegistry,
+    ErrorResult& aRv) {
   nsTArray<nsCString> resIds = ToTArray<nsTArray<nsCString>>(aResourceIds);
 
   nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
 
-  return do_AddRef(new Localization(global, resIds, aIsSync));
+  if (aRegistry.WasPassed()) {
+    return do_AddRef(
+        new Localization(global, resIds, aIsSync, aRegistry.Value()));
+  } else {
+    return do_AddRef(new Localization(global, resIds, aIsSync));
+  }
 }
 
 JSObject* Localization::WrapObject(JSContext* aCx,
@@ -230,10 +244,15 @@ already_AddRefed<Promise> Localization::FormatValues(
       mRaw.get(), &l10nKeys, promise,
       // callback function which will be invoked by the rust code, passing the
       // promise back in.
-      [](const Promise* aPromise, const nsTArray<nsCString>* aRaw) {
+      [](const Promise* aPromise, const nsTArray<nsCString>* aValues,
+         const nsTArray<nsCString>* aErrors) {
         Promise* promise = const_cast<Promise*>(aPromise);
 
-        promise->MaybeResolve(*aRaw);
+        if (!aErrors->IsEmpty()) {
+          promise->MaybeRejectWithInvalidStateError(aErrors->ElementAt(0));
+        } else {
+          promise->MaybeResolve(*aValues);
+        }
       });
 
   return MaybeWrapPromise(promise);

intl/l10n/Localization.h

@@ -15,6 +15,7 @@
 #include "mozilla/dom/BindingDeclarations.h"
 #include "mozilla/dom/LocalizationBinding.h"
 #include "mozilla/intl/LocalizationBindings.h"
+#include "mozilla/intl/L10nRegistry.h"
 
 namespace mozilla {
 namespace intl {
@@ -31,6 +32,7 @@ class Localization : public nsIObserver, public nsWrapperCache {
   static already_AddRefed<Localization> Constructor(
       const dom::GlobalObject& aGlobal,
       const dom::Sequence<nsCString>& aResourceIds, bool aIsSync,
+      const dom::Optional<dom::NonNull<L10nRegistry>>& aRegistry,
       ErrorResult& aRv);
   static already_AddRefed<Localization> Create(
       const nsTArray<nsCString>& aResourceIds, bool aIsSync);
@@ -74,6 +76,8 @@ class Localization : public nsIObserver, public nsWrapperCache {
   Localization(const nsTArray<nsCString>& aResIds, bool aIsSync);
   Localization(nsIGlobalObject* aGlobal, const nsTArray<nsCString>& aResIds,
                bool aIsSync);
+  Localization(nsIGlobalObject* aGlobal, const nsTArray<nsCString>& aResIds,
+               bool aIsSync, const L10nRegistry& aRegistry);
   explicit Localization(nsIGlobalObject* aGlobal);
   Localization(nsIGlobalObject* aGlobal, bool aIsSync);
   virtual ~Localization();

intl/l10n/rust/l10nregistry-ffi/src/registry.rs

@@ -165,11 +165,11 @@ pub enum L10nRegistryStatus {
 }
 
 #[no_mangle]
-pub extern "C" fn l10nregistry_new() -> *const GeckoL10nRegistry {
+pub extern "C" fn l10nregistry_new(use_isolating: bool) -> *const GeckoL10nRegistry {
     let env = GeckoEnvironment;
     let mut reg = L10nRegistry::with_provider(env);
     let _ = reg
-        .set_adapt_bundle(GeckoBundleAdapter::default())
+        .set_adapt_bundle(GeckoBundleAdapter { use_isolating })
         .report_error();
     Rc::into_raw(Rc::new(reg))
 }

intl/l10n/rust/l10nregistry-ffi/src/source.rs

@@ -6,7 +6,7 @@ use super::fetcher::{GeckoFileFetcher, MockFileFetcher};
 use crate::env::GeckoEnvironment;
 
 use fluent::FluentResource;
-use l10nregistry::source::{FileSource, ResourceStatus};
+use l10nregistry::source::{FileSource, FileSourceOptions, ResourceStatus};
 
 use nsstring::{nsACString, nsCString};
 use thin_vec::ThinVec;
@@ -29,6 +29,7 @@ pub extern "C" fn l10nfilesource_new(
     name: &nsACString,
     locales: &ThinVec<nsCString>,
     pre_path: &nsACString,
+    allow_override: bool,
     status: &mut L10nFileSourceStatus,
 ) -> *const FileSource {
     if name.is_empty() {
@@ -55,7 +56,7 @@ pub extern "C" fn l10nfilesource_new(
         name.to_string(),
         locales,
         pre_path.to_string(),
-        Default::default(),
+        FileSourceOptions { allow_override },
         GeckoFileFetcher,
     );
     source.set_reporter(GeckoEnvironment);
@@ -71,6 +72,7 @@ pub unsafe extern "C" fn l10nfilesource_new_with_index(
     pre_path: &nsACString,
     index_elements: *const nsCString,
     index_length: usize,
+    allow_override: bool,
     status: &mut L10nFileSourceStatus,
 ) -> *const FileSource {
     if name.is_empty() {
@@ -107,7 +109,7 @@ pub unsafe extern "C" fn l10nfilesource_new_with_index(
         name.to_string(),
         locales,
         pre_path.to_string(),
-        Default::default(),
+        FileSourceOptions { allow_override },
         GeckoFileFetcher,
         index,
     );

intl/l10n/rust/localization-ffi/cbindgen.toml

@@ -12,10 +12,11 @@ line_length = 100
 tab_width = 2
 language = "C++"
 namespaces = ["mozilla", "intl", "ffi"]
+includes = ["mozilla/intl/RegistryBindings.h"]
 
 [parse]
 parse_deps = true
-include = ["fluent-fallback"]
+include = ["fluent-fallback", "l10nregistry-ffi"]
 
 [enum]
 derive_helper_methods = true

intl/l10n/rust/localization-ffi/src/lib.rs

@@ -301,7 +301,7 @@ impl LocalizationRc {
         &self,
         keys: &ThinVec<L10nKey>,
         promise: &xpcom::Promise,
-        callback: extern "C" fn(&xpcom::Promise, &ThinVec<nsCString>),
+        callback: extern "C" fn(&xpcom::Promise, &ThinVec<nsCString>, &ThinVec<nsCString>),
     ) {
         let bundles = self.inner.borrow().bundles().clone();
 
@@ -326,7 +326,11 @@ impl LocalizationRc {
                 })
                 .collect::<ThinVec<_>>();
 
-            callback(&strong_promise, &ret_val);
+            assert_eq!(keys.len(), ret_val.len());
+
+            let errors = errors.into_iter().map(|err| err.to_string().into()).collect();
+
+            callback(&strong_promise, &ret_val, &errors);
         })
         .expect("Failed to spawn future");
     }
@@ -395,6 +399,19 @@ pub extern "C" fn localization_new(
     NS_OK
 }
 
+#[no_mangle]
+pub extern "C" fn localization_new_with_reg(
+    res_ids: &ThinVec<nsCString>,
+    is_sync: bool,
+    reg: &GeckoL10nRegistry,
+    result: &mut *const LocalizationRc,
+) {
+    *result = std::ptr::null_mut();
+
+    let res_ids: Vec<String> = res_ids.iter().map(|res| res.to_string()).collect();
+    *result = RefPtr::forget_into_raw(LocalizationRc::new(&reg, res_ids, is_sync));
+}
+
 #[no_mangle]
 pub unsafe extern "C" fn localization_addref(loc: &LocalizationRc) -> nsrefcnt {
     loc.refcnt.inc()
@@ -483,7 +500,7 @@ pub extern "C" fn localization_format_values(
     loc: &LocalizationRc,
     keys: &ThinVec<L10nKey>,
     promise: &xpcom::Promise,
-    callback: extern "C" fn(&xpcom::Promise, &ThinVec<nsCString>),
+    callback: extern "C" fn(&xpcom::Promise, &ThinVec<nsCString>, &ThinVec<nsCString>),
 ) {
     loc.format_values(keys, promise, callback);
 }

intl/l10n/test/mochitest/localization/test_formatValue.html

@@ -7,17 +7,22 @@
   <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
   <script type="application/javascript">
   "use strict";
-  async function* generateBundles(resourceIds) {
-    const bundle = new FluentBundle("en-US", {
-      useIsolating: false,
-    });
-    bundle.addResource(new FluentResource(`
+  const mockSource = L10nFileSource.createMock("test", ["en-US"], "/localization/{locale}/", [
+    {
+      path: "/localization/en-US/mock.ftl",
+      source: `
 key1 = Value
 key2 = Value { $user }
 key3 = Value { $count }
-    `));
-    yield bundle;
-  }
+`
+    }
+  ]);
+  let registry = new L10nRegistry({
+    bundleOptions: {
+      useIsolating: false
+    }
+  });
+  registry.registerSources([mockSource]);
 
   (async () => {
     SimpleTest.waitForExplicitFinish();
@@ -25,7 +30,7 @@ key3 = Value { $count }
     const loc = new Localization(
       ['mock.ftl'],
       false,
-      { generateBundles },
+      registry,
     );
 
     {
@@ -53,8 +58,8 @@ key3 = Value { $count }
         let val = await loc.formatValue("key3");
         ok(false, "Missing argument didn't cause an exception.");
       } catch (e) {
-        is(e,
-          "[fluent][resolver] errors in en-US/key3: Resolver error: Unknown variable: $count.",
+        is(e.message,
+          "[fluent][resolver] errors in en-US/key3: Resolver error: Unknown variable: $count",
           "Missing key causes an exception.");
       }
     }

intl/l10n/test/mochitest/localization/test_formatValues.html

@@ -7,18 +7,22 @@
   <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
   <script type="application/javascript">
   "use strict";
-  async function* generateBundles(resourceIds) {
-    const bundle = new FluentBundle("en-US",
+  const mockSource = L10nFileSource.createMock("test", ["en-US"], "/localization/{locale}/", [
     {
-      useIsolating: false,
-    });
-    bundle.addResource(new FluentResource(`
+      path: "/localization/en-US/mock.ftl",
+      source: `
 key1 = Value
 key2 = Value { $user }
 key3 = Value { $count }
-    `));
-    yield bundle;
-  }
+`
+    }
+  ]);
+  let registry = new L10nRegistry({
+    bundleOptions: {
+      useIsolating: false
+    }
+  });
+  registry.registerSources([mockSource]);
 
   (async () => {
     SimpleTest.waitForExplicitFinish();
@@ -26,7 +30,7 @@ key3 = Value { $count }
     const loc = new Localization(
       ['mock.ftl'],
       false,
-      { generateBundles },
+      registry,
     );
 
     {
@@ -51,8 +55,8 @@ key3 = Value { $count }
         ]);
         ok(false, "Missing argument didn't cause an exception.");
       } catch (e) {
-        is(e,
-          "[fluent][resolver] errors in en-US/key2: Resolver error: Unknown variable: $user.",
+        is(e.message,
+          "[fluent][resolver] errors in en-US/key2: Resolver error: Unknown variable: $user",
           "Missing key causes an exception.");
       }
     }
@@ -66,8 +70,8 @@ key3 = Value { $count }
         ]);
         ok(false, "Missing key didn't cause an exception.");
       } catch (e) {
-        is(e,
-           "[fluent] Missing translations in en-US: key4.",
+        is(e.message,
+	   "[fluent] Missing message: key4",
            "Missing key causes an exception.");
       }
     }