adblockplusopera: Port UI code from Chrome

background.js

Index: background.js
===================================================================
--- a/background.js
+++ b/background.js
@@ -1,8 +1,13 @@
-var FilterNotifier = require("filterNotifier").FilterNotifier;
-var FilterStorage = require("filterStorage").FilterStorage;
-var Synchronizer = require("synchronizer").Synchronizer;
-var Subscription = require("subscriptionClasses").Subscription;
-var DownloadableSubscription = require("subscriptionClasses").DownloadableSubscription;
+importAll("filterClasses", this);
+importAll("subscriptionClasses", this);
+importAll("filterStorage", this);
+importAll("elemHide", this);
+importAll("filterListener", this);
+importAll("filterNotifier", this);
+importAll("matcher", this);
+importAll("prefs", this);
+importAll("synchronizer", this);
+importAll("utils", this);

[Wladimir Palant, 2012/10/12 15:04:02]

Please don't use importAll, it hides what is actually being imported. Chrome
uses it because the module system used to work like that and introducing
importAll() was easier in the short term than making it work properly.

[Felix Dahlke, 2012/10/16 14:20:01]

Okay, done. I tried to keep it similar to Chrome so I copied even the unused
imports. But background.js differs a lot so that probably doesn't make sense.

[Felix Dahlke, 2012/10/16 14:20:01]

Done. I actually did that to keep the files similar, but they're so different
that that's probably not worth it.

 
 var isFirstRun = false;
 FilterNotifier.addListener(function(action)
@@ -144,3 +149,35 @@
   };
   request.send(null);
 }
+
+function addToolbarIcon()
+{
+  var properties = {
+    disabled: false,
+    title: "Adblock Plus",
+    icon: "icons/abp-18.png"
+  };
+
+  var button = opera.contexts.toolbar.createItem(properties);
+  opera.contexts.toolbar.addItem(button);
+}
+
+function loadMessages(callback)
+{
+  var request = new XMLHttpRequest();
+  request.open("GET", "messages.json");
+  request.responseType = "json";
+  request.addEventListener("load", function()
+  {
+    callback(request.response);
+  });
+  request.send();
+}
+
+addToolbarIcon();
+
+var i18nMessages;
+loadMessages(function(messages)
+{
+  i18nMessages = messages;
+});

[Wladimir Palant, 2012/10/12 15:04:02]

a) Is it necessary to assign explicitly here? Can't this be done by
loadMessages(), without any callback? The file name to be loaded is hardcoded, I
don't see any obvious reason not to hardcode the variable name as well. I would
declare that variable just before loadMessages() for clarity (and also place
loadMessages() call right behind it to group related code).

b) This is asynchronous, are we safe from race conditions here? Please add a
comment noting that this will cause issues with any code trying to access
messages during startup. I *assume* that there will be no issues with the
first-run page when we add it, there might be issues with filter loading code -
it uses strings occasionally (adblockplus_compat doesn't implement
Utils.getString() properly so right now it doesn't matter).

I think that the only synchronous alternative is using a <script> tag but that
would require a change to the messages.json files.

c) Are we using the right language IDs? This is particularly something to be
checked for "pt_BR" and similar, shouldn't it be "pt-BR" in Opera? Also, what if
Opera uses "es-ES" and we provide only "es", will it correctly fall back to our
locale?

[Felix Dahlke, 2012/10/16 14:20:01]

Hm, don't you think it would be less obvious that this happens when the script
is loaded if we don't make the call at the bottom? That's where I expect all
immediately executed code.

Anyway, there's no reason not to work with i18nMessages directly, I've changed
that.
As discussed on IRC, we'll use a synchronous XHR.
 
It seems you're right, they're using a different format. The only documentation
I could find on this is:

http://dev.opera.com/articles/view/creating-multilingual-extensions/

The format is apparently en-gb - all lower case and separated by minus instead
of underscore.

I've written a small script that copies the locales from adblockpluschrome,
renames the folders accordingly and replaces all occurences of "Chrome" with
"Opera". I also had to implement fallback on missing keys, so loadMessages() has
become more complex.