Issue 1678 - Added bidi support for 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 86 matching lines @@
       candidates.push(language + "_" + region);
 
     candidates.push(language);
 
     if (candidates.indexOf(defaultLocale) == -1)
       candidates.push(defaultLocale);
 
     return candidates;
   };
 
-  var initCatalog = function(ui_locale)

[Thomas Greiner, 2014/12/10 10:29:42]

Style nit: I do see why you used underscores here but JavaScript variables
shouldn't contain any underscores to keep it consistent with the rest of our
code.

Same applies to `bidi_dir` below.

[Sebastian Noack, 2014/12/10 10:37:29]

That wasn't an aware decision. It happened rather accidentally when I copied the
variable names from the message names. Sure, that should be camel-case.

-  {
-    var bidi_dir = /^(ar|fa|he|ug|ur)_|$/.test(ui_locale) ? "rtl" : "ltr";

[Thomas Greiner, 2014/12/10 10:29:42]

This regular expression will match any string due to the `|` at the end. I'd
assume you wanted to make the underscore optional. In this case just replace `|`
with `?`.

[Sebastian Noack, 2014/12/10 10:37:29]

Ouch, I forgot to set parentheses.
Then "ar_" would match while "ar_SA" wouldn't.

[Thomas Greiner, 2014/12/10 11:12:12]

The corrected version still matches "ar_". By adding `.` after `_` you could
ensure that it won't match. Apart from that it looks good.

[Sebastian Noack, 2014/12/10 11:32:06]

Why should we care? We want to match "ar" and "ar_SA". "ar_" is a theoretical
case we can ignore. And even if it occurs, it can still be considered a variant
of Arabic, where we have to apply right-to-left.

[Thomas Greiner, 2014/12/10 12:06:02]

I can agree with that line of argument.

+  var initCatalog = function(uiLocale)
+  {
+    var bidiDir = /^(ar|fa|he|ug|ur)(_|$)/.test(uiLocale) ? "rtl" : "ltr";

[Wladimir Palant, 2014/12/11 16:52:15]

I only realized now that we are looking at UI locale here rather than our
locale. What will happen if the UI locale is "ur" and we don't have that
translation? I guess that the user will see the fallback to English yet rendered
as RTL. Given that his browser UI is RTL, this might even make sense...

But I guess that this is a purely theoretical discussion. Sebastian mentioned
that Safari only supports ar and he, we have both of them covered.

[Sebastian Noack, 2014/12/11 17:03:05]

To be honest I didn't consider that case and agree that this code is
theoretically wrong, in terms that it considers the language of the browser,
instead the language of the translation used.

However, as you say, currently the only right-to-left languages supported by OS
X are Arabic and Hebrew. So this case won't happen in practice. Also note that
considering the language of the translation would require some additional
complexity since translations are loaded lazily on first use.

     var catalog = Object.create(null);
 
-    catalog["@@ui_locale"] = [ui_locale, []];
-    catalog["@@bidi_dir" ] = [bidi_dir,  []];
+    catalog["@@ui_locale"] = [uiLocale, []];
+    catalog["@@bidi_dir" ] = [bidiDir,  []];
 
     return catalog;
   };
 
   var locales = getLocaleCandidates();
   var catalog = initCatalog(locales[0]);
 
   var replacePlaceholder = function(text, placeholder, content)
   {
     return text.split("$" + placeholder + "$").join(content || "");
@@ skipping 70 matching lines @@
   };
 
 
   /* Utils */
 
   ext.getURL = function(path)
   {
     return safari.extension.baseURI + path;
   };
 })();