Issue 1502 - Cache translation catalogs on Safari

safari/ext/common.js

Index: safari/ext/common.js
===================================================================
--- a/safari/ext/common.js
+++ b/safari/ext/common.js
@@ -77,9 +77,6 @@
 
   /* I18n */
 
-  var localeCandidates = null;
-  var uiLocale;
-
   var getLocaleCandidates = function()
   {
     var candidates = [];
@@ -100,82 +97,81 @@
     return candidates;
   };
 
-  var getCatalog = function(locale)
+  var locales = getLocaleCandidates();
+  var catalog = {__proto__: null, "@@ui_locale": [locales[0], []]};
+
+  var replacePlaceholder = function(text, placeholder, content)
+  {
+    return text.split("$" + placeholder + "$").join(content || "");

[Wladimir Palant, 2014/10/30 23:00:44]

Why not keep the more obvious text.replace(...)? And - no, I don't care which
one is a split millisecond faster.

[Sebastian Noack, 2014/10/30 23:53:01]

Because .replace() with a string as first argument only replaces the first
occurrence of that string. I could have used .replace() with a regex using the g
flag as first argument. However, since the string is variable it would require
escaping. So using .split().join() is simplest way to replace all occurrences of
a variable string.

The old code was just broken. So yes, this change is unrelated. However, I had
to restructure that code anyway, and didn't mind fixing it while doing so.

+  };
+
+  var parseMessage = function(rawMessage)
+  {
+    var text = rawMessage.message;
+    var placeholders = [];
+
+    for (var placeholder in rawMessage.placeholders)

[Wladimir Palant, 2014/10/30 23:00:44]

I didn't expect it but apparently one can iterate over null :/

[Sebastian Noack, 2014/10/30 23:53:01]

You can. Iterating over null (or undefined) has the same result as iterating
over an empty object. While talking about unrelated changes, note that the old
code also didn't mind checking for null here. ;)

+    {
+      var content = rawMessage.placeholders[placeholder].content;
+
+      if (/^\$\d+$/.test(content))
+        placeholders[parseInt(content.substr(1)) - 1] = placeholder;

[Wladimir Palant, 2014/10/30 23:00:44]

Doing both regular expressions and string manipulation is unnecessary here and
obfuscates the meaning. How about:

 var match = /^\$(\d+)$/.exec(content);
 if (match)
    placeholders[parseInt(match[1], 10)] = placeholder;

Note that the second parameter to parseInt() is important. I'm not sure whether
all our supported Safari versions will have parseInt("017") == 17.

[Sebastian Noack, 2014/10/30 23:53:01]

Neither the old code, nor most other calls to parseInt() in this repository,
specify a radix. I'm going to file a separate issue for that.

+      else
+        text = replacePlaceholder(text, placeholder, content);
+    }
+
+    return [text, placeholders];
+  };
+
+  var readCatalog = function(locale)
   {
     var xhr = new XMLHttpRequest();
-
     xhr.open("GET", safari.extension.baseURI + "_locales/" + locale + "/messages.json", false);
 
-    try {
+    try
+    {
       xhr.send();
     }
     catch (e)
     {
-      return null;
+      return;
     }
 
     if (xhr.status != 200 && xhr.status != 0)
-      return null;
+      return;
 
-    return JSON.parse(xhr.responseText);
+    var rawCatalog = JSON.parse(xhr.responseText);
+    for (var msgId in rawCatalog)
+    {
+      if (!(msgId in catalog))
+        catalog[msgId] = parseMessage(rawCatalog[msgId]);
+    }
   };
 
   ext.i18n = {
     getMessage: function(msgId, substitutions)
     {
-      if (!localeCandidates)
+      while (true)
       {
-        localeCandidates = getLocaleCandidates();
-        uiLocale = localeCandidates[0];
-      }
+        var message = catalog[msgId];
+        if (message)
+        {
+          var [text, placeholders] = message;
 
-      if (msgId == "@@ui_locale")
-        return uiLocale;
+          if (!(substitutions instanceof Array))
+            substitutions = [substitutions];
 
-      for (var i = 0; i < localeCandidates.length; i++)
-      {
-        var catalog = getCatalog(localeCandidates[i]);
-        if (!catalog)
-        {
-          // if there is no catalog for this locale
-          // candidate, don't try to load it again
-          localeCandidates.splice(i--, 1);
-          continue;
+          for (var i = 0; i < placeholders.length; i++)
+            text = replacePlaceholder(text, placeholders[i], substitutions[i]);
+
+          return text;
         }
 
-        var msg = catalog[msgId];
-        if (!msg)
-          continue;
+        if (locales.length == 0)
+          return "";
 
-        var msgstr = msg.message;
-        if (!msgstr)
-          continue;
-
-        for (var placeholder in msg.placeholders)
-        {
-          var placeholderDetails = msg.placeholders[placeholder];
-          if (!placeholderDetails || !placeholderDetails.content)
-            continue;
-          if (placeholderDetails.content.indexOf("$") != 0)
-            continue;
-
-          var placeholderIdx = parseInt(placeholderDetails.content.substr(1));
-          if (isNaN(placeholderIdx) || placeholderIdx < 1)
-            continue;
-
-          var placeholderValue;
-          if (typeof substitutions != "string")
-            placeholderValue = substitutions[placeholderIdx - 1];
-          else if (placeholderIdx == 1)
-            placeholderValue = substitutions;
-
-          msgstr = msgstr.replace("$" + placeholder + "$", placeholderValue || "");
-        }
-
-        return msgstr;
+        readCatalog(locales.shift());
       }
-
-      return "";
     }
   };