Issue 1502 - Cache translation catalogs on Safari

safari/ext/common.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 84 matching lines @@
       candidates.push(defaultLocale);
 
     return candidates;
   };
 
   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(source)
+  var parseMessage = function(rawMessage)
   {
-    var text = source.message;
+    var text = rawMessage.message;
     var placeholders = [];
 
-    for (var placeholder in source.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 = source.placeholders[placeholder].content;
+      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
     {
       xhr.send();
     }
     catch (e)
     {
       return;
     }
 
     if (xhr.status != 200 && xhr.status != 0)
       return;
 
-    var source = JSON.parse(xhr.responseText);

[kzar, 2014/10/28 19:12:16]

I found it confusing that this variable is called source and then part of it is
passed to the parsing function as a parameter called source. Maybe rename the
source var here or the source parameter above?

[Sebastian Noack, 2014/10/28 19:35:53]

I could argue that this is a different context and "source" is a rather generic
term, like "i" (short for "iteration"), which is used in for-loops all over the
place.

However, if you have a better idea how to call this and/or the other variable, I
can rename them.

[kzar, 2014/10/28 22:05:29]

Perhaps the source variable here could be renamed "messages" and the source
parameter above could be renamed "message"?

[Sebastian Noack, 2014/10/29 08:08:54]

Not really consistent. Since the variable storing the parsed message is already
called just "message", one might assume that "messages" would be a collection of
them.

However, I renamed this variable to "rawMessage" (since its parsed counterpart
is called "message") and the other variable to "rawCatalog" (since its parsed
counterpart is called "catalog").

[kzar, 2014/10/30 12:40:15]

Cool that makes more sense to me now.

-    for (var msgId in source)
+    var rawCatalog = JSON.parse(xhr.responseText);
+    for (var msgId in rawCatalog)
     {
       if (!(msgId in catalog))
-        catalog[msgId] = parseMessage(source[msgId]);
+        catalog[msgId] = parseMessage(rawCatalog[msgId]);
     }
   };
 
   ext.i18n = {
     getMessage: function(msgId, substitutions)
     {
       while (true)
       {
         var message = catalog[msgId];
         if (message)
         {
           var [text, placeholders] = message;

[kzar, 2014/10/28 19:12:16]

I've never seen this syntax in JS before, sure it's valid? (I tried `var [a,b] =
[1,2];` in the console just now and it didn't seem to work.)

[Sebastian Noack, 2014/10/28 19:35:53]

It's called destructuring assignment, and is a new feature in ES6:
http://wiki.ecmascript.org/doku.php?id=harmony:destructuring

It's not yet supported by Safari and Chrome. However, this code is processed by
jshydra, which lets us use some new JavaScript syntax like this, and converts
that line into following code:

  var text = message[0];
  var placeholders = message[1];

[kzar, 2014/10/28 22:05:29]

I guessed you were somehow right :p (Understand about destructuring from other
languages but hadn't realised it was included in ES6!)

 
           if (!(substitutions instanceof Array))
             substitutions = [substitutions];
 
           for (var i = 0; i < placeholders.length; i++)
             text = replacePlaceholder(text, placeholders[i], substitutions[i]);
 
           return text;
         }
 
         if (locales.length == 0)
           return "";
 
         readCatalog(locales.shift());
       }
     }
   };
 
 
   /* Utils */
 
   ext.getURL = function(path)
   {
     return safari.extension.baseURI + path;
   };
 })();